| Message ID | 20220125191717.2945308-1-dan.aloni@vastdata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xrpcrdma: add missing error checks in rpcrdma_ep_destroy | expand |
> On Jan 25, 2022, at 2:17 PM, Dan Aloni <dan.aloni@vastdata.com> wrote: > > These pointers can be non-NULL and contain errors if initialization is > aborted early. This is similar to how `__svc_rdma_free` takes care of > it. IIUC the only place that can set these values to an ERR_PTR is rpcrdma_ep_create() ? I think I'd rather have rpcrdma_ep_create() set the fields to NULL in the error cases. Good catch. I'm afraid to ask how you found this. > Signed-off-by: Dan Aloni <dan.aloni@vastdata.com> > --- > net/sunrpc/xprtrdma/verbs.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c > index f172d1298013..7f3173073e72 100644 > --- a/net/sunrpc/xprtrdma/verbs.c > +++ b/net/sunrpc/xprtrdma/verbs.c > @@ -336,14 +336,14 @@ static void rpcrdma_ep_destroy(struct kref *kref) > ep->re_id->qp = NULL; > } > > - if (ep->re_attr.recv_cq) > + if (ep->re_attr.recv_cq && !IS_ERR(ep->re_attr.recv_cq)) > ib_free_cq(ep->re_attr.recv_cq); > ep->re_attr.recv_cq = NULL; > - if (ep->re_attr.send_cq) > + if (ep->re_attr.send_cq && !IS_ERR(ep->re_attr.send_cq)) > ib_free_cq(ep->re_attr.send_cq); > ep->re_attr.send_cq = NULL; > > - if (ep->re_pd) > + if (ep->re_pd && !IS_ERR(ep->re_pd)) > ib_dealloc_pd(ep->re_pd); > ep->re_pd = NULL; > > -- > 2.23.0 > -- Chuck Lever
On Tue, Jan 25, 2022 at 07:30:05PM +0000, Chuck Lever III wrote: > > > > On Jan 25, 2022, at 2:17 PM, Dan Aloni <dan.aloni@vastdata.com> wrote: > > > > These pointers can be non-NULL and contain errors if initialization is > > aborted early. This is similar to how `__svc_rdma_free` takes care of > > it. > > IIUC the only place that can set these values to an > ERR_PTR is rpcrdma_ep_create() ? I think I'd rather > have rpcrdma_ep_create() set the fields to NULL in > the error cases. Actually that was my initialization draft but then I saw `__svc_rdma_free`. Will send over. > Good catch. I'm afraid to ask how you found this. Just some adapter entering error state in firmware.
diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c index f172d1298013..7f3173073e72 100644 --- a/net/sunrpc/xprtrdma/verbs.c +++ b/net/sunrpc/xprtrdma/verbs.c @@ -336,14 +336,14 @@ static void rpcrdma_ep_destroy(struct kref *kref) ep->re_id->qp = NULL; } - if (ep->re_attr.recv_cq) + if (ep->re_attr.recv_cq && !IS_ERR(ep->re_attr.recv_cq)) ib_free_cq(ep->re_attr.recv_cq); ep->re_attr.recv_cq = NULL; - if (ep->re_attr.send_cq) + if (ep->re_attr.send_cq && !IS_ERR(ep->re_attr.send_cq)) ib_free_cq(ep->re_attr.send_cq); ep->re_attr.send_cq = NULL; - if (ep->re_pd) + if (ep->re_pd && !IS_ERR(ep->re_pd)) ib_dealloc_pd(ep->re_pd); ep->re_pd = NULL;
These pointers can be non-NULL and contain errors if initialization is aborted early. This is similar to how `__svc_rdma_free` takes care of it. Signed-off-by: Dan Aloni <dan.aloni@vastdata.com> --- net/sunrpc/xprtrdma/verbs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)