[PATCH v1 0/2] NFS: fix RCU and tracing pointer safety

[PATCH v1 0/2] NFS: fix RCU and tracing pointer safety

[Sean Chang]

This series addresses two Sparse static analysis warnings in the NFS 
client. 

The first patch fixes an RCU-unsafe dereference when comparing 
superblock addresses by adding the necessary RCU read lock and 
dereference wrappers.

The second patch resolves a "noderef" warning in the tracing 
infrastructure by changing a pointer field to an unsigned long, 
ensuring we aren't incorrectly marking private trace-buffer 
pointers as dereferenceable.

Sean Chang (2):
  NFS: fix RCU safety in nfs_compare_super_address
  NFS: use unsigned long for req field in nfs_page_class

 fs/nfs/nfstrace.h |  6 +++---
 fs/nfs/super.c    | 32 ++++++++++++++++++++++----------
 2 files changed, 25 insertions(+), 13 deletions(-)

-- 
2.34.1

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

[Sean Chang]

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>
---
 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;
 
 	if (!net_eq(xprt1->xprt_net, xprt2->xprt_net))
-		return 0;
+		goto out;
 
 	sap1 = (struct sockaddr *)&server1->nfs_client->cl_addr;
 	sap2 = (struct sockaddr *)&server2->nfs_client->cl_addr;
 
 	if (sap1->sa_family != sap2->sa_family)
-		return 0;
+		goto out;
 
 	switch (sap1->sa_family) {
 	case AF_INET: {
 		struct sockaddr_in *sin1 = (struct sockaddr_in *)sap1;
 		struct sockaddr_in *sin2 = (struct sockaddr_in *)sap2;
 		if (sin1->sin_addr.s_addr != sin2->sin_addr.s_addr)
-			return 0;
+			goto out;
 		if (sin1->sin_port != sin2->sin_port)
-			return 0;
+			goto out;
 		break;
 	}
 	case AF_INET6: {
 		struct sockaddr_in6 *sin1 = (struct sockaddr_in6 *)sap1;
 		struct sockaddr_in6 *sin2 = (struct sockaddr_in6 *)sap2;
 		if (!ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr))
-			return 0;
+			goto out;
 		if (sin1->sin6_port != sin2->sin6_port)
-			return 0;
+			goto out;
 		break;
 	}
 	default:
-		return 0;
+		goto out;
 	}
 
-	return 1;
+	ret = 1;
+
+out:
+	rcu_read_unlock();
+	return ret;
 }
 
 static int nfs_compare_userns(const struct nfs_server *old,
-- 
2.34.1

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

[Benjamin Coddington]

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.

>  	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..

Ben

>
>  	sap1 = (struct sockaddr *)&server1->nfs_client->cl_addr;
>  	sap2 = (struct sockaddr *)&server2->nfs_client->cl_addr;
>
>  	if (sap1->sa_family != sap2->sa_family)
> -		return 0;
> +		goto out;
>
>  	switch (sap1->sa_family) {
>  	case AF_INET: {
>  		struct sockaddr_in *sin1 = (struct sockaddr_in *)sap1;
>  		struct sockaddr_in *sin2 = (struct sockaddr_in *)sap2;
>  		if (sin1->sin_addr.s_addr != sin2->sin_addr.s_addr)
> -			return 0;
> +			goto out;
>  		if (sin1->sin_port != sin2->sin_port)
> -			return 0;
> +			goto out;
>  		break;
>  	}
>  	case AF_INET6: {
>  		struct sockaddr_in6 *sin1 = (struct sockaddr_in6 *)sap1;
>  		struct sockaddr_in6 *sin2 = (struct sockaddr_in6 *)sap2;
>  		if (!ipv6_addr_equal(&sin1->sin6_addr, &sin2->sin6_addr))
> -			return 0;
> +			goto out;
>  		if (sin1->sin6_port != sin2->sin6_port)
> -			return 0;
> +			goto out;
>  		break;
>  	}
>  	default:
> -		return 0;
> +		goto out;
>  	}
>
> -	return 1;
> +	ret = 1;
> +
> +out:
> +	rcu_read_unlock();
> +	return ret;
>  }
>
>  static int nfs_compare_userns(const struct nfs_server *old,
> -- 
> 2.34.1

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@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

[PATCH v1 2/2] NFS: use unsigned long for req field in nfs_page_class

[Sean Chang]

The nfs_page_class tracepoint used a pointer for the req field. This
caused Sparse to complain about dereferencing a pointer marked as
__private within the trace ring buffer context.

Change the field type to unsigned long to store the address of the
request without dereferencing it. Update TP_printk to use 0x%lx for
consistent hexadecimal output, allowing for unique identification of
requests across the trace log.

Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 fs/nfs/nfstrace.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
index 9f9ce4a565ea..4150bbd99cfa 100644
--- a/fs/nfs/nfstrace.h
+++ b/fs/nfs/nfstrace.h
@@ -1496,7 +1496,7 @@ DECLARE_EVENT_CLASS(nfs_page_class,
 			__field(dev_t, dev)
 			__field(u32, fhandle)
 			__field(u64, fileid)
-			__field(const struct nfs_page *__private, req)
+			__field(unsigned long, req)
 			__field(loff_t, offset)
 			__field(unsigned int, count)
 			__field(unsigned long, flags)
@@ -1509,14 +1509,14 @@ DECLARE_EVENT_CLASS(nfs_page_class,
 			__entry->dev = inode->i_sb->s_dev;
 			__entry->fileid = nfsi->fileid;
 			__entry->fhandle = nfs_fhandle_hash(&nfsi->fh);
-			__entry->req = req;
+			__entry->req = (unsigned long)req;
 			__entry->offset = req_offset(req);
 			__entry->count = req->wb_bytes;
 			__entry->flags = req->wb_flags;
 		),
 
 		TP_printk(
-			"fileid=%02x:%02x:%llu fhandle=0x%08x req=%p offset=%lld count=%u flags=%s",
+			"fileid=%02x:%02x:%llu fhandle=0x%08x req=0x%lx offset=%lld count=%u flags=%s",
 			MAJOR(__entry->dev), MINOR(__entry->dev),
 			(unsigned long long)__entry->fileid, __entry->fhandle,
 			__entry->req, __entry->offset, __entry->count,
-- 
2.34.1

Re: [PATCH v1 2/2] NFS: use unsigned long for req field in nfs_page_class

[Benjamin Coddington]

Hi Sean,

On 8 Apr 2026, at 12:14, Sean Chang wrote:

> The nfs_page_class tracepoint used a pointer for the req field. This
> caused Sparse to complain about dereferencing a pointer marked as
> __private within the trace ring buffer context.
>
> Change the field type to unsigned long to store the address of the
> request without dereferencing it. Update TP_printk to use 0x%lx for
> consistent hexadecimal output, allowing for unique identification of
> requests across the trace log.

Probably we don't want to bypass the %p formatting because some
configurations use it to obfuscate kernel pointers.

I think in this context the __private annotation is incorrect.  We deref the
nfs_page pointer only in TP_fast_assign() which runs at the call site, and
the TP_printk only outputs the pointer value.

cc: Jeff

Ben

>
> Signed-off-by: Sean Chang <seanwascoding@gmail.com>
> ---
>  fs/nfs/nfstrace.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
> index 9f9ce4a565ea..4150bbd99cfa 100644
> --- a/fs/nfs/nfstrace.h
> +++ b/fs/nfs/nfstrace.h
> @@ -1496,7 +1496,7 @@ DECLARE_EVENT_CLASS(nfs_page_class,
>  			__field(dev_t, dev)
>  			__field(u32, fhandle)
>  			__field(u64, fileid)
> -			__field(const struct nfs_page *__private, req)
> +			__field(unsigned long, req)
>  			__field(loff_t, offset)
>  			__field(unsigned int, count)
>  			__field(unsigned long, flags)
> @@ -1509,14 +1509,14 @@ DECLARE_EVENT_CLASS(nfs_page_class,
>  			__entry->dev = inode->i_sb->s_dev;
>  			__entry->fileid = nfsi->fileid;
>  			__entry->fhandle = nfs_fhandle_hash(&nfsi->fh);
> -			__entry->req = req;
> +			__entry->req = (unsigned long)req;
>  			__entry->offset = req_offset(req);
>  			__entry->count = req->wb_bytes;
>  			__entry->flags = req->wb_flags;
>  		),
>
>  		TP_printk(
> -			"fileid=%02x:%02x:%llu fhandle=0x%08x req=%p offset=%lld count=%u flags=%s",
> +			"fileid=%02x:%02x:%llu fhandle=0x%08x req=0x%lx offset=%lld count=%u flags=%s",
>  			MAJOR(__entry->dev), MINOR(__entry->dev),
>  			(unsigned long long)__entry->fileid, __entry->fhandle,
>  			__entry->req, __entry->offset, __entry->count,
> -- 
> 2.34.1

Re: [PATCH v1 2/2] NFS: use unsigned long for req field in nfs_page_class

[Sean Chang]

On Fri, Apr 10, 2026 at 11:23 PM Benjamin Coddington
<ben.coddington@hammerspace.com> wrote:
>
> Hi Sean,
>
> On 8 Apr 2026, at 12:14, Sean Chang wrote:
>
> > The nfs_page_class tracepoint used a pointer for the req field. This
> > caused Sparse to complain about dereferencing a pointer marked as
> > __private within the trace ring buffer context.
> >
> > Change the field type to unsigned long to store the address of the
> > request without dereferencing it. Update TP_printk to use 0x%lx for
> > consistent hexadecimal output, allowing for unique identification of
> > requests across the trace log.
>
> Probably we don't want to bypass the %p formatting because some
> configurations use it to obfuscate kernel pointers.
>
> I think in this context the __private annotation is incorrect.  We deref the
> nfs_page pointer only in TP_fast_assign() which runs at the call site, and
> the TP_printk only outputs the pointer value.
>

Hi Ben,

Thanks for pointing this out.

I realize that using unsigned long results in printing the raw pointer
address, which bypasses the intended %p formatting.

You are right that the __private annotation is not appropriate here,
since the req pointer is only dereferenced in TP_fast_assign() at the
call site and only printed from the trace buffer context.

I will rework the patch to remove __private and keep the pointer type
with %p formatting.

Best Regards,
Sean