Re: [PATCH 3/4] SUNRPC: Create a helper function for accessing the rpc_clnt's xprt_switch

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2023-12-01 at 16:15 -0500, Anna Schumaker wrote:
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> This function takes the necessary rcu read lock to dereference the
> client's rpc_xprt_switch and bump the reference count so it doesn't
> disappear underneath us before returning. This does mean that callers
> are responsible for calling xprt_switch_put() on the returned object
> when they are done with it.
> 
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
>  net/sunrpc/clnt.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 8df944444e9b..0b2c4b5484f5 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -797,15 +797,24 @@ int rpc_switch_client_transport(struct rpc_clnt *clnt,
>  }
>  EXPORT_SYMBOL_GPL(rpc_switch_client_transport);
>  
> +static struct rpc_xprt_switch *rpc_clnt_xprt_switch_get(struct rpc_clnt *clnt)
> +{
> +	struct rpc_xprt_switch *xps;
> +
> +	rcu_read_lock();
> +	xps = xprt_switch_get(rcu_dereference(clnt->cl_xpi.xpi_xpswitch));
> +	rcu_read_unlock();
> +
> +	return xps;
> +}
> +
>  static
>  int _rpc_clnt_xprt_iter_init(struct rpc_clnt *clnt, struct rpc_xprt_iter *xpi,
>  			     void func(struct rpc_xprt_iter *xpi, struct rpc_xprt_switch *xps))
>  {
>  	struct rpc_xprt_switch *xps;
>  
> -	rcu_read_lock();
> -	xps = xprt_switch_get(rcu_dereference(clnt->cl_xpi.xpi_xpswitch));
> -	rcu_read_unlock();
> +	xps = rpc_clnt_xprt_switch_get(clnt);
>  	if (xps == NULL)
>  		return -EAGAIN;
>  	func(xpi, xps);
> @@ -2206,9 +2215,7 @@ call_connect_status(struct rpc_task *task)
>  			struct rpc_xprt *saved = task->tk_xprt;
>  			struct rpc_xprt_switch *xps;
>  
> -			rcu_read_lock();
> -			xps = xprt_switch_get(rcu_dereference(clnt->cl_xpi.xpi_xpswitch));
> -			rcu_read_unlock();
> +			xps = rpc_clnt_xprt_switch_get(clnt);
>  			if (xps->xps_nxprts > 1) {
>  				long value;
>  
> @@ -3251,22 +3258,23 @@ void rpc_clnt_xprt_set_online(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
>  {
>  	struct rpc_xprt_switch *xps;
>  
> -	rcu_read_lock();
> -	xps = rcu_dereference(clnt->cl_xpi.xpi_xpswitch);
> -	rcu_read_unlock();
> +	xps = rpc_clnt_xprt_switch_get(clnt);
>  	xprt_set_online_locked(xprt, xps);
> +	xprt_switch_put(xps);
>  }

FWIW, it looks like the above fixes a real bug. It's almost certainly
not safe to dereference xps once you drop the rcu_read_lock there.

>  
>  void rpc_clnt_xprt_switch_add_xprt(struct rpc_clnt *clnt, struct rpc_xprt *xprt)
>  {
> +	struct rpc_xprt_switch *xps;
> +
>  	if (rpc_clnt_xprt_switch_has_addr(clnt,
>  		(const struct sockaddr *)&xprt->addr)) {
>  		return rpc_clnt_xprt_set_online(clnt, xprt);
>  	}
> -	rcu_read_lock();
> -	rpc_xprt_switch_add_xprt(rcu_dereference(clnt->cl_xpi.xpi_xpswitch),
> -				 xprt);
> -	rcu_read_unlock();
> +
> +	xps = rpc_clnt_xprt_switch_get(clnt);
> +	rpc_xprt_switch_add_xprt(xps, xprt);
> +	xprt_switch_put(xps);
>  }
>  EXPORT_SYMBOL_GPL(rpc_clnt_xprt_switch_add_xprt);
>  

Reviewed-by: Jeff Layton <jlayton@kernel.org>