From: Sean Chang <seanwascoding@gmail.com>
To: Benjamin Coddington <ben.coddington@hammerspace.com>
Cc: trondmy@kernel.org, anna@kernel.org, linux-nfs@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/2] NFS: fix RCU safety in nfs_compare_super_address
Date: Wed, 15 Apr 2026 00:12:07 +0800 [thread overview]
Message-ID: <CAAb=EJUfoq0dq=mSciyoBjTGWM05QKAHgRUZebq0pDh82pQ0gw@mail.gmail.com> (raw)
In-Reply-To: <C9851FD8-FFFA-47DD-BAA7-F465F293295B@hammerspace.com>
On Fri, Apr 10, 2026 at 11:09 PM Benjamin Coddington
<ben.coddington@hammerspace.com> 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@gmail.com>
>
> 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
next prev parent reply other threads:[~2026-04-14 16:12 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-08 16:14 [PATCH v1 0/2] NFS: fix RCU and tracing pointer safety Sean Chang
2026-04-08 16:14 ` [PATCH v1 1/2] NFS: fix RCU safety in nfs_compare_super_address Sean Chang
2026-04-10 15:09 ` Benjamin Coddington
2026-04-14 16:12 ` Sean Chang [this message]
2026-04-08 16:14 ` [PATCH v1 2/2] NFS: use unsigned long for req field in nfs_page_class Sean Chang
2026-04-10 15:23 ` Benjamin Coddington
2026-04-14 9:14 ` Sean Chang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAb=EJUfoq0dq=mSciyoBjTGWM05QKAHgRUZebq0pDh82pQ0gw@mail.gmail.com' \
--to=seanwascoding@gmail.com \
--cc=anna@kernel.org \
--cc=ben.coddington@hammerspace.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=trondmy@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).