Re: [PATCH 1/1] libceph: use RCU to protect monmap in ceph_compare_options()

On Sun, 2026-07-12 at 14:28 +0800, Yong Wang wrote:
> 
> 
> 在 2026/7/11 0:33, Viacheslav Dubeyko 写道:
> > On Fri, 2026-07-10 at 10:16 +0800, Yong Wang wrote:
> > > 
> > > 在 2026/7/9 6:43, Viacheslav Dubeyko 写道:
> > > > On Wed, 2026-07-08 at 11:03 +0800, Ren Wei wrote:
> > > > > From: Yong Wang <[email protected]>
> > > > > 
> > > > > ceph_compare_options() checks whether a new mount shares any
> > > > > monitor
> > > > > address with an existing client by walking client-
> > > > > >monc.monmap via
> > > > > ceph_monmap_contains().  That comparison can run under
> > > > > sb_lock or
> > > > > rbd_client_list_lock, so it cannot take monc->mutex.
> > > > > 
> > > > > Meanwhile, monmap update handling replaces monc->monmap under
> > > > > monc->mutex and frees the old map immediately.  A concurrent
> > > > > shared-
> > > > > mount comparison can therefore dereference a freed monmap and
> > > > > walk
> > > > > stale mon_inst[] entries, triggering a use-after-free.
> > > > > 
> > > > > Protect the compare path with RCU and publish/free monitor
> > > > > maps with
> > > > > rcu_assign_pointer() and kfree_rcu().  Annotate monc->monmap
> > > > > as an
> > > > > RCU pointer and use rcu_dereference_protected() in mutex-
> > > > > protected
> > > > > paths to keep the accesses consistent with the new pointer
> > > > > contract.
> > > > > 
> > > > > This keeps the existing non-blocking comparison semantics
> > > > > while
> > > > > ensuring that replaced monmaps remain alive until readers are
> > > > > done.
> > > > 
> > > > The approach makes sense to me. However, I have some concern.
> > > > If the replaced
> > > > monmap(s) could be in use with newly allocated one(s), then are
> > > > we safe here?
> > > 
> > > The new monmap is fully allocated and initialized before
> > > publication, then
> > > installed with rcu_assign_pointer() under monc->mutex. Readers
> > > only dereference
> > > an RCU snapshot and treat the monmap as immutable, while the
> > > replaced monmap is
> > > freed with kfree_rcu(), which means the old map is not reclaimed
> > > immediately, 
> > > but is delayed until all readers that are still accessing it have
> > > exited the 
> > > read-side critical section.Readers only see either the old map or
> > > the new map.
> > > 
> > > So it is safe to use.
> > > 
> > > > Have you tried to run xfstests for the patch?
> > > 
> > > I ran the ceph xfstests in QEMU on the patched kernel, and all
> > > ceph-
> > > specific tests in the current tree passed: ceph/001-006.
> > > 
> > > 
> > 
> > The running only Ceph specific test-cases is not enough. The patch
> > should
> > survive the auto group of xfstests.
> > 
> I reran the tests on a baseline kernel before the RCU patch.
> 
> Excluding the notrun cases, the following tests failed both before
> and after the patch:
> 
> - generic/363: fsx reported READ BAD DATA
> - generic/429: encrypted dentry/key revalidation behavior did not
> match expectations
> - generic/440: encrypted name/key cache consistency behavior did not
> match expectations
> - generic/580: encrypted files became inaccessible after key
> removal/re-addition
> - generic/593: filesystem-level/provisioning-key encryption flow
> failed
> - generic/595: encrypted file remained inaccessible after key
> eviction/re-addition
> - generic/631: overlayfs-on-Ceph scratch mount path failed
> 
> generic/650 and generic/777 could not be completed reliably in my
> local environment because 
> they caused the terminal to become unresponsive, so I do not have a
> meaningful 
> before/after comparison for them.
> 
> So from this comparison, I do not have evidence that the above
> failures were introduced 
> by the RCU patch.
> 
> Kernel(net) commit tested before the patch:
>   6d27e29a90bc6a717b97c6ddcd866db7bd8e4adc
> 
> xfstests commit:
>   ffc8bad17e5b2f56e48dbac43f7c5ae8ac368fe5
> 
> 

It's interesting that you can reproduce more failures that I can
reproduce in my environment. :) Usually, I can see around 3 failed
xfstests. I've already shared fixes for several ones. But my fixes
haven't been accepted. We still need to fix some xfstests failures. :)

The patch makes sense to me and it looks good.

Reviewed-by: Viacheslav Dubeyko <[email protected]>

Thanks,
Slava.