Re: [PATCH] RCU safety for vma maple tree walks

[Liam R. Howlett]

On 26/08/31 07:35AM, Andi Kleen wrote:
> I ran into the following scenario in a slightly modified kernel:

...

> 
> That said I wasn't actually able to trigger it in a unmodified kernel
> so far with stress testing.
> 
> The following old unsolved syzkaller report has a similar signature,
> so maybe it was already seen:
> https://syzkaller.appspot.com/bug?id=4c5268fbb1d6d508a4c34dc425e2693d1ff9911a

Did you look at that syzbot?  He has 1 reply on-list pointing to the
discussion and remedy of the issue - which was debug validation code
running outside the rcu lock.

> 
> I guess in many cases where it happens for real you don't notice it
> if you don't have KASAN active.
> 
> The patch fixes up all callers to maintain the RCU reader lock
> regions correctly during the VMA walk. If they cannot be maintained the
> iterator is refreshed by a new VMA address lookup in a new region, unless
> it is proven safe not to.
> 
> In the cases where there is no sleeping it is strictly not needed
> because this scenario could not happen due the existing VMA locking.
> 
> But I fixed them too to not violate the maple tree iterator
> "rcu read lock or write lock" contract.
> 

Did you check this with lockdep?

Any access to the maple tree without holding the write lock or the rcu
read lock will cause lockdep to complain.

Likewise, any sleeping while holding the rcu read lock would produce
warnings.

Note that you do not need to hold the rcu read lock on the vma maple
tree if you have the write lock.  This looks to be the case for most (or
all?) of the cases you have below.

I suspect you have a locking issue in your modifications.


> The ones that do not strictly need it are: count_mm_mlocked_page_nr,
> remap_file_pages, range_contains_unmapped.
> 
> The ones that may sleep and clearly need it are: apply_vma_lock_flags,
> do_mprotect_pkey, remap_move, mseal_apply, mbind_range, userfaultfd
> register/unregister,
> mwriteprotect_range (doesn't sleep in the walk itself, but uses iterator
> after sleep)
> 
> In principle it could be optimized more, e.g. for example only do the
> re-lookups when actual sleeping happened. Some of it could be done
> with a new cond resched variant. But I tried to keep it minimal
> for now.
> 
> The patch survived most of LTP, the kernel mm selftests and
> my own stress tests. I didn't do any benchmarks.
> 
> For when it was introduced it's a complex area, but I believe the patch
> that originally added the problem was
> commit b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()")
> Then
> commit 17f1ae9b40c6 ("mm/vma: change munmap to use vma_munmap_struct() for
> accounting and surrounding vmas")
> extended the pattern to more places, and then later it was copied
> elsewhere too. I'm mentioning only the first below.
> 
> No cc stable so far, needs some discussion first.

Please send patches as RFC if they need discussion.

> 
> Fixes: b2b3b886738f ("mm: don't use __vma_adjust() in __split_vma()")
> Assisted-by: omp:gpt-5.6-luna
> Signed-off-by: Andi Kleen <ak@xxxxxxxxxx>

It looks like you have asked gtp to fix the locking issue you have
discovered by editing the exit_mmap() function.  I suspect this
functions complex locking dance has fooled things up on you.

If any of these issues were a problem, lockdep would be reporting about
missing locks and people would have hit these in the last 3 years (the
last syzbot report which is resolved).  Your scenario isn't that unusual
- most cloud vendors run machines flat out with as much ram used as
possible.

Thanks,
Liam