| Message ID | 20181221021938.13784-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [-next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert | expand |
On Fri, Dec 21, 2018 at 10:19:38AM +0800, YueHaibing wrote: > It should goto err handle if qib_user_sdma_rb_insert fails, > other than success return. > > Fixes: 67810e8c3c01 ("RDMA/qib: Remove all occurrences of BUG_ON()") > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > drivers/infiniband/hw/qib/qib_user_sdma.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c > index 31c523b..e87c0a7 100644 > --- a/drivers/infiniband/hw/qib/qib_user_sdma.c > +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c > @@ -237,6 +237,8 @@ qib_user_sdma_queue_create(struct device *dev, int unit, int ctxt, int sctxt) > > ret = qib_user_sdma_rb_insert(&qib_user_sdma_rb_root, > sdma_rb_node); > + if (ret == 0) > + goto err_rb; > } This doesn't look right, what about undoing the kmalloc directly above? Jason
On Wed, Jan 02, 2019 at 10:12:24AM -0700, Jason Gunthorpe wrote: > On Fri, Dec 21, 2018 at 10:19:38AM +0800, YueHaibing wrote: > > It should goto err handle if qib_user_sdma_rb_insert fails, > > other than success return. > > > > Fixes: 67810e8c3c01 ("RDMA/qib: Remove all occurrences of BUG_ON()") > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > --- > > drivers/infiniband/hw/qib/qib_user_sdma.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c > > index 31c523b..e87c0a7 100644 > > --- a/drivers/infiniband/hw/qib/qib_user_sdma.c > > +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c > > @@ -237,6 +237,8 @@ qib_user_sdma_queue_create(struct device *dev, int unit, int ctxt, int sctxt) > > > > ret = qib_user_sdma_rb_insert(&qib_user_sdma_rb_root, > > sdma_rb_node); > > + if (ret == 0) > > + goto err_rb; > > } > > This doesn't look right, what about undoing the kmalloc directly > above? Back then, I came to conclusion that qib_user_sdma_rb_insert() never returns 0. Otherwise, Dennis would see that BUG_ON() for a long time ago. Thanks > > Jason
On Wed, Jan 02, 2019 at 08:40:50PM +0200, Leon Romanovsky wrote: > On Wed, Jan 02, 2019 at 10:12:24AM -0700, Jason Gunthorpe wrote: > > On Fri, Dec 21, 2018 at 10:19:38AM +0800, YueHaibing wrote: > > > It should goto err handle if qib_user_sdma_rb_insert fails, > > > other than success return. > > > > > > Fixes: 67810e8c3c01 ("RDMA/qib: Remove all occurrences of BUG_ON()") > > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > drivers/infiniband/hw/qib/qib_user_sdma.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c > > > index 31c523b..e87c0a7 100644 > > > +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c > > > @@ -237,6 +237,8 @@ qib_user_sdma_queue_create(struct device *dev, int unit, int ctxt, int sctxt) > > > > > > ret = qib_user_sdma_rb_insert(&qib_user_sdma_rb_root, > > > sdma_rb_node); > > > + if (ret == 0) > > > + goto err_rb; > > > } > > > > This doesn't look right, what about undoing the kmalloc directly > > above? > > Back then, I came to conclusion that qib_user_sdma_rb_insert() never > returns 0. Otherwise, Dennis would see that BUG_ON() for a long time > ago. It fails if the index is in the RB tree, which would indicate a programming bug. The way to replace the BUG_ON is something like: if (WARN_ON(ret == 0)) { kfree() goto err_rb; } Jason
On Wed, Jan 02, 2019 at 12:07:40PM -0700, Jason Gunthorpe wrote: > On Wed, Jan 02, 2019 at 08:40:50PM +0200, Leon Romanovsky wrote: > > On Wed, Jan 02, 2019 at 10:12:24AM -0700, Jason Gunthorpe wrote: > > > On Fri, Dec 21, 2018 at 10:19:38AM +0800, YueHaibing wrote: > > > > It should goto err handle if qib_user_sdma_rb_insert fails, > > > > other than success return. > > > > > > > > Fixes: 67810e8c3c01 ("RDMA/qib: Remove all occurrences of BUG_ON()") > > > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > > drivers/infiniband/hw/qib/qib_user_sdma.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c > > > > index 31c523b..e87c0a7 100644 > > > > +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c > > > > @@ -237,6 +237,8 @@ qib_user_sdma_queue_create(struct device *dev, int unit, int ctxt, int sctxt) > > > > > > > > ret = qib_user_sdma_rb_insert(&qib_user_sdma_rb_root, > > > > sdma_rb_node); > > > > + if (ret == 0) > > > > + goto err_rb; > > > > } > > > > > > This doesn't look right, what about undoing the kmalloc directly > > > above? > > > > Back then, I came to conclusion that qib_user_sdma_rb_insert() never > > returns 0. Otherwise, Dennis would see that BUG_ON() for a long time > > ago. > > It fails if the index is in the RB tree, which would indicate a > programming bug. I tried to say that this programming bug doesn't exist in the reality. > > The way to replace the BUG_ON is something like: > > if (WARN_ON(ret == 0)) { > kfree() > goto err_rb; > } > > Jason
On 2019/1/3 3:07, Jason Gunthorpe wrote: > On Wed, Jan 02, 2019 at 08:40:50PM +0200, Leon Romanovsky wrote: >> On Wed, Jan 02, 2019 at 10:12:24AM -0700, Jason Gunthorpe wrote: >>> On Fri, Dec 21, 2018 at 10:19:38AM +0800, YueHaibing wrote: >>>> It should goto err handle if qib_user_sdma_rb_insert fails, >>>> other than success return. >>>> >>>> Fixes: 67810e8c3c01 ("RDMA/qib: Remove all occurrences of BUG_ON()") >>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>>> drivers/infiniband/hw/qib/qib_user_sdma.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c >>>> index 31c523b..e87c0a7 100644 >>>> +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c >>>> @@ -237,6 +237,8 @@ qib_user_sdma_queue_create(struct device *dev, int unit, int ctxt, int sctxt) >>>> >>>> ret = qib_user_sdma_rb_insert(&qib_user_sdma_rb_root, >>>> sdma_rb_node); >>>> + if (ret == 0) >>>> + goto err_rb; >>>> } >>> >>> This doesn't look right, what about undoing the kmalloc directly >>> above? >> >> Back then, I came to conclusion that qib_user_sdma_rb_insert() never >> returns 0. Otherwise, Dennis would see that BUG_ON() for a long time >> ago. > > It fails if the index is in the RB tree, which would indicate a > programming bug. > > The way to replace the BUG_ON is something like: > Oh, yes, I forget kfree this mem. > if (WARN_ON(ret == 0)) { > kfree() > goto err_rb; > } > > Jason > > . >
On 2019/1/3 3:22, Leon Romanovsky wrote:> On Wed, Jan 02, 2019 at 12:07:40PM -0700, Jason Gunthorpe wrote: >> On Wed, Jan 02, 2019 at 08:40:50PM +0200, Leon Romanovsky wrote: >>> On Wed, Jan 02, 2019 at 10:12:24AM -0700, Jason Gunthorpe wrote: >>>> On Fri, Dec 21, 2018 at 10:19:38AM +0800, YueHaibing wrote: >>>>> It should goto err handle if qib_user_sdma_rb_insert fails, >>>>> other than success return. >>>>> >>>>> Fixes: 67810e8c3c01 ("RDMA/qib: Remove all occurrences of BUG_ON()") >>>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com> >>>>> drivers/infiniband/hw/qib/qib_user_sdma.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c >>>>> index 31c523b..e87c0a7 100644 >>>>> +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c >>>>> @@ -237,6 +237,8 @@ qib_user_sdma_queue_create(struct device *dev, int unit, int ctxt, int sctxt) >>>>> >>>>> ret = qib_user_sdma_rb_insert(&qib_user_sdma_rb_root, >>>>> sdma_rb_node); >>>>> + if (ret == 0) >>>>> + goto err_rb; >>>>> } >>>> >>>> This doesn't look right, what about undoing the kmalloc directly >>>> above? >>> >>> Back then, I came to conclusion that qib_user_sdma_rb_insert() never >>> returns 0. Otherwise, Dennis would see that BUG_ON() for a long time >>> ago. >> >> It fails if the index is in the RB tree, which would indicate a >> programming bug. > > I tried to say that this programming bug doesn't exist in the reality. In view of this, leave it just as this. > >> >> The way to replace the BUG_ON is something like: >> >> if (WARN_ON(ret == 0)) { >> kfree() >> goto err_rb; >> } >> >> Jason
> diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c > b/drivers/infiniband/hw/qib/qib_user_sdma.c > index 31c523b..e87c0a7 100644 > --- a/drivers/infiniband/hw/qib/qib_user_sdma.c > +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c > @@ -237,6 +237,8 @@ qib_user_sdma_queue_create(struct device *dev, > int unit, int ctxt, int sctxt) > > ret = qib_user_sdma_rb_insert(&qib_user_sdma_rb_root, > sdma_rb_node); > + if (ret == 0) > + goto err_rb; > } > pq->sdma_rb_node = sdma_rb_node; > > -- > 2.7.0 > Thanks! This patch is ok. The NULL returned from this routine will result in an error return up to PSM which will fail. The bail branch will do the necessary cleanup, including freeing the node that couldn't be added. Acked-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
On Fri, Jan 04, 2019 at 06:39:50PM +0000, Marciniszyn, Mike wrote: > > diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c > > b/drivers/infiniband/hw/qib/qib_user_sdma.c > > index 31c523b..e87c0a7 100644 > > +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c > > @@ -237,6 +237,8 @@ qib_user_sdma_queue_create(struct device *dev, > > int unit, int ctxt, int sctxt) > > > > ret = qib_user_sdma_rb_insert(&qib_user_sdma_rb_root, > > sdma_rb_node); > > + if (ret == 0) > > + goto err_rb; > > } > > pq->sdma_rb_node = sdma_rb_node; > > > > Thanks! > > This patch is ok. > The NULL returned from this routine will result in an error return up to PSM which will fail. > > The bail branch will do the necessary cleanup, including freeing the > node that couldn't be added. How? sdma_rb_node is a stack variable that is not accessed during cleanup? Jason
diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c index 31c523b..e87c0a7 100644 --- a/drivers/infiniband/hw/qib/qib_user_sdma.c +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c @@ -237,6 +237,8 @@ qib_user_sdma_queue_create(struct device *dev, int unit, int ctxt, int sctxt) ret = qib_user_sdma_rb_insert(&qib_user_sdma_rb_root, sdma_rb_node); + if (ret == 0) + goto err_rb; } pq->sdma_rb_node = sdma_rb_node;
It should goto err handle if qib_user_sdma_rb_insert fails, other than success return. Fixes: 67810e8c3c01 ("RDMA/qib: Remove all occurrences of BUG_ON()") Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- drivers/infiniband/hw/qib/qib_user_sdma.c | 2 ++ 1 file changed, 2 insertions(+)