[PATCH v3 0/4] protect access to bc_serv

[PATCH v3 0/4] protect access to bc_serv

[Olga Kornievskaia]

This patch series tries to address the problem that can arise when
the client is shutting down but gets a backchannel request. As it
stands now, In nfs4_shutdown_client() we call nfs4_destroy_callback()
which would destroy/free the bc_serv structure stored in an xprt
structure. But the xprt structure is still around for a bit until
it's destroyed. If we get backchannel request after svc_destroy()
is called, then in xprt_complete_bc_request() we are accessing freed
memory.

I propose to use bc_pa_lock lock to protect access to the bc_serv
structure and make sure we free/nullify bc_serv under the lock
and then access it under a lock in  xs_complete_bs_request() and
rpcrdma_bc_receive_call().

-- v3 Addressing Anna'a comments. (1) created function for shared
code between tcp/rdma backchannel and (2) created
xprt_svc_destroy_nullify_bc() for both on/off CONFIG_SUNRPC_BACKCHANNEL
values.

-- v2 fixing kernel test bot reported compile error in usage of
xprt_svc_destroy_nullify_bc (CONFIG_SUNRPC_BACKCHANNEL toggle related)

Olga Kornievskaia (4):
  NFSv4.1: pass transport for callback shutdown
  SUNRPC: cleanup common code in backchannel request
  SUNRPC: new helper function for stopping backchannel server
  NFSv4.1: protect destroying and nullifying bc_serv structure

 fs/nfs/callback.c                 |  7 +++++--
 fs/nfs/callback.h                 |  3 ++-
 fs/nfs/nfs4client.c               |  9 ++++++--
 include/linux/sunrpc/bc_xprt.h    |  6 ++++++
 net/sunrpc/backchannel_rqst.c     | 35 ++++++++++++++++++++++++++++---
 net/sunrpc/xprtrdma/backchannel.c |  8 ++-----
 6 files changed, 54 insertions(+), 14 deletions(-)

-- 
2.47.1

[PATCH v3 1/4] NFSv4.1: pass transport for callback shutdown

[Olga Kornievskaia]

When we are setting up the 4.1 callback server, we pass in
the appropriate rpc_xprt transport pointer with which to associate
the callback server structure. Similarly, pass in the rpc_xprt
pointer for when we are shutting down the callback. This will be
used to make sure that we free the server structure and then clear
the rpc_xprt's bc_server pointer in a safe manner.

Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 fs/nfs/callback.c   | 2 +-
 fs/nfs/callback.h   | 3 ++-
 fs/nfs/nfs4client.c | 9 +++++++--
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index c8b837006bb2..8b674ee093a6 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -258,7 +258,7 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 /*
  * Kill the callback thread if it's no longer being used.
  */
-void nfs_callback_down(int minorversion, struct net *net)
+void nfs_callback_down(int minorversion, struct net *net, struct rpc_xprt *xprt)
 {
 	struct nfs_callback_data *cb_info = &nfs_callback_info[minorversion];
 	struct svc_serv *serv;
diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
index 154a6ed1299f..8809f93d82c0 100644
--- a/fs/nfs/callback.h
+++ b/fs/nfs/callback.h
@@ -188,7 +188,8 @@ extern __be32 nfs4_callback_recall(void *argp, void *resp,
 				   struct cb_process_state *cps);
 #if IS_ENABLED(CONFIG_NFS_V4)
 extern int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt);
-extern void nfs_callback_down(int minorversion, struct net *net);
+extern void nfs_callback_down(int minorversion, struct net *net,
+			      struct rpc_xprt *xprt);
 #endif /* CONFIG_NFS_V4 */
 /*
  * nfs41: Callbacks are expected to not cause substantial latency,
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 5998d6bd8a4f..ab38efd020b0 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -280,8 +280,13 @@ struct nfs_client *nfs4_alloc_client(const struct nfs_client_initdata *cl_init)
  */
 static void nfs4_destroy_callback(struct nfs_client *clp)
 {
-	if (__test_and_clear_bit(NFS_CS_CALLBACK, &clp->cl_res_state))
-		nfs_callback_down(clp->cl_mvops->minor_version, clp->cl_net);
+	if (__test_and_clear_bit(NFS_CS_CALLBACK, &clp->cl_res_state)) {
+		struct rpc_xprt *xprt;
+
+		xprt = rcu_dereference_raw(clp->cl_rpcclient->cl_xprt);
+		nfs_callback_down(clp->cl_mvops->minor_version, clp->cl_net,
+				  xprt);
+	}
 }
 
 static void nfs4_shutdown_client(struct nfs_client *clp)
-- 
2.47.1

[PATCH v3 2/4] SUNRPC: cleanup common code in backchannel request

[Olga Kornievskaia]

Create a helper function for common code between rdma
and tcp backchannel handling of the backchannel request.
Make sure that access is protected by the bc_pa_lock
lock.

Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 include/linux/sunrpc/bc_xprt.h    |  1 +
 net/sunrpc/backchannel_rqst.c     | 19 ++++++++++++++++---
 net/sunrpc/xprtrdma/backchannel.c |  8 ++------
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
index f22bf915dcf6..178f34ad8db6 100644
--- a/include/linux/sunrpc/bc_xprt.h
+++ b/include/linux/sunrpc/bc_xprt.h
@@ -25,6 +25,7 @@ void xprt_init_bc_request(struct rpc_rqst *req, struct rpc_task *task,
 void xprt_free_bc_request(struct rpc_rqst *req);
 int xprt_setup_backchannel(struct rpc_xprt *, unsigned int min_reqs);
 void xprt_destroy_backchannel(struct rpc_xprt *, unsigned int max_reqs);
+void xprt_enqueue_bc_request(struct rpc_rqst *req);
 
 /* Socket backchannel transport methods */
 int xprt_setup_bc(struct rpc_xprt *xprt, unsigned int min_reqs);
diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
index caa94cf57123..efddea0f4b8b 100644
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -354,7 +354,6 @@ struct rpc_rqst *xprt_lookup_bc_request(struct rpc_xprt *xprt, __be32 xid)
 void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied)
 {
 	struct rpc_xprt *xprt = req->rq_xprt;
-	struct svc_serv *bc_serv = xprt->bc_serv;
 
 	spin_lock(&xprt->bc_pa_lock);
 	list_del(&req->rq_bc_pa_list);
@@ -365,7 +364,21 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied)
 	set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
 
 	dprintk("RPC:       add callback request to list\n");
+	xprt_enqueue_bc_request(req);
+}
+
+void xprt_enqueue_bc_request(struct rpc_rqst *req)
+{
+	struct rpc_xprt *xprt = req->rq_xprt;
+	struct svc_serv *bc_serv;
+
 	xprt_get(xprt);
-	lwq_enqueue(&req->rq_bc_list, &bc_serv->sv_cb_list);
-	svc_pool_wake_idle_thread(&bc_serv->sv_pools[0]);
+	spin_lock(&xprt->bc_pa_lock);
+	bc_serv = xprt->bc_serv;
+	if (bc_serv) {
+		lwq_enqueue(&req->rq_bc_list, &bc_serv->sv_cb_list);
+		svc_pool_wake_idle_thread(&bc_serv->sv_pools[0]);
+	}
+	spin_unlock(&xprt->bc_pa_lock);
 }
+EXPORT_SYMBOL_GPL(xprt_enqueue_bc_request);
diff --git a/net/sunrpc/xprtrdma/backchannel.c b/net/sunrpc/xprtrdma/backchannel.c
index 8c817e755262..2f0f9618dd05 100644
--- a/net/sunrpc/xprtrdma/backchannel.c
+++ b/net/sunrpc/xprtrdma/backchannel.c
@@ -9,6 +9,7 @@
 #include <linux/sunrpc/svc.h>
 #include <linux/sunrpc/svc_xprt.h>
 #include <linux/sunrpc/svc_rdma.h>
+#include <linux/sunrpc/bc_xprt.h>
 
 #include "xprt_rdma.h"
 #include <trace/events/rpcrdma.h>
@@ -220,7 +221,6 @@ void rpcrdma_bc_receive_call(struct rpcrdma_xprt *r_xprt,
 			     struct rpcrdma_rep *rep)
 {
 	struct rpc_xprt *xprt = &r_xprt->rx_xprt;
-	struct svc_serv *bc_serv;
 	struct rpcrdma_req *req;
 	struct rpc_rqst *rqst;
 	struct xdr_buf *buf;
@@ -261,11 +261,7 @@ void rpcrdma_bc_receive_call(struct rpcrdma_xprt *r_xprt,
 	trace_xprtrdma_cb_call(r_xprt, rqst);
 
 	/* Queue rqst for ULP's callback service */
-	bc_serv = xprt->bc_serv;
-	xprt_get(xprt);
-	lwq_enqueue(&rqst->rq_bc_list, &bc_serv->sv_cb_list);
-
-	svc_pool_wake_idle_thread(&bc_serv->sv_pools[0]);
+	xprt_enqueue_bc_request(rqst);
 
 	r_xprt->rx_stats.bcall_count++;
 	return;
-- 
2.47.1

[PATCH v3 3/4] SUNRPC: new helper function for stopping backchannel server

[Olga Kornievskaia]

Create a new backchannel function to stop the backchannel server
and clear the bc_serv in transport protected under the bc_pa_lock.

Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 include/linux/sunrpc/bc_xprt.h |  5 +++++
 net/sunrpc/backchannel_rqst.c  | 16 ++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
index 178f34ad8db6..9cac8f442fd4 100644
--- a/include/linux/sunrpc/bc_xprt.h
+++ b/include/linux/sunrpc/bc_xprt.h
@@ -32,6 +32,7 @@ int xprt_setup_bc(struct rpc_xprt *xprt, unsigned int min_reqs);
 void xprt_destroy_bc(struct rpc_xprt *xprt, unsigned int max_reqs);
 void xprt_free_bc_rqst(struct rpc_rqst *req);
 unsigned int xprt_bc_max_slots(struct rpc_xprt *xprt);
+void xprt_svc_destroy_nullify_bc(struct rpc_xprt *xprt, struct svc_serv **serv);
 
 /*
  * Determine if a shared backchannel is in use
@@ -69,5 +70,9 @@ static inline void set_bc_enabled(struct svc_serv *serv)
 static inline void xprt_free_bc_request(struct rpc_rqst *req)
 {
 }
+static void xprt_svc_destroy_nullify_bc(struct rpc_xprt *xprt, struct svc_serv **serv)
+{
+	svc_destroy(serv);
+}
 #endif /* CONFIG_SUNRPC_BACKCHANNEL */
 #endif /* _LINUX_SUNRPC_BC_XPRT_H */
diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
index efddea0f4b8b..68b1fcdea8f0 100644
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -24,6 +24,22 @@ unsigned int xprt_bc_max_slots(struct rpc_xprt *xprt)
 	return BC_MAX_SLOTS;
 }
 
+/*
+ * Helper function to nullify backchannel server pointer in transport.
+ * We need to synchronize setting the pointer to NULL (done so after
+ * the backchannel server is shutdown) with the usage of that pointer
+ * by the backchannel request processing routines
+ * xprt_complete_bc_request() and rpcrdma_bc_receive_call().
+ */
+void xprt_svc_destroy_nullify_bc(struct rpc_xprt *xprt, struct svc_serv **serv)
+{
+	spin_lock(&xprt->bc_pa_lock);
+	svc_destroy(serv);
+	xprt->bc_serv = NULL;
+	spin_unlock(&xprt->bc_pa_lock);
+}
+EXPORT_SYMBOL_GPL(xprt_svc_destroy_nullify_bc);
+
 /*
  * Helper routines that track the number of preallocation elements
  * on the transport.
-- 
2.47.1

[PATCH v3 4/4] NFSv4.1: protect destroying and nullifying bc_serv structure

[Olga Kornievskaia]

When we are shutting down the client, we free the callback
server structure and then at a later pointer we free the
transport used by the client. Yet, it's possible that after
the callback server is freed, the transport receives a
backchannel request at which point we can dereferene freed
memory. Instead, do the freeing the bc server and nullying
bc_serv under the lock.

Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 fs/nfs/callback.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 8b674ee093a6..58e865bba03f 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -270,7 +270,10 @@ void nfs_callback_down(int minorversion, struct net *net, struct rpc_xprt *xprt)
 	if (cb_info->users == 0) {
 		svc_set_num_threads(serv, NULL, 0);
 		dprintk("nfs_callback_down: service destroyed\n");
-		svc_destroy(&cb_info->serv);
+		if (!minorversion)
+			svc_destroy(&cb_info->serv);
+		else
+			xprt_svc_destroy_nullify_bc(xprt, &cb_info->serv);
 	}
 	mutex_unlock(&nfs_callback_mutex);
 }
-- 
2.47.1

Re: [PATCH v3 3/4] SUNRPC: new helper function for stopping backchannel server

[kernel test robot]

Hi Olga,

kernel test robot noticed the following build warnings:

[auto build test WARNING on trondmy-nfs/linux-next]
[also build test WARNING on linus/master v6.18-rc4 next-20251104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Olga-Kornievskaia/NFSv4-1-pass-transport-for-callback-shutdown/20251104-062226
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link:    https://lore.kernel.org/r/20251103221339.45145-4-okorniev%40redhat.com
patch subject: [PATCH v3 3/4] SUNRPC: new helper function for stopping backchannel server
config: openrisc-defconfig (https://download.01.org/0day-ci/archive/20251105/202511050011.oPM1X3Kt-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251105/202511050011.oPM1X3Kt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511050011.oPM1X3Kt-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from net/sunrpc/clnt.c:40:
>> include/linux/sunrpc/bc_xprt.h:73:13: warning: 'xprt_svc_destroy_nullify_bc' defined but not used [-Wunused-function]
      73 | static void xprt_svc_destroy_nullify_bc(struct rpc_xprt *xprt, struct svc_serv **serv)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/xprt_svc_destroy_nullify_bc +73 include/linux/sunrpc/bc_xprt.h

    69	
    70	static inline void xprt_free_bc_request(struct rpc_rqst *req)
    71	{
    72	}
  > 73	static void xprt_svc_destroy_nullify_bc(struct rpc_xprt *xprt, struct svc_serv **serv)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 3/4] SUNRPC: new helper function for stopping backchannel server

[Anna Schumaker]

Hi Olga,

On 11/3/25 5:13 PM, Olga Kornievskaia wrote:
> Create a new backchannel function to stop the backchannel server
> and clear the bc_serv in transport protected under the bc_pa_lock.
> 
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>  include/linux/sunrpc/bc_xprt.h |  5 +++++
>  net/sunrpc/backchannel_rqst.c  | 16 ++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/linux/sunrpc/bc_xprt.h b/include/linux/sunrpc/bc_xprt.h
> index 178f34ad8db6..9cac8f442fd4 100644
> --- a/include/linux/sunrpc/bc_xprt.h
> +++ b/include/linux/sunrpc/bc_xprt.h
> @@ -32,6 +32,7 @@ int xprt_setup_bc(struct rpc_xprt *xprt, unsigned int min_reqs);
>  void xprt_destroy_bc(struct rpc_xprt *xprt, unsigned int max_reqs);
>  void xprt_free_bc_rqst(struct rpc_rqst *req);
>  unsigned int xprt_bc_max_slots(struct rpc_xprt *xprt);
> +void xprt_svc_destroy_nullify_bc(struct rpc_xprt *xprt, struct svc_serv **serv);
>  
>  /*
>   * Determine if a shared backchannel is in use
> @@ -69,5 +70,9 @@ static inline void set_bc_enabled(struct svc_serv *serv)
>  static inline void xprt_free_bc_request(struct rpc_rqst *req)
>  {
>  }

Can you add an extra line of whitespace between xprt_free_bc_request() and
xprt_svc_destroy_nullify_bc()?

> +static void xprt_svc_destroy_nullify_bc(struct rpc_xprt *xprt, struct svc_serv **serv)

I think the build warning is being triggered because this function isn't being
marked as "inline". If you fix that up, it should go away.

Thanks,
Anna

> +{
> +	svc_destroy(serv);
> +}
>  #endif /* CONFIG_SUNRPC_BACKCHANNEL */
>  #endif /* _LINUX_SUNRPC_BC_XPRT_H */
> diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
> index efddea0f4b8b..68b1fcdea8f0 100644
> --- a/net/sunrpc/backchannel_rqst.c
> +++ b/net/sunrpc/backchannel_rqst.c
> @@ -24,6 +24,22 @@ unsigned int xprt_bc_max_slots(struct rpc_xprt *xprt)
>  	return BC_MAX_SLOTS;
>  }
>  
> +/*
> + * Helper function to nullify backchannel server pointer in transport.
> + * We need to synchronize setting the pointer to NULL (done so after
> + * the backchannel server is shutdown) with the usage of that pointer
> + * by the backchannel request processing routines
> + * xprt_complete_bc_request() and rpcrdma_bc_receive_call().
> + */
> +void xprt_svc_destroy_nullify_bc(struct rpc_xprt *xprt, struct svc_serv **serv)
> +{
> +	spin_lock(&xprt->bc_pa_lock);
> +	svc_destroy(serv);
> +	xprt->bc_serv = NULL;
> +	spin_unlock(&xprt->bc_pa_lock);
> +}
> +EXPORT_SYMBOL_GPL(xprt_svc_destroy_nullify_bc);
> +
>  /*
>   * Helper routines that track the number of preallocation elements
>   * on the transport.

Re: [PATCH v3 4/4] NFSv4.1: protect destroying and nullifying bc_serv structure

[Anna Schumaker]

Hi Olga,

On 11/3/25 5:13 PM, Olga Kornievskaia wrote:
> When we are shutting down the client, we free the callback
> server structure and then at a later pointer we free the
> transport used by the client. Yet, it's possible that after
> the callback server is freed, the transport receives a
> backchannel request at which point we can dereferene freed
> memory. Instead, do the freeing the bc server and nullying
> bc_serv under the lock.
> 
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>  fs/nfs/callback.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 8b674ee093a6..58e865bba03f 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -270,7 +270,10 @@ void nfs_callback_down(int minorversion, struct net *net, struct rpc_xprt *xprt)
>  	if (cb_info->users == 0) {
>  		svc_set_num_threads(serv, NULL, 0);
>  		dprintk("nfs_callback_down: service destroyed\n");
> -		svc_destroy(&cb_info->serv);
> +		if (!minorversion)
> +			svc_destroy(&cb_info->serv);
> +		else
> +			xprt_svc_destroy_nullify_bc(xprt, &cb_info->serv);

Any reason we can't ditch the if/else and unconditionally call
xprt_svc_destroy_nullify_bc() here?

Thanks,
Anna

>  	}
>  	mutex_unlock(&nfs_callback_mutex);
>  }