[PATCH v4 1/2] locking/lockdep: Forcing subclasses to have same name pointer as their parent class
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: de8f5e4f2dc1f ("lockdep: Introduce wait-type checks") Cc: stable@vger.kernel.org Signed-off-by: Ahmed Ehab bottaawesome633@gmail.com --- v3->v4: - Fixed subject line truncation.
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)
From: Ahmed Ehab bottaawesome633@gmail.com
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: de8f5e4f2dc1f ("lockdep: Introduce wait-type checks") Cc: stable@vger.kernel.org Signed-off-by: Ahmed Ehab bottaawesome633@gmail.com --- v3->v4: - Fixed subject line truncation.
lib/locking-selftest.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c index 6f6a5fc85b42..aeed613799ca 100644 --- a/lib/locking-selftest.c +++ b/lib/locking-selftest.c @@ -2710,6 +2710,25 @@ static void local_lock_3B(void)
}
+ /** + * after setting the subclass the lockdep_map.name changes + * if we initialize a new string literal for the subclass + * we will have a new name pointer + */ +static void class_subclass_X1_name_test(void) +{ + printk(" --------------------------------------------------------------------------\n"); + printk(" | class and subclass name test|\n"); + printk(" ---------------------\n"); + const char *name_before_setting_subclass = rwsem_X1.dep_map.name; + const char *name_after_setting_subclass; + + WARN_ON(!rwsem_X1.dep_map.name); + lockdep_set_subclass(&rwsem_X1, 1); + name_after_setting_subclass = rwsem_X1.dep_map.name; + WARN_ON(name_before_setting_subclass != name_after_setting_subclass); +} + static void local_lock_tests(void) { printk(" --------------------------------------------------------------------------\n"); @@ -2916,6 +2935,8 @@ void locking_selftest(void)
local_lock_tests();
+ class_subclass_X1_name_test(); + print_testname("hardirq_unsafe_softirq_safe"); dotest(hardirq_deadlock_softirq_not_deadlock, FAILURE, LOCKTYPE_SPECIAL); pr_cont("\n");
On Mon, Jul 15, 2024 at 04:26:38PM +0300, botta633 wrote:
From: Ahmed Ehab bottaawesome633@gmail.com
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: de8f5e4f2dc1f ("lockdep: Introduce wait-type checks") Cc: stable@vger.kernel.org
You seems to miss my comment at v2:
https://lore.kernel.org/lkml/ZpRKcHNZfsMuACRG@boqun-archlinux/
, i.e. you don't need the Reported-by, Fixes and Cc tag for the patch that adds a test case.
Signed-off-by: Ahmed Ehab bottaawesome633@gmail.com
v3->v4: - Fixed subject line truncation.
lib/locking-selftest.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c index 6f6a5fc85b42..aeed613799ca 100644 --- a/lib/locking-selftest.c +++ b/lib/locking-selftest.c @@ -2710,6 +2710,25 @@ static void local_lock_3B(void) }
- /**
^ there is a tailing space here, next time you can detect this by using checkpatch. Also "/**" style is especially for function signature comment, you could just use a "/*" here.
- after setting the subclass the lockdep_map.name changes
- if we initialize a new string literal for the subclass
- we will have a new name pointer
- */
+static void class_subclass_X1_name_test(void) +{
- printk(" --------------------------------------------------------------------------\n");
- printk(" | class and subclass name test|\n");
- printk(" ---------------------\n");
- const char *name_before_setting_subclass = rwsem_X1.dep_map.name;
- const char *name_after_setting_subclass;
- WARN_ON(!rwsem_X1.dep_map.name);
- lockdep_set_subclass(&rwsem_X1, 1);
- name_after_setting_subclass = rwsem_X1.dep_map.name;
- WARN_ON(name_before_setting_subclass != name_after_setting_subclass);
+}
static void local_lock_tests(void) { printk(" --------------------------------------------------------------------------\n"); @@ -2916,6 +2935,8 @@ void locking_selftest(void) local_lock_tests();
- class_subclass_X1_name_test();
I got this in the serial log:
[ 0.619454] -------------------------------------------------------------------------- [ 0.621463] | local_lock tests | [ 0.622326] --------------------- [ 0.623211] local_lock inversion 2: ok | [ 0.624904] local_lock inversion 3A: ok | [ 0.626740] local_lock inversion 3B: ok | [ 0.628492] -------------------------------------------------------------------------- [ 0.630513] | class and subclass name test| [ 0.631614] --------------------- [ 0.632502] hardirq_unsafe_softirq_safe: ok |
two problems here:
1) The "class and subclass name test" line interrupts the output of testsuite "local_lock tests".
2) Instead of a WARN_ON(), could you look into using dotest() to print "ok" if the test passes, which is consistent with other tests.
Could you please fix all above problems and send another version of this patch (no need to resend the first one)? Thanks!
Regards, Boqun
print_testname("hardirq_unsafe_softirq_safe"); dotest(hardirq_deadlock_softirq_not_deadlock, FAILURE, LOCKTYPE_SPECIAL); pr_cont("\n"); -- 2.45.2
On Mon, Jul 15, 2024 at 04:26:37PM +0300, 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: de8f5e4f2dc1f ("lockdep: Introduce wait-type checks") Cc: stable@vger.kernel.org Signed-off-by: Ahmed Ehab bottaawesome633@gmail.com
This looks good to me.
Regards, Boqun
v3->v4: - Fixed subject line truncation.
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)
-- 2.45.2
Hi Ahmed,
Sorry I was missing some points on title and changelog, please see below and refer to Documentation/process/maintainer-tip.rst:
The title needs to be something like:
locking/lockdep: Avoid creating new name string literals in lockdep_set_subclass()
the title and the changlog needs to be in imperative mode.
On Mon, Jul 15, 2024 at 04:26:37PM +0300, botta633 wrote:
From: Ahmed Ehab bottaawesome633@gmail.com
Preventing lockdep_set_subclass from creating a new instance of the
Same here, s/Preventing/Prevent, and when you reference a function, you want to do "lockdep_set_subclass()" instead of "lockdep_set_subclass".
Besides, overall, you want to structure your changelog as follow:
Syzbot reports a problem that a warning will be triggered while searching a lock class in look_up_lock_class().
// ^ the problem statement
The cause of the issue is that instead of the existing name of a lock class, a new name (a string literal) is created and used by lockdep_set_subclass(), and this results in two lock classes with the same key but different address of the names, and a WARN_ONCE() is triggered because of that in look_up_lock_class().
// ^ the analysis of the problem, you can merge the above two into one // paragraph if that works for you.
To fix this, change lockdep_set_subclass() to use the existing name instead of a new one. As a result no new name will be created by lockdep_set_subclass(), hence the warning is avoided.
// ^ the fix.
Please send a new version if these make sense to you. The patch #2 also needs some changes in the title and changelog, but since that's adding a new test instead of fixing an issue, you could just write something like:
Add a test case to ensure that no new name string literal will be created in lockdep_set_subclass(), otherwise a warning will be triggered in look_up_lock_class(). Add this to catch the problem in the future.
Thanks!
Regards, Boqun
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: de8f5e4f2dc1f ("lockdep: Introduce wait-type checks") Cc: stable@vger.kernel.org Signed-off-by: Ahmed Ehab bottaawesome633@gmail.com
v3->v4: - Fixed subject line truncation.
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)
-- 2.45.2
linux-stable-mirror@lists.linaro.org
-
Boqun Feng -
botta633