Re: [PATCH 3/3] SUNRPC: Destroy the back channel when we destroy the host transport

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

On Wed, Oct 16 2019, Trond Myklebust wrote:

> When we're destroying the host transport mechanism, we should ensure
> that we do not leak memory by failing to release any back channel
> slots that might still exist.
>
> Reported-by: Neil Brown <neilb@suse.de>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  net/sunrpc/xprt.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index 8a45b3ccc313..41df4c507193 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1942,6 +1942,11 @@ static void xprt_destroy_cb(struct work_struct *work)
>  	rpc_destroy_wait_queue(&xprt->sending);
>  	rpc_destroy_wait_queue(&xprt->backlog);
>  	kfree(xprt->servername);
> +	/*
> +	 * Destroy any existing back channel
> +	 */
> +	xprt_destroy_backchannel(xprt, UINT_MAX);
> +

This will cause xprt->bc_alloc_max to become meaningless.
That isn't really a problem as the xprt is about to be freed, but it
still seems a little untidy - fragile maybe.
How about another line in the comment:

   * Note: this corrupts ->bc_alloc_max, but it is too late for that to
   * matter.
??

Also, possibly add
 WARN_ON(atomic_read(&xprt->bc_slot_count);
either before or after the xprt_destroy_backchannel - because there
really shouldn't be any requests by this stage.

Thanks,
NeilBrown


>  	/*
>  	 * Tear down transport state and free the rpc_xprt
>  	 */
> -- 
> 2.21.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]