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

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

[Sean Chang]

This series addresses two Sparse static analysis warnings in the NFS
client related to RCU safety and pointer attributes.

The first patch resolves a "dereferencing noderef expression" warning 
within the nfs_page_class tracepoint by removing a redundant __private 
attribute that was causing Sparse to complain during trace-buffer 
assignments.

The second patch fixes an RCU-unsafe dereference in nfs_compare_super_address.
It wraps cl_xprt access with rcu_read_lock() and rcu_dereference(). 
Following reviewer feedback, the RCU critical section is kept minimal, 
covering only the transport and network namespace checks. An additional 
check for XPRT_CONNECTED is included to ensure the transport is logically 
active during the comparison.

v2:
  - Patch 1: Instead of changing the 'req' field type to unsigned long (as in v1),
    simply remove the redundant __private attribute. This resolves the
    Sparse warning while preserving the original pointer type.
  - Patch 2: Reduced RCU read-side critical section scope to cover only
    the necessary transport/net-ns checks, as suggested by reviewers.

Sean Chang (2):
  NFS: remove redundant __private attribute from nfs_page_class
  NFS: Fix RCU dereference of cl_xprt in nfs_compare_super_address

 fs/nfs/nfstrace.h |  2 +-
 fs/nfs/super.c    | 21 ++++++++++++++++++---
 2 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] NFS: remove redundant __private attribute from nfs_page_class

[Sean Chang]

The nfs_page_class tracepoint uses a pointer for the 'req' field marked
with the __private attribute. This causes Sparse to complain about
dereferencing a private pointer within the trace ring buffer context,
specifically during the TP_fast_assign() operation.

This fixes a Sparse warning introduced in commit b6ef079fd984 ("nfs:
more in-depth tracing of writepage events") by removing the redundant
__private attribute from the 'req' field.

Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 fs/nfs/nfstrace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
index 9f9ce4a565ea..ff467959f733 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(const struct nfs_page *, req)
 			__field(loff_t, offset)
 			__field(unsigned int, count)
 			__field(unsigned long, flags)
-- 
2.43.0

[PATCH v2 2/2] NFS: Fix RCU dereference of cl_xprt 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() is unsafe and triggers
Sparse warnings.

Fix this by wrapping the access with rcu_read_lock() and using
rcu_dereference() to safely retrieve the transport pointer. This
ensures the xprt structure remains memory-safe during the comparison
of network namespaces and addresses.

Additionally, add a check for the XPRT_CONNECTED state bit. While RCU
guarantees the memory remains valid, checking XPRT_CONNECTED ensures
the transport is still logically active, preventing operations on a
transport that is already undergoing teardown.

Fixes: 7e3fcf61abde ("nfs: don't share mounts between network namespaces")
Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
 fs/nfs/super.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 7a318581f85b..c9044d9d64cc 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1166,12 +1166,23 @@ 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;
+
+	rcu_read_lock();
+
+	xprt1 = rcu_dereference(server1->client->cl_xprt);
+	xprt2 = rcu_dereference(server2->client->cl_xprt);
+
+	if (!xprt1 || !xprt2 ||
+	    !test_bit(XPRT_CONNECTED, &xprt1->state) ||
+	    !test_bit(XPRT_CONNECTED, &xprt2->state))
+		goto out_unlock;
 
 	if (!net_eq(xprt1->xprt_net, xprt2->xprt_net))
-		return 0;
+		goto out_unlock;
+
+	rcu_read_unlock();
 
 	sap1 = (struct sockaddr *)&server1->nfs_client->cl_addr;
 	sap2 = (struct sockaddr *)&server2->nfs_client->cl_addr;
@@ -1203,6 +1214,10 @@ static int nfs_compare_super_address(struct nfs_server *server1,
 	}
 
 	return 1;
+
+out_unlock:
+	rcu_read_unlock();
+	return 0;
 }
 
 static int nfs_compare_userns(const struct nfs_server *old,
-- 
2.43.0

Re: [PATCH v2 1/2] NFS: remove redundant __private attribute from nfs_page_class

[Benjamin Coddington]

On 19 Apr 2026, at 3:01, Sean Chang wrote:

> The nfs_page_class tracepoint uses a pointer for the 'req' field marked
> with the __private attribute. This causes Sparse to complain about
> dereferencing a private pointer within the trace ring buffer context,
> specifically during the TP_fast_assign() operation.
>
> This fixes a Sparse warning introduced in commit b6ef079fd984 ("nfs:
> more in-depth tracing of writepage events") by removing the redundant
> __private attribute from the 'req' field.
>
> Signed-off-by: Sean Chang <seanwascoding@gmail.com>

Reviewed-by: Benjamin Coddington <bcodding@hammerspace.com>

Ben

Re: [PATCH v2 2/2] NFS: Fix RCU dereference of cl_xprt in nfs_compare_super_address

[Benjamin Coddington]

On 19 Apr 2026, at 3:01, Sean Chang wrote:

> The cl_xprt pointer in struct rpc_clnt is marked as __rcu. Accessing
> it directly in nfs_compare_super_address() is unsafe and triggers
> Sparse warnings.
>
> Fix this by wrapping the access with rcu_read_lock() and using
> rcu_dereference() to safely retrieve the transport pointer. This
> ensures the xprt structure remains memory-safe during the comparison
> of network namespaces and addresses.
>
> Additionally, add a check for the XPRT_CONNECTED state bit. While RCU
> guarantees the memory remains valid, checking XPRT_CONNECTED ensures
> the transport is still logically active, preventing operations on a
> transport that is already undergoing teardown.
>
> Fixes: 7e3fcf61abde ("nfs: don't share mounts between network namespaces")
> Signed-off-by: Sean Chang <seanwascoding@gmail.com>
> ---
>  fs/nfs/super.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 7a318581f85b..c9044d9d64cc 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1166,12 +1166,23 @@ 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;
> +
> +	rcu_read_lock();
> +
> +	xprt1 = rcu_dereference(server1->client->cl_xprt);
> +	xprt2 = rcu_dereference(server2->client->cl_xprt);

This is a good fix for the sparse warning, but..

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

^^ I really don't think this check is necessary.  Aren't we only ever
comparing with one freshly created, and the other looked up holding sb_lock?

I'm doubtful this hunk is fixing a real problem.

Ben

Re: [PATCH v2 2/2] NFS: Fix RCU dereference of cl_xprt in nfs_compare_super_address

[Sean Chang]

On Sun, Apr 19, 2026 at 9:52 PM Benjamin Coddington
<ben.coddington@hammerspace.com> wrote:
>
>
> > +
> > +     if (!xprt1 || !xprt2 ||
> > +         !test_bit(XPRT_CONNECTED, &xprt1->state) ||
> > +         !test_bit(XPRT_CONNECTED, &xprt2->state))
> > +             goto out_unlock;
>
> ^^ I really don't think this check is necessary.  Aren't we only ever
> comparing with one freshly created, and the other looked up holding sb_lock?
>
> I'm doubtful this hunk is fixing a real problem.
>

Hi Ben,

Thanks for the clarification.

You're right. I've traced the call path and confirmed that
nfs_compare_super() is called by sget_fc() while holding
the global sb_lock. This ensures the existence and stability
of the existing superblocks and their associated transports
during the comparison.

Since the connection state doesn't affect the identity of the
server, I'll remove the redundant test_bit and pointer checks and send out v3.

Thanks,
Sean