| Message ID | 20180803192431.pmisuz3qwefd45se@kili.mountain (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | RDMA: Fix an uninitialized variable | expand |
On Fri, Aug 03, 2018 at 10:24:31PM +0300, Dan Carpenter wrote: > A couple of the callers assume that ib_post_send() initializes > *bad_send_wr. It doesn't look like we can rely on the ->post_send() > function to initialize it. For example in the i40iw_post_recv() > function and there are some error paths there which don't set it. I think those are bugs in the drivers, if bad_wr is provided, and post_send fails then it must be set to the wr that has a problem, left unset/uninit is incorrect. This is a high performance call path, so I'd prefer not to see unnecessary babying of drivers.. Bart? Thanks, Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2018-08-03 at 13:43 -0600, Jason Gunthorpe wrote: > On Fri, Aug 03, 2018 at 10:24:31PM +0300, Dan Carpenter wrote: > > A couple of the callers assume that ib_post_send() initializes > > *bad_send_wr. It doesn't look like we can rely on the ->post_send() > > function to initialize it. For example in the i40iw_post_recv() > > function and there are some error paths there which don't set it. > > I think those are bugs in the drivers, if bad_wr is provided, and > post_send fails then it must be set to the wr that has a problem, left > unset/uninit is incorrect. > > This is a high performance call path, so I'd prefer not to see > unnecessary babying of drivers.. > > Bart? Hello Dan and Jason, The documentation of the bad_wr parameter for ib_post_send(), ib_post_recv() and ib_post_srq_recv() is as follows: * @bad_...wr: On an immediate failure, this parameter will reference * the work request that failed to be posted on the QP. In other words, nothing is guaranteed about the *bad_wr value if these functions return 0. My proposal is to proceed as follows: * In the ULPs that read *bad_wr if one of these functions returns 0, initialize *bad_wr before calling one of these functions (I think all ULPs already do this). * Make sure that all HW drivers set *bad_wr if the return value is not 0. Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 8caee90e6dee..0ebacbe5019e 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -3389,6 +3389,8 @@ static inline int ib_post_send(struct ib_qp *qp, { const struct ib_send_wr *dummy; + if (bad_send_wr) + *bad_send_wr = NULL; return qp->device->post_send(qp, send_wr, bad_send_wr ? : &dummy); }
A couple of the callers assume that ib_post_send() initializes *bad_send_wr. It doesn't look like we can rely on the ->post_send() function to initialize it. For example in the i40iw_post_recv() function and there are some error paths there which don't set it. Here are the static checker warnings for reference: net/sunrpc/xprtrdma/verbs.c:1564 rpcrdma_post_recvs() error: uninitialized symbol 'bad_wr'. net/sunrpc/xprtrdma/svc_rdma_rw.c:350 svc_rdma_post_chunk_ctxt() error: uninitialized symbol 'bad_wr'. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html