[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

[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