Re: [PATCH] maple_tree: Annotate lockless pivot reads for KCSAN

[Liam R. Howlett]

On 26/08/27 05:35AM, Hui Zhu wrote:
> > 
> > On 26/08/26 09:41AM, Andrew Morton wrote:
> > 
> > > 
> > > On Wed, 26 Aug 2026 15:44:30 +0800 "Hui Zhu" <hui.zhu@xxxxxxxxx> wrote:
> > >  
> > >  In RCU mode, replaced maple nodes are marked dead and freed via RCU
> > >  after the new node has been published. Arming the RCU free writes
> > >  node->rcu.next and node->rcu.func, which share storage with
> > >  pivot[0] and pivot[1] (see struct maple_node), while lockless
> > >  readers may still walk the dead node. These stores therefore race
> > >  with the pivot loads performed by the walkers.
> > >  
> > >  This is harmless: the writer marks the node dead with an smp_wmb()
> > >  before arming the rcu_head, and the walkers re-check ma_dead_node()
> > >  after reading the node and restart the walk when the node is dead,
> > >  so any pivot read that raced with the rcu_head stores is discarded.
> > >  KCSAN cannot see this protocol and reports the plain accesses, so
> > >  annotate the lockless pivot reads with data_race() through a new
> > >  ma_pivot_rcu() helper.
> > >  
> > >  Found by fuzzing on a 6.6 kernel; the race still exists on
> > >  mainline. No functional change intended.
> > >  
> > >  
> > >  Thanks. AI review suggests that the patch is correct, but incomplete?
> > >  
> > >  https://sashiko.dev/#/patchset/20260826074430.1139325-1-hui.zhu@xxxxxxxxx
> > > 
> > Yes, this is literally every read of numerous pivots on any reader that
> > would need something like this. Most code is shared with the writer
> > side, so we'll have data_race() annotation where it is not needed there.
> > 
> > I don't like the name of the function and I don't agree that it is worth
> > doing, especially half of it. If you notice ma_ functions take a maple
> > node as the first argument, but your new function takes an array pointer
> > in the node.
> > 
> > The from address does not agree with the sign-off on the patch.
> > 
> > I also don't think a benign race needs a Fixes tag?
> > 
> > It might be worth looking at other ways to calm kcsans down such as the
> > type qualifier __data_racy, or maybe the makefile option.
> 
> Hi Liam,
> 
> __data_racy is defined as volatile for KCSAN kernels, so the qualifier
> has to propagate to every access site: ma_pivots() would return a
> volatile pointer, and all the "unsigned long *pivots" locals and helper
> parameters in maple_tree.c (about 25 sites) would need the qualifier
> too.
> It also marks the whole pivot array as racy, while only pivot[0] and
> pivot[1] actually overlap the rcu_head.

Isn't your change already using the data_racy annotation on more than
pivot 0 and 1?  For instance, mas->offset is often passed in and that
is likely not 0 or 1.

Fair point about the code churn, though.  I don't need every function
being changed to accept a volatile.

> 
> The Makefile option (KCSAN_SANITIZE_maple_tree.o := n) is a one-liner,
> but it disables KCSAN for the entire file, so any real data race
> introduced in maple_tree.c later would go unnoticed.

KCSAN has never reported anything real to me and that's why I suggested
turning it off.  After speaking with Paul McKenney on the matter I have
been convinced to not disable it.

I still don't really see a whole lot of value in annotating the code to
say a particular race is not an issue - we will have mostly disabled the
tool by annotation in the end.  And these are most likely going to be
the areas where we have issues if any arise - ie, some special arch
corner case that comes up that didn't exist or wasn't in mind during
annotation.

> 
> Do you think one of these two is still the better choice, or should I
> keep the current approach and fix the patch according to your review
> comments?
> 

I guess respin it and see how it looks with your current path.

Thanks,
Liam