| Message ID | 20171208190106.GA32066@bryantan-devbox.prom.eng.vmware.com.prom.eng.vmware.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > The QP cleanup did not previously call ib_umem_release. Fix this. > > Testing Done: ibv_rc_pingpong, rping, perftests. > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > Reviewed-by: Adit Ranadive <aditr@vmware.com> > Reviewed-by: Aditya Sarwade <asarwade@vmware.com> > Reviewed-by: Jorgen Hansen <jhansen@vmware.com> > Signed-off-by: Bryan Tan <bryantan@vmware.com> > --- > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > index b932b7e..77e7e57 100644 > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > atomic_dec(&qp->refcnt); > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > + if (!qp->is_kernel) { > + if (qp->rumem) IS_ERR_OR_NULL? > + ib_umem_release(qp->rumem); > + if (qp->sumem) > + ib_umem_release(qp->sumem); > + } > + > pvrdma_page_dir_cleanup(dev, &qp->pdir); > > kfree(qp); > -- > 1.8.5.6 > > -- > 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 -- 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 Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > Reviewed-by: Adit Ranadive <aditr@vmware.com> > > Reviewed-by: Aditya Sarwade <asarwade@vmware.com> > > Reviewed-by: Jorgen Hansen <jhansen@vmware.com> > > Signed-off-by: Bryan Tan <bryantan@vmware.com> > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > index b932b7e..77e7e57 100644 > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > atomic_dec(&qp->refcnt); > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > + if (!qp->is_kernel) { > > + if (qp->rumem) > > IS_ERR_OR_NULL? How can we get an ERR_PTR in rumem? 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 Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > Reviewed-by: Adit Ranadive <aditr@vmware.com> > > > Reviewed-by: Aditya Sarwade <asarwade@vmware.com> > > > Reviewed-by: Jorgen Hansen <jhansen@vmware.com> > > > Signed-off-by: Bryan Tan <bryantan@vmware.com> > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > index b932b7e..77e7e57 100644 > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > atomic_dec(&qp->refcnt); > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > + if (!qp->is_kernel) { > > > + if (qp->rumem) > > > > IS_ERR_OR_NULL? > > How can we get an ERR_PTR in rumem? qp->rumem is set with ib_umem_get which can either return ib_umem pointer or ERR_PTR. So i looked at other places which calls ib_umem_release. The first one that pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. So assuming it can be used here. > > 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 -- 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 Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote: > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > > Reviewed-by: Adit Ranadive <aditr@vmware.com> > > > > Reviewed-by: Aditya Sarwade <asarwade@vmware.com> > > > > Reviewed-by: Jorgen Hansen <jhansen@vmware.com> > > > > Signed-off-by: Bryan Tan <bryantan@vmware.com> > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > index b932b7e..77e7e57 100644 > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > > atomic_dec(&qp->refcnt); > > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > > > + if (!qp->is_kernel) { > > > > + if (qp->rumem) > > > > > > IS_ERR_OR_NULL? > > > > How can we get an ERR_PTR in rumem? > > qp->rumem is set with ib_umem_get which can either return ib_umem pointer > or ERR_PTR. > So i looked at other places which calls ib_umem_release. The first one that > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. > > So assuming it can be used here. We check in pvrdma_qp.c pvrdma_create_qp the result of ib_umem_get. If it's ERR_PTR we fail the QP creation: > qp->rumem = ib_umem_get(pd->uobject->context, > ucmd.rbuf_addr, > ucmd.rbuf_size, 0, 0); > if (IS_ERR(qp->rumem)) { > ret = PTR_ERR(qp->rumem); > goto err_qp; > } so checking IS_ERR_OR_NULL isn't necessary here (and same for sumem). Thanks, Bryan -- 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 Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote: > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > > Reviewed-by: Adit Ranadive <aditr@vmware.com> > > > > Reviewed-by: Aditya Sarwade <asarwade@vmware.com> > > > > Reviewed-by: Jorgen Hansen <jhansen@vmware.com> > > > > Signed-off-by: Bryan Tan <bryantan@vmware.com> > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > index b932b7e..77e7e57 100644 > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > > atomic_dec(&qp->refcnt); > > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > > > + if (!qp->is_kernel) { > > > > + if (qp->rumem) > > > > > > IS_ERR_OR_NULL? > > > > How can we get an ERR_PTR in rumem? > > qp->rumem is set with ib_umem_get which can either return ib_umem pointer > or ERR_PTR. In generall ERR_PTR's should not be permanently stored in structs and I do not expect to see IS_ERR_OR_NULL used. Several kernel devs consider is an abomination, and it certainly should *NEVER* be used on a ptr which cannot, by design, have an ERR_PTR in it. At least vmw_pvrdma appears to be OK: qp->rumem = ib_umem_get(pd->uobject->context, ucmd.rbuf_addr, ucmd.rbuf_size, 0, 0); if (IS_ERR(qp->rumem)) { ret = PTR_ERR(qp->rumem); goto err_qp; [..] err_qp: kfree(qp); So we cannot have a 'qp' struct with an ERR_PTR in rumem. > So i looked at other places which calls ib_umem_release. The first one that > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. The test in bnxt looks wrong to me, and should be removed: umem = ib_umem_get(context, ureq.qprva, bytes, IB_ACCESS_LOCAL_WRITE, 1); if (IS_ERR(umem)) goto rqfail; qp->rumem = umem; So rumem can never be assigned to PTR_ERR, and this is garbage: if (!IS_ERR_OR_NULL(qp->rumem)) ib_umem_release(qp->rumem); Similar remark for other tests in that driver. Care to send a patch? 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 Wed, Dec 13, 2017 at 10:55:18AM -0800, Bryan Tan wrote: > On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote: > > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > > > Reviewed-by: Adit Ranadive <aditr@vmware.com> > > > > > Reviewed-by: Aditya Sarwade <asarwade@vmware.com> > > > > > Reviewed-by: Jorgen Hansen <jhansen@vmware.com> > > > > > Signed-off-by: Bryan Tan <bryantan@vmware.com> > > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > > index b932b7e..77e7e57 100644 > > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > > > atomic_dec(&qp->refcnt); > > > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > > > > > + if (!qp->is_kernel) { > > > > > + if (qp->rumem) > > > > > > > > IS_ERR_OR_NULL? > > > > > > How can we get an ERR_PTR in rumem? > > > > qp->rumem is set with ib_umem_get which can either return ib_umem pointer > > or ERR_PTR. > > So i looked at other places which calls ib_umem_release. The first one that > > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. > > > > So assuming it can be used here. > > We check in pvrdma_qp.c pvrdma_create_qp the result of ib_umem_get. > If it's ERR_PTR we fail the QP creation: > > > qp->rumem = ib_umem_get(pd->uobject->context, > > ucmd.rbuf_addr, > > ucmd.rbuf_size, 0, 0); > > if (IS_ERR(qp->rumem)) { > > ret = PTR_ERR(qp->rumem); > > goto err_qp; > > } > > so checking IS_ERR_OR_NULL isn't necessary here (and same for sumem). I see. Missed that. > > Thanks, > Bryan -- 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 Wed, Dec 13, 2017 at 09:04:22PM +0200, Yuval Shaia wrote: > On Wed, Dec 13, 2017 at 10:55:18AM -0800, Bryan Tan wrote: > > On Wed, Dec 13, 2017 at 08:40:13PM +0200, Yuval Shaia wrote: > > > On Wed, Dec 13, 2017 at 09:17:55AM -0700, Jason Gunthorpe wrote: > > > > On Wed, Dec 13, 2017 at 10:53:18AM +0200, Yuval Shaia wrote: > > > > > On Fri, Dec 08, 2017 at 11:01:18AM -0800, Bryan Tan wrote: > > > > > > The QP cleanup did not previously call ib_umem_release. Fix this. > > > > > > > > > > > > Testing Done: ibv_rc_pingpong, rping, perftests. > > > > > > > > > > > > Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") > > > > > > Reviewed-by: Adit Ranadive <aditr@vmware.com> > > > > > > Reviewed-by: Aditya Sarwade <asarwade@vmware.com> > > > > > > Reviewed-by: Jorgen Hansen <jhansen@vmware.com> > > > > > > Signed-off-by: Bryan Tan <bryantan@vmware.com> > > > > > > drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > > > index b932b7e..77e7e57 100644 > > > > > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c > > > > > > @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) > > > > > > atomic_dec(&qp->refcnt); > > > > > > wait_event(qp->wait, !atomic_read(&qp->refcnt)); > > > > > > > > > > > > + if (!qp->is_kernel) { > > > > > > + if (qp->rumem) > > > > > > > > > > IS_ERR_OR_NULL? > > > > > > > > How can we get an ERR_PTR in rumem? > > > > > > qp->rumem is set with ib_umem_get which can either return ib_umem pointer > > > or ERR_PTR. > > > So i looked at other places which calls ib_umem_release. The first one that > > > pops up is bnxt_re which do this check with IS_ERR_OR_NULL?. > > > > > > So assuming it can be used here. > > > > We check in pvrdma_qp.c pvrdma_create_qp the result of ib_umem_get. > > If it's ERR_PTR we fail the QP creation: > > > > > qp->rumem = ib_umem_get(pd->uobject->context, > > > ucmd.rbuf_addr, > > > ucmd.rbuf_size, 0, 0); > > > if (IS_ERR(qp->rumem)) { > > > ret = PTR_ERR(qp->rumem); > > > goto err_qp; > > > } > > > > so checking IS_ERR_OR_NULL isn't necessary here (and same for sumem). > > I see. > Missed that. That being said i have no other questions. Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com> > > > > > Thanks, > > Bryan > -- > 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 -- 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/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c index b932b7e..77e7e57 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c @@ -430,6 +430,13 @@ static void pvrdma_free_qp(struct pvrdma_qp *qp) atomic_dec(&qp->refcnt); wait_event(qp->wait, !atomic_read(&qp->refcnt)); + if (!qp->is_kernel) { + if (qp->rumem) + ib_umem_release(qp->rumem); + if (qp->sumem) + ib_umem_release(qp->sumem); + } + pvrdma_page_dir_cleanup(dev, &qp->pdir); kfree(qp);
The QP cleanup did not previously call ib_umem_release. Fix this. Testing Done: ibv_rc_pingpong, rping, perftests. Fixes: 29c8d9eba550 ("IB: Add vmw_pvrdma driver") Reviewed-by: Adit Ranadive <aditr@vmware.com> Reviewed-by: Aditya Sarwade <asarwade@vmware.com> Reviewed-by: Jorgen Hansen <jhansen@vmware.com> Signed-off-by: Bryan Tan <bryantan@vmware.com> --- drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 7 +++++++ 1 file changed, 7 insertions(+)