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

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

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
>