Re: [PATCH v1 1/2] NFS: fix RCU safety in nfs_compare_super_address

[Sean Chang]

On Fri, Apr 10, 2026 at 11:09 PM Benjamin Coddington
<ben.coddington@xxxxxxxxxxxxxxx> wrote:
>
> On 8 Apr 2026, at 12:14, Sean Chang wrote:
>
> > The cl_xprt pointer in struct rpc_clnt is marked as __rcu. Accessing
> > it directly in nfs_compare_super_address() without RCU protection is
> > unsafe and triggers Sparse warnings about dereferencing noderef
> > expressions.
> >
> > Fix this by wrapping the access with rcu_read_lock() and using
> > rcu_dereference() to safely retrieve the transport pointer. This
> > ensures the xprt remains valid during the comparison of network
> > namespaces and addresses, preventing potential use-after-free during
> > concurrent transport updates.
> >
> > Signed-off-by: Sean Chang <seanwascoding@xxxxxxxxx>
>
> Fixes: 7e3fcf61abde ("nfs: don't share mounts between network namespaces")
>
> > ---
> >  fs/nfs/super.c | 32 ++++++++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index 7a318581f85b..071337f9ea37 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -1166,43 +1166,55 @@ static int nfs_set_super(struct super_block *s, struct fs_context *fc)
> >  static int nfs_compare_super_address(struct nfs_server *server1,
> >                                    struct nfs_server *server2)
> >  {
> > +     struct rpc_xprt *xprt1, *xprt2;
> >       struct sockaddr *sap1, *sap2;
> > -     struct rpc_xprt *xprt1 = server1->client->cl_xprt;
> > -     struct rpc_xprt *xprt2 = server2->client->cl_xprt;
> > +     int ret = 0;
> > +
> > +     rcu_read_lock();
> > +
> > +     xprt1 = rcu_dereference(server1->client->cl_xprt);
> > +     xprt2 = rcu_dereference(server2->client->cl_xprt);
> > +
> > +     if (!xprt1 || !xprt2)
> > +             goto out;
>
> ^^ I'm not sure that's a great test, the rpc_xprt objects are refcounted.
> These might not be null but you could still race with a freeing path?
>
> However, I think you might just be safe inside the RCU section here because
> rpc_switch_client_transport() uses synchronize_rcu() before xprt_put(old).
> I didn't audit all the freeing paths.
>

Thanks for the insightful feedback and for auditing the
rpc_switch_client_transport path!

You're absolutely right that synchronize_rcu() provides
a safety net during transport switches. However, to ensure
robustness across all potential freeing paths (including
those not yet fully audited) and to address the race you
mentioned, I've updated the patch to check the xprt state:

+        if (!xprt1 || !xprt2 ||
+            !test_bit(XPRT_CONNECTED, &xprt1->state) ||
+            !test_bit(XPRT_CONNECTED, &xprt2->state))
+                goto out_unlock;

The XPRT_CONNECTED check ensures that the transport
is not only memory-safe (via RCU) but also logically active.

Since the RPC layer clears this bit before initiating the teardown
of an rpc_xprt, this prevents accessing xprt_net on an object
that is on the freeing path.

> >       if (!net_eq(xprt1->xprt_net, xprt2->xprt_net))
> > -             return 0;
> > +             goto out;
>
> Probably safe to drop the RCU protection scope after this point.  No need to
> hold it over all the other checks..
>

Agreed. The RCU protection is indeed only necessary for fetching
and comparing the network namespaces from the rpc_xprt objects.
I will move rcu_read_unlock() to immediately after the net_eq check
in v2 to keep the critical section as small as possible.
Thanks!

Best Regards,
Sean