Re: [PATCH 2/2] ext4: Testing lock class and subclass got the same name pointer

[Boqun Feng <boqun.feng@gmail.com>]

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
>