Re: [PATCH AUTOSEL 5.4 26/35] SUNRPC: defer slow parts of rpc_free_client() to a workqueue.

[NeilBrown]

On Thu, May 07 2020, Sasha Levin wrote:

> From: NeilBrown <neilb@xxxxxxx>
>
> [ Upstream commit 7c4310ff56422ea43418305d22bbc5fe19150ec4 ]

This one is buggy - it introduces a use-after-free.  Best delay it for
now.

NeilBrown

>
> The rpciod workqueue is on the write-out path for freeing dirty memory,
> so it is important that it never block waiting for memory to be
> allocated - this can lead to a deadlock.
>
> rpc_execute() - which is often called by an rpciod work item - calls
> rcp_task_release_client() which can lead to rpc_free_client().
>
> rpc_free_client() makes two calls which could potentially block wating
> for memory allocation.
>
> rpc_clnt_debugfs_unregister() calls into debugfs and will block while
> any of the debugfs files are being accessed.  In particular it can block
> while any of the 'open' methods are being called and all of these use
> malloc for one thing or another.  So this can deadlock if the memory
> allocation waits for NFS to complete some writes via rpciod.
>
> rpc_clnt_remove_pipedir() can take the inode_lock() and while it isn't
> obvious that memory allocations can happen while the lock it held, it is
> safer to assume they might and to not let rpciod call
> rpc_clnt_remove_pipedir().
>
> So this patch moves these two calls (together with the final kfree() and
> rpciod_down()) into a work-item to be run from the system work-queue.
> rpciod can continue its important work, and the final stages of the free
> can happen whenever they happen.
>
> I have seen this deadlock on a 4.12 based kernel where debugfs used
> synchronize_srcu() when removing objects.  synchronize_srcu() requires a
> workqueue and there were no free workther threads and none could be
> allocated.  While debugsfs no longer uses SRCU, I believe the deadlock
> is still possible.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
>  include/linux/sunrpc/clnt.h |  8 +++++++-
>  net/sunrpc/clnt.c           | 21 +++++++++++++++++----
>  2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
> index abc63bd1be2b5..d99d39d45a494 100644
> --- a/include/linux/sunrpc/clnt.h
> +++ b/include/linux/sunrpc/clnt.h
> @@ -71,7 +71,13 @@ struct rpc_clnt {
>  #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>  	struct dentry		*cl_debugfs;	/* debugfs directory */
>  #endif
> -	struct rpc_xprt_iter	cl_xpi;
> +	/* cl_work is only needed after cl_xpi is no longer used,
> +	 * and that are of similar size
> +	 */
> +	union {
> +		struct rpc_xprt_iter	cl_xpi;
> +		struct work_struct	cl_work;
> +	};
>  	const struct cred	*cl_cred;
>  };
>  
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index f7f78566be463..a7430b66c7389 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -877,6 +877,20 @@ EXPORT_SYMBOL_GPL(rpc_shutdown_client);
>  /*
>   * Free an RPC client
>   */
> +static void rpc_free_client_work(struct work_struct *work)
> +{
> +	struct rpc_clnt *clnt = container_of(work, struct rpc_clnt, cl_work);
> +
> +	/* These might block on processes that might allocate memory,
> +	 * so they cannot be called in rpciod, so they are handled separately
> +	 * here.
> +	 */
> +	rpc_clnt_debugfs_unregister(clnt);
> +	rpc_clnt_remove_pipedir(clnt);
> +
> +	kfree(clnt);
> +	rpciod_down();
> +}
>  static struct rpc_clnt *
>  rpc_free_client(struct rpc_clnt *clnt)
>  {
> @@ -887,17 +901,16 @@ rpc_free_client(struct rpc_clnt *clnt)
>  			rcu_dereference(clnt->cl_xprt)->servername);
>  	if (clnt->cl_parent != clnt)
>  		parent = clnt->cl_parent;
> -	rpc_clnt_debugfs_unregister(clnt);
> -	rpc_clnt_remove_pipedir(clnt);
>  	rpc_unregister_client(clnt);
>  	rpc_free_iostats(clnt->cl_metrics);
>  	clnt->cl_metrics = NULL;
>  	xprt_put(rcu_dereference_raw(clnt->cl_xprt));
>  	xprt_iter_destroy(&clnt->cl_xpi);
> -	rpciod_down();
>  	put_cred(clnt->cl_cred);
>  	rpc_free_clid(clnt);
> -	kfree(clnt);
> +
> +	INIT_WORK(&clnt->cl_work, rpc_free_client_work);
> +	schedule_work(&clnt->cl_work);
>  	return parent;
>  }
>  
> -- 
> 2.20.1

Attachment: signature.asc
Description: PGP signature