On Wed, Aug 07, 2024 at 06:00:11AM +0300, ahmed Ehab wrote:
On Sat, Aug 3, 2024 at 3:51 AM Boqun Feng boqun.feng@gmail.com wrote:
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:
The "class and subclass name test" line interrupts the output of testsuite "local_lock tests".Instead of a WARN_ON(), could you look into using dotest() to print "ok" if the test passes, which is consistent with othertests.I wrote it this way: static void lock_class_subclass_X1(void) { const char *name_before_setting_subclass = rwsem_X1.dep_map.name; const char *name_after_setting_subclass;
lockdep_set_subclass(&rwsem_X1, 1); name_after_setting_subclass = rwsem_X1.dep_map.name; debug_locks = name_before_setting_subclass == name_after_setting_subclass;
I think you could use:
DEBUG_LOCK_WARN_ON(name_before_setting_subclass != name_after_setting_subclass);
here.
Regards, Boqun
} ... static void class_subclass_X1_name_test(void) { printk(" --------------------------------------------------------------------------\n"); printk(" | class and subclass name test|\n"); printk(" ---------------------\n");
print_testname("lock class and subclass same name"); dotest(lock_class_subclass_X1, SUCCESS, LOCKTYPE_RWSEM); pr_cont("\n"); } However, assigning a value to debug_locks seems very uncommon. I tried to check other test cases; however, they seem to rely on the method they are testing. Do you have a suggestion for my scenario if I want to compare the names before and after setting the subclass? Or you suggest that I follow a different approach other than comparing the names such as checking debug_locks in lockdep_init_map_type and returning when we have multiple instantiations for lock->name?
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
Regards, Ahmed