[PATCH] SUNRPC: Fix autobind on cloned rpc clients

[Andy Whitcroft]

On Tue, Mar 24, 2009 at 02:55:20PM +0100, Stefan Bader wrote:
> From: Trond Myklebust <Trond.Myklebust at netapp.com>
> 
> Bug: #212485, #341783
> 
> commit 9a4bd29fe8f6d3f015fe1c8e5450eb62cfebfcc9 upstream
> 
> Despite the fact that cloned rpc clients won't have the cl_autobind flag
> set, they may still find themselves calling rpcb_getport_async(). For this
> to happen, it suffices for a _parent_ rpc_clnt to use autobinding, in which
> case any clone may find itself triggering the !xprt_bound() case in
> call_bind().
> 
> The correct fix for this is to walk back up the tree of cloned rpc clients,
> in order to find the parent that 'owns' the transport, either because it
> has clnt->cl_autobind set, or because it originally created the
> transport...
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust at netapp.com>
> Signed-off-by: Stefan Bader <stefan.bader at canonical.com>
> ---
>  net/sunrpc/rpcb_clnt.c |   36 +++++++++++++++++++++++++++++-------
>  1 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index f42433d..201a4ae 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -295,6 +295,28 @@ int rpcb_getport_sync(struct sockaddr_in *sin, __u32 prog,
>  }
>  EXPORT_SYMBOL_GPL(rpcb_getport_sync);
>  
> +/*
> + * In the case where rpc clients have been cloned, we want to make
> + * sure that we use the program number/version etc of the actual
> + * owner of the xprt. To do so, we walk back up the tree of parents
> + * to find whoever created the transport and/or whoever has the
> + * autobind flag set.
> + */
> +static struct rpc_clnt *rpcb_find_transport_owner(struct rpc_clnt *clnt)
> +{
> +	struct rpc_clnt *parent = clnt->cl_parent;
> +
> +	while (parent != clnt) {
> +		if (parent->cl_xprt != clnt->cl_xprt)
> +			break;
> +		if (clnt->cl_autobind)
> +			break;
> +		clnt = parent;
> +		parent = parent->cl_parent;
> +	}
> +	return clnt;
> +}
> +
>  /**
>   * rpcb_getport_async - obtain the port for a given RPC service on a given host
>   * @task: task that is waiting for portmapper request
> @@ -304,9 +326,9 @@ EXPORT_SYMBOL_GPL(rpcb_getport_sync);
>   */
>  void rpcb_getport_async(struct rpc_task *task)
>  {
> -	struct rpc_clnt *clnt = task->tk_client;
> +	struct rpc_clnt *clnt;
>  	int bind_version;
> -	struct rpc_xprt *xprt = task->tk_xprt;
> +	struct rpc_xprt *xprt;
>  	struct rpc_clnt	*rpcb_clnt;
>  	static struct rpcbind_args *map;
>  	struct rpc_task	*child;
> @@ -314,13 +336,13 @@ void rpcb_getport_async(struct rpc_task *task)
>  	int status;
>  	struct rpcb_info *info;
>  
> +	clnt = rpcb_find_transport_owner(task->tk_client);
> +	xprt = clnt->cl_xprt;
> +
>  	dprintk("RPC: %5u %s(%s, %u, %u, %d)\n",
>  		task->tk_pid, __FUNCTION__,
>  		clnt->cl_server, clnt->cl_prog, clnt->cl_vers, xprt->prot);
>  
> -	/* Autobind on cloned rpc clients is discouraged */
> -	BUG_ON(clnt->cl_parent != clnt);
> -
>  	if (xprt_test_and_set_binding(xprt)) {
>  		status = -EAGAIN;	/* tell caller to check again */
>  		dprintk("RPC: %5u %s: waiting for another binder\n",
> @@ -401,9 +423,9 @@ void rpcb_getport_async(struct rpc_task *task)
>  			task->tk_pid, __FUNCTION__);
>  		goto bailout;
>  	}
> -	rpc_put_task(child);
>  
> -	task->tk_xprt->stat.bind_count++;
> +	xprt->stat.bind_count++;
> +	rpc_put_task(child);

This change of order seems odd, but benign to my reading.  I guess as
its this way upstream its ok.

>  	return;
>  
>  bailout:

Acked-by: Andy Whitcroft <apw at canonical.com>

-apw