[3/3] SUNRPC: Destroy the back channel when we destroy the host transport
diff mbox series

Message ID 20191016141546.32277-3-trond.myklebust@hammerspace.com
State New
Headers show
Series
  • [1/3] SUNRPC: The TCP back channel mustn't disappear while requests are outstanding
Related show

Commit Message

Trond Myklebust Oct. 16, 2019, 2:15 p.m. UTC
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(+)

Comments

NeilBrown Oct. 16, 2019, 10:08 p.m. UTC | #1
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
Trond Myklebust Oct. 17, 2019, 1:06 p.m. UTC | #2
On Thu, 2019-10-17 at 09:08 +1100, NeilBrown wrote:
> 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.

Fixed in v2.

> 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.

I considered it, but since RDMA doesn't use those variable, it wouldn't
really be a sufficient check.

Thanks
  Trond

Patch
diff mbox series

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);
+
 	/*
 	 * Tear down transport state and free the rpc_xprt
 	 */