linux-kernel - Re: [PATCH] mm/damon/tests/vaddr-kunit: don't use mas_lock for MM_MT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e83dedb2-89a3-4327-9a2f-610d3199f0e1@roeck-us.net>
Date: Tue, 3 Sep 2024 19:38:39 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>, SeongJae Park
 <sj@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>,
 "Matthew Wilcox (Oracle)" <willy@...radead.org>,
 David Hildenbrand <david@...hat.com>,
 Brendan Higgins <brendanhiggins@...gle.com>, David Gow
 <davidgow@...gle.com>, damon@...ts.linux.dev, linux-mm@...ck.org,
 kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/damon/tests/vaddr-kunit: don't use mas_lock for
 MM_MT_FLAGS-initialized maple tree

On 9/3/24 19:31, Liam R. Howlett wrote:
> * SeongJae Park <sj@...nel.org> [240903 21:18]:
>> On Tue,  3 Sep 2024 17:58:15 -0700 SeongJae Park <sj@...nel.org> wrote:
>>
>>> On Tue, 3 Sep 2024 20:48:53 -0400 "Liam R. Howlett" <Liam.Howlett@...cle.com> wrote:
>>>
>>>> * SeongJae Park <sj@...nel.org> [240903 20:45]:
>>>>> 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 celar when spinlock debugging is turned on, since it
>>>>> reports spinlock bad magic bug.  Fix the issue by not using the mt_lock
>>>>> as promised.
>>>>
>>>> You can't do this, lockdep will tell you this is wrong.
>>>
>>> Hmm, but lockdep was silence on my setup?
>>>
>>>> We need a lock and to use the lock for writes.
>>>
>>> This code is executed by a single-thread test code.  Do we still need the lock?
>>>
>>>>
>>>> I'd suggest using different flags so the spinlock is used.
>>>
>>> The reporter mentioned simply dropping MT_FLAGS_LOCK_EXTERN from the flags
>>> causes suspicious RCU usage message.  May I ask if you have a suggestion of
>>> better flags?
> 
> That would be the lockdep complaining, so that's good.
> 
>>
>> I was actually thinking replacing the mt_init_flags() with mt_init(), which
>> same to mt_init_flags() with zero flag, like below.
> 
> Yes.  This will use the spinlock which should fix your issue, but it
> will use a different style of maple tree.
> 
> Perhaps use MT_FLAGS_ALLOC_RANGE to use the same type of maple tree, if
> you ever add threading you will want the rcu flag as well
> (MT_FLAGS_USE_RCU).
> 
> I would recommend those two and just use the spinlock.
> 

I tried that (MT_FLAGS_ALLOC_RANGE | MT_FLAGS_USE_RCU). it also triggers
the suspicious RCU usage message.

Guenter

>>
>> ```
>> --- a/mm/damon/tests/vaddr-kunit.h
>> +++ b/mm/damon/tests/vaddr-kunit.h
>> @@ -77,7 +77,7 @@ static void damon_test_three_regions_in_vmas(struct kunit *test)
>>                  (struct vm_area_struct) {.vm_start = 307, .vm_end = 330},
>>          };
>>
>> -       mt_init_flags(&mm.mm_mt, MM_MT_FLAGS);
>> +       mt_init(&mm.mm_mt);
>>          if (__link_vmas(&mm.mm_mt, vmas, ARRAY_SIZE(vmas)))
>>                  kunit_skip(test, "Failed to create VMA tree");
>> ```
>>
>> And just confirmed it also convinces the reproducer.  But because I'm obviously
>> not familiar with maple tree, would like to hear some comments from Liam or
>> others first.
>>
>> FYI, I ended up writing v1 to simply remove lock usage based on my humble
>> understanding of the documetnation.
>>
>>      The maple tree uses a spinlock by default, but external locks can be used for
>>      tree updates as well.  To use an external lock, the tree must be initialized
>>      with the ``MT_FLAGS_LOCK_EXTERN flag``, this is usually done with the
>>      MTREE_INIT_EXT() #define, which takes an external lock as an argument.
>>
>> (from Documentation/core-api/maple_tree.rst)
>>
>> I was thinking the fact that the test code is executed in single thread is same
>> to use of external lock.  I will be happy to learn if I missed something.
>>
>>
>> Thanks,
>> SJ
>>
>>>
>>>
>>> Thanks,
>>> SJ
>>>
>>>>
>>>>>
>>>>> Reported-by: Guenter Roeck <linux@...ck-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")
>>>>> Signed-off-by: SeongJae Park <sj@...nel.org>
>>>>> ---
>>>>>   mm/damon/tests/vaddr-kunit.h | 10 +++-------
>>>>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/mm/damon/tests/vaddr-kunit.h b/mm/damon/tests/vaddr-kunit.h
>>>>> index 83626483f82b..c6c7e0e0ab07 100644
>>>>> --- a/mm/damon/tests/vaddr-kunit.h
>>>>> +++ b/mm/damon/tests/vaddr-kunit.h
>>>>> @@ -17,23 +17,19 @@
>>>>>   static int __link_vmas(struct maple_tree *mt, struct vm_area_struct *vmas,
>>>>>   			ssize_t nr_vmas)
>>>>>   {
>>>>> -	int i, ret = -ENOMEM;
>>>>> +	int i;
>>>>>   	MA_STATE(mas, mt, 0, 0);
>>>>>   
>>>>>   	if (!nr_vmas)
>>>>>   		return 0;
>>>>>   
>>>>> -	mas_lock(&mas);
>>>>>   	for (i = 0; i < nr_vmas; i++) {
>>>>>   		mas_set_range(&mas, vmas[i].vm_start, vmas[i].vm_end - 1);
>>>>>   		if (mas_store_gfp(&mas, &vmas[i], GFP_KERNEL))
>>>>> -			goto failed;
>>>>> +			return -ENOMEM;
>>>>>   	}
>>>>>   
>>>>> -	ret = 0;
>>>>> -failed:
>>>>> -	mas_unlock(&mas);
>>>>> -	return ret;
>>>>> +	return 0;
>>>>>   }
>>>>>   
>>>>>   /*
>>>>> -- 
>>>>> 2.39.2
>>>>>