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@xxxxxxxxx> wrote:
>
> > On Mon, Jul 15, 2024 at 04:26:38PM +0300, botta633 wrote:
> > > From: Ahmed Ehab <bottaawesome633@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxxxxxxxxxxxx>
> > > Fixes: de8f5e4f2dc1f ("lockdep: Introduce wait-type checks")
> > > Cc: <stable@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > > ---
> > > 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:
signature.asc
Description: PGP signature
