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:

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.

...

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