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

[Liam R. Howlett]

On 26/08/31 09:52AM, Andi Kleen wrote:
> > 
> > Did you check this with lockdep?
> 
> Yes all tests were running with lockdep, and it didn't trigger.
> 
> > 
> > Any access to the maple tree without holding the write lock or the rcu
> > read lock will cause lockdep to complain.
> 
> Hmm, seems that's not working for some reason?
> 
> The VM callers of for_each_vma clearly don't take rcu read locks.

I can't say why it's not triggering for you, but I can tell you that
lockdep detects incorrect locking for me and others using the maple
tree.

You even pointed to a syzbot example of a report of an rcu locking issue
- which is resolved.  No unresolved issues by syzbot (which has lockdep
 working..) has been reported in any of the functions you have patched.

It is okay to not hold the rcu lock if you hold the write lock.

> 
> > 
> > Likewise, any sleeping while holding the rcu read lock would produce
> > warnings.
> 
> Right but the walkers don't use the read lock.

Every walker either holds the read or the write lock, please recheck the
code.

> > 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.
> 
> Yes the write lock is hold (like I discussed).  The problem isn't parallel
> modification but that splitting during the walk invalidates the
> iterator caching if you sleep too. That's all purely on the local
> thread. The only contribution from others is to cause more sleeping
> due to memory pressure or rescheduling.

I'm going to state some things that will hopefully help us understand
our disconnect.

Parallel modifications are not possible.

Splits are writes.

Writes update the maple state.

Sleeping with the write lock means nothing writes.

Writers replace the nodes in the tree before marking old nodes as dead.

Writers mark the nodes dead before they are added to the rcu free list.

Writers return pointing to the new node, not the dead node.

Readers cannot sleep.

Readers that see a dead node restart the walk from the top of the tree
into a new node that is not dead.

Nodes cannot be freed until all readers which could have had a reference
have dropped the rcu read lock.

I am very confident you are doing something wrong in the code that is
not upstream.  If you have a reproducer with the upstream kernel, then I
can help you.

Thanks,
Liam