diff mbox series

xprtrdma: Fix a NULL dereference in frwr_unmap_sync()

Message ID 161989777022.1772.5943251585208443632.stgit@manet.1015granger.net (mailing list archive)
State New
Headers show
Series xprtrdma: Fix a NULL dereference in frwr_unmap_sync() | expand

Commit Message

Chuck Lever III May 1, 2021, 7:38 p.m. UTC
The normal mechanism that invalidates and unmaps MRs is
frwr_unmap_async(). frwr_unmap_sync() is used only when an RPC
Reply bearing Write or Reply chunks has been lost (ie, almost
never).

Coverity found that after commit 9a301cafc861 ("xprtrdma: Move
fr_linv_done field to struct rpcrdma_mr"), the while() loop in
frwr_unmap_sync() exits only once @mr is NULL, unconditionally
causing dereferences of @mr to Oops.

I've tested this fix by creating a client that skips invoking
frwr_unmap_async() when RPC Replies complete. That forces all
invalidation tasks to fall upon frwr_unmap_sync(). Simple workloads
with this fix applied to the adulterated client work as designed.

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1504556 ("Null pointer dereferences")
Fixes: 9a301cafc861 ("xprtrdma: Move fr_linv_done field to struct rpcrdma_mr")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/xprtrdma/frwr_ops.c |    1 +
 1 file changed, 1 insertion(+)

Trond Note: you may need to update the commit ID for 9a301cafc861
as needed to properly reference the final commit ID for ("xprtrdma:
Move fr_linv_done field to struct rpcrdma_mr").

Comments

Chuck Lever III May 1, 2021, 7:44 p.m. UTC | #1
> On May 1, 2021, at 3:38 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> The normal mechanism that invalidates and unmaps MRs is
> frwr_unmap_async(). frwr_unmap_sync() is used only when an RPC
> Reply bearing Write or Reply chunks has been lost (ie, almost
> never).
> 
> Coverity found that after commit 9a301cafc861 ("xprtrdma: Move
> fr_linv_done field to struct rpcrdma_mr"), the while() loop in
> frwr_unmap_sync() exits only once @mr is NULL, unconditionally
> causing dereferences of @mr to Oops.

^causing dereferences^causing subsequent dereferences

Sigh.


> I've tested this fix by creating a client that skips invoking
> frwr_unmap_async() when RPC Replies complete. That forces all
> invalidation tasks to fall upon frwr_unmap_sync(). Simple workloads
> with this fix applied to the adulterated client work as designed.
> 
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1504556 ("Null pointer dereferences")
> Fixes: 9a301cafc861 ("xprtrdma: Move fr_linv_done field to struct rpcrdma_mr")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> net/sunrpc/xprtrdma/frwr_ops.c |    1 +
> 1 file changed, 1 insertion(+)
> 
> Trond Note: you may need to update the commit ID for 9a301cafc861
> as needed to properly reference the final commit ID for ("xprtrdma:
> Move fr_linv_done field to struct rpcrdma_mr").
> 
> diff --git a/net/sunrpc/xprtrdma/frwr_ops.c b/net/sunrpc/xprtrdma/frwr_ops.c
> index 285d73246fc2..229fcc9a9064 100644
> --- a/net/sunrpc/xprtrdma/frwr_ops.c
> +++ b/net/sunrpc/xprtrdma/frwr_ops.c
> @@ -530,6 +530,7 @@ void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
> 		*prev = last;
> 		prev = &last->next;
> 	}
> +	mr = container_of(last, struct rpcrdma_mr, mr_invwr);
> 
> 	/* Strong send queue ordering guarantees that when the
> 	 * last WR in the chain completes, all WRs in the chain
> 
> 

--
Chuck Lever
diff mbox series

Patch

diff --git a/net/sunrpc/xprtrdma/frwr_ops.c b/net/sunrpc/xprtrdma/frwr_ops.c
index 285d73246fc2..229fcc9a9064 100644
--- a/net/sunrpc/xprtrdma/frwr_ops.c
+++ b/net/sunrpc/xprtrdma/frwr_ops.c
@@ -530,6 +530,7 @@  void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
 		*prev = last;
 		prev = &last->next;
 	}
+	mr = container_of(last, struct rpcrdma_mr, mr_invwr);
 
 	/* Strong send queue ordering guarantees that when the
 	 * last WR in the chain completes, all WRs in the chain