Re: [PATCH v4 1/2] locking/lockdep: Forcing subclasses to have same name pointer as their parent class

Re: [PATCH v4 1/2] locking/lockdep: Forcing subclasses to have same name pointer as their parent class

[Greg KH]

On Mon, Jul 15, 2024 at 09:34:46AM +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>
> ---
>  include/linux/lockdep.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH v4 1/2] locking/lockdep: Forcing subclasses to have same name pointer as their parent class

[botta633]

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>
---
 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

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

[botta633]

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>
---
 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");
-- 
2.45.2

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

[Boqun Feng]

[-- Attachment #1: Type: text/plain, Size: 5633 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Boqun Feng]

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
> 

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

[botta633]

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");
-- 
2.45.2