Message-ID: <ZrLaKCEJc8FqI38I@tardis>
Date: Tue, 6 Aug 2024 19:21:28 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: ahmed Ehab <bottaawesome633@...il.com>
Cc: linux-kernel@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
	Waiman Long <longman@...hat.com>, linux-ext4@...r.kernel.org,
	syzkaller@...glegroups.com,
	syzbot+7f4a6f7f7051474e40ad@...kaller.appspotmail.com,
	stable@...r.kernel.org
Subject: Re: [PATCH v4 2/2] locking/lockdep: Testing lock class and subclass
 got the same name pointer

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@...il.com> wrote:
> 
> > On Mon, Jul 15, 2024 at 04:26:38PM +0300, botta633 wrote:
> > > From: Ahmed Ehab <bottaawesome633@...il.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@...kaller.appspotmail.com>
> > > Fixes: de8f5e4f2dc1f ("lockdep: Introduce wait-type checks")
> > > Cc: <stable@...r.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@...il.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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists