From: Ahmed Ehab bottaawesome633@gmail.com
Preventing lockdep_set_subclass from creating a new instance of the string literal. Hence, we will always have the same class->name among parent and subclasses. This prevents kernel panics when looking up a lock class while comparing class locks and class names.
Reported-by: syzbot+7f4a6f7f7051474e40ad@syzkaller.appspotmail.com Fixes: fd5e3f5fe27 Cc: stable@vger.kernel.org Signed-off-by: Ahmed Ehab bottaawesome633@gmail.com --- include/linux/lockdep.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 08b0d1d9d78b..df8fa5929de7 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -173,7 +173,7 @@ static inline void lockdep_init_map(struct lockdep_map *lock, const char *name, (lock)->dep_map.lock_type)
#define lockdep_set_subclass(lock, sub) \ - lockdep_init_map_type(&(lock)->dep_map, #lock, (lock)->dep_map.key, sub,\ + lockdep_init_map_type(&(lock)->dep_map, (lock)->dep_map.name, (lock)->dep_map.key, sub,\ (lock)->dep_map.wait_type_inner, \ (lock)->dep_map.wait_type_outer, \ (lock)->dep_map.lock_type)
Checking if the lockdep_map->name will change when setting the subclass. It shouldn't change so that the lock class and subclass will have the same name
Reported-by: syzbot+7f4a6f7f7051474e40ad@syzkaller.appspotmail.com Fixes: fd5e3f5fe27 Cc: stable@vger.kernel.org Signed-off-by: botta633 bottaawesome633@gmail.com --- lib/locking-selftest.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c index 6f6a5fc85b42..1d7885205f36 100644 --- a/lib/locking-selftest.c +++ b/lib/locking-selftest.c @@ -2710,12 +2710,24 @@ static void local_lock_3B(void)
}
+static void class_subclass_X1_name(void) +{ + const char *name_before_subclass = rwsem_X1.dep_map.name; + const char *name_after_subclass; + + WARN_ON(!rwsem_X1.dep_map.name); + lockdep_set_subclass(&rwsem_X1, 1); + WARN_ON(name_before_subclass != name_after_subclass); +} + static void local_lock_tests(void) { printk(" --------------------------------------------------------------------------\n"); printk(" | local_lock tests |\n"); printk(" ---------------------\n");
+ init_class_X(&lock_X1, &rwlock_X1, &mutex_X1, &rwsem_X1); + print_testname("local_lock inversion 2"); dotest(local_lock_2, SUCCESS, LOCKTYPE_LL); pr_cont("\n"); @@ -2727,6 +2739,10 @@ static void local_lock_tests(void) print_testname("local_lock inversion 3B"); dotest(local_lock_3B, FAILURE, LOCKTYPE_LL); pr_cont("\n"); + + print_testname("Class and subclass"); + dotest(class_subclass_X1_name, SUCCESS, LOCKTYPE_RWSEM); + pr_cont("\n"); }
static void hardirq_deadlock_softirq_not_deadlock(void)
On Sun, Jul 14, 2024 at 08:14:27AM +0300, botta633 wrote:
First, the subsystem tag also needs to be "locking/lockdep" or "lockdep".
Checking if the lockdep_map->name will change when setting the subclass. It shouldn't change so that the lock class and subclass will have the same name
Could you make the commit log wrapped at 75 columns?
Reported-by: syzbot+7f4a6f7f7051474e40ad@syzkaller.appspotmail.com Fixes: fd5e3f5fe27 Cc: stable@vger.kernel.org
Since this is only adding test for a bug fix, you don't need to put these tags here.
Signed-off-by: botta633 bottaawesome633@gmail.com
Again, could you please put your name here?
Also seems that you send two patch #2, one is with the proper version number "v2", but not in-reply-to the patch #1, the other doesn't have the correct version number but has the correct "in-reply-to" field. Could you use the correct version number and "in-reply-to" next time?
lib/locking-selftest.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c index 6f6a5fc85b42..1d7885205f36 100644 --- a/lib/locking-selftest.c +++ b/lib/locking-selftest.c @@ -2710,12 +2710,24 @@ static void local_lock_3B(void) } +static void class_subclass_X1_name(void) +{
- const char *name_before_subclass = rwsem_X1.dep_map.name;
- const char *name_after_subclass;
- WARN_ON(!rwsem_X1.dep_map.name);
- lockdep_set_subclass(&rwsem_X1, 1);
- WARN_ON(name_before_subclass != name_after_subclass);
Could you add some comments here explaining your test? For example, where name_after_subclass gets set?
+}
static void local_lock_tests(void) {
Please don't add this test into another test, you could directly call your class_subclass_X1_name() (maybe rename it to *_test()) in lockding_selftest() function.
Besides, make sure you run the test with and without your modification in patch #1, and confirm this is the test that could verify your fix.
Regards, Boqun
printk(" --------------------------------------------------------------------------\n"); printk(" | local_lock tests |\n"); printk(" ---------------------\n");
- init_class_X(&lock_X1, &rwlock_X1, &mutex_X1, &rwsem_X1);
- print_testname("local_lock inversion 2"); dotest(local_lock_2, SUCCESS, LOCKTYPE_LL); pr_cont("\n");
@@ -2727,6 +2739,10 @@ static void local_lock_tests(void) print_testname("local_lock inversion 3B"); dotest(local_lock_3B, FAILURE, LOCKTYPE_LL); pr_cont("\n");
- print_testname("Class and subclass");
- dotest(class_subclass_X1_name, SUCCESS, LOCKTYPE_RWSEM);
- pr_cont("\n");
} static void hardirq_deadlock_softirq_not_deadlock(void) -- 2.45.2
On 7/14/24 01:14, botta633 wrote:
From: Ahmed Ehab bottaawesome633@gmail.com
Preventing lockdep_set_subclass from creating a new instance of the string literal. Hence, we will always have the same class->name among parent and subclasses. This prevents kernel panics when looking up a lock class while comparing class locks and class names.
Reported-by: syzbot+7f4a6f7f7051474e40ad@syzkaller.appspotmail.com Fixes: fd5e3f5fe27 Cc: stable@vger.kernel.org Signed-off-by: Ahmed Ehab bottaawesome633@gmail.com
include/linux/lockdep.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 08b0d1d9d78b..df8fa5929de7 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -173,7 +173,7 @@ static inline void lockdep_init_map(struct lockdep_map *lock, const char *name, (lock)->dep_map.lock_type) #define lockdep_set_subclass(lock, sub) \
- lockdep_init_map_type(&(lock)->dep_map, #lock, (lock)->dep_map.key, sub,\
- lockdep_init_map_type(&(lock)->dep_map, (lock)->dep_map.name, (lock)->dep_map.key, sub,\ (lock)->dep_map.wait_type_inner, \ (lock)->dep_map.wait_type_outer, \ (lock)->dep_map.lock_type)
ext4 is a filesystem. It has nothing to do with locking/lockdep. Could you resend the patches with the proper prefix of "lockdep:" or "locking/lockdep:"?
Thanks, Longman
linux-stable-mirror@lists.linaro.org