Re: [PATCH v2] mm/damon/tests/vaddr-kunit: init maple tree without MT_FLAGS_LOCK_EXTERN

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* SeongJae Park <sj@kernel.org> [240904 14:53]:
> On Wed, 4 Sep 2024 11:41:28 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > On 9/4/24 10:29, SeongJae Park wrote:
> > > damon_test_three_regions_in_vmas() initializes a maple tree with
> > > MM_MT_FLAGS.  The flags contains MT_FLAGS_LOCK_EXTERN, which means
> > > mt_lock of the maple tree will not be used.  And therefore the maple
> > > tree initialization code skips initialization of the mt_lock.  However,
> > > __link_vmas(), which adds vmas for test to the maple tree, uses the
> > > mt_lock.  In other words, the uninitialized spinlock is used.  The
> > > problem becomes clear when spinlock debugging is turned on, since it
> > > reports spinlock bad magic bug.
> > > 
> > > Fix the issue by excluding MT_FLAGS_LOCK_EXTERN from the maple tree
> > > initialization flags.  Note that we don't use empty flags to make it
> > > further similar to the usage of mm maple tree, and to be prepared for
> > > possible future changes, as suggested by Liam.
> > > 
> > > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > > Closes: https://lore.kernel.org/1453b2b2-6119-4082-ad9e-f3c5239bf87e@roeck-us.net
> > > Fixes: d0cf3dd47f0d ("damon: convert __damon_va_three_regions to use the VMA iterator")
> > > Suggested-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > > Signed-off-by: SeongJae Park <sj@kernel.org>
> > > ---
> > > Changes from v1
> > > (https://lore.kernel.org/20240904004534.1189-1-sj@kernel.org)
> > > - Keep lock usage and update the initialization flags (Liam)
> > 
> > Not sure I understand how this is better. Is the resulting rcu warning
> > considered to be irrelevant or a separate problem ?
> 
> I believe it's a separate potential problem (warning) that deserves a look.
> Meanwhile, this patch is fixing an issue that is definitely a bug, so better to
> fix right now, imho.
> 

Whenever you walk the tree, you need to hold the write lock or the rcu
read lock.  I believe that __damon_va_three_regions() needs to have the
rcu_read_lock()/ruc_read_unlock() around the for_each_vma() loop.

I sent a patch to the old thread that I suspect will fix it - but I
don't see the issue so it is hard to say for sure.

Thanks,
Liam