* [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert
@ 2018-12-21 2:19 YueHaibing
2019-01-02 17:12 ` Jason Gunthorpe
2019-01-04 18:39 ` Marciniszyn, Mike
0 siblings, 2 replies; 9+ messages in thread
From: YueHaibing @ 2018-12-21 2:19 UTC (permalink / raw)
To: dennis.dalessandro, mike.marciniszyn, dledford, jgg
Cc: linux-kernel, linux-rdma, YueHaibing
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;
}
pq->sdma_rb_node = sdma_rb_node;
--
2.7.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert 2018-12-21 2:19 [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert YueHaibing @ 2019-01-02 17:12 ` Jason Gunthorpe 2019-01-02 18:40 ` Leon Romanovsky 2019-01-04 18:39 ` Marciniszyn, Mike 1 sibling, 1 reply; 9+ messages in thread From: Jason Gunthorpe @ 2019-01-02 17:12 UTC (permalink / raw) To: YueHaibing Cc: dennis.dalessandro, mike.marciniszyn, dledford, linux-kernel, linux-rdma 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert 2019-01-02 17:12 ` Jason Gunthorpe @ 2019-01-02 18:40 ` Leon Romanovsky 2019-01-02 19:07 ` Jason Gunthorpe 0 siblings, 1 reply; 9+ messages in thread From: Leon Romanovsky @ 2019-01-02 18:40 UTC (permalink / raw) To: Jason Gunthorpe Cc: YueHaibing, dennis.dalessandro, mike.marciniszyn, dledford, linux-kernel, linux-rdma [-- Attachment #1: Type: text/plain, Size: 1180 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert 2019-01-02 18:40 ` Leon Romanovsky @ 2019-01-02 19:07 ` Jason Gunthorpe 2019-01-02 19:22 ` Leon Romanovsky 2019-01-03 2:05 ` YueHaibing 0 siblings, 2 replies; 9+ messages in thread From: Jason Gunthorpe @ 2019-01-02 19:07 UTC (permalink / raw) To: Leon Romanovsky Cc: YueHaibing, dennis.dalessandro, mike.marciniszyn, dledford, linux-kernel, linux-rdma 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert 2019-01-02 19:07 ` Jason Gunthorpe @ 2019-01-02 19:22 ` Leon Romanovsky 2019-01-04 6:08 ` YueHaibing 2019-01-03 2:05 ` YueHaibing 1 sibling, 1 reply; 9+ messages in thread From: Leon Romanovsky @ 2019-01-02 19:22 UTC (permalink / raw) To: Jason Gunthorpe Cc: YueHaibing, dennis.dalessandro, mike.marciniszyn, dledford, linux-kernel, linux-rdma [-- Attachment #1: Type: text/plain, Size: 1629 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert 2019-01-02 19:22 ` Leon Romanovsky @ 2019-01-04 6:08 ` YueHaibing 0 siblings, 0 replies; 9+ messages in thread From: YueHaibing @ 2019-01-04 6:08 UTC (permalink / raw) To: Leon Romanovsky, Jason Gunthorpe Cc: dennis.dalessandro, mike.marciniszyn, dledford, linux-kernel, linux-rdma 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert 2019-01-02 19:07 ` Jason Gunthorpe 2019-01-02 19:22 ` Leon Romanovsky @ 2019-01-03 2:05 ` YueHaibing 1 sibling, 0 replies; 9+ messages in thread From: YueHaibing @ 2019-01-03 2:05 UTC (permalink / raw) To: Jason Gunthorpe, Leon Romanovsky Cc: dennis.dalessandro, mike.marciniszyn, dledford, linux-kernel, linux-rdma 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 > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert 2018-12-21 2:19 [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert YueHaibing 2019-01-02 17:12 ` Jason Gunthorpe @ 2019-01-04 18:39 ` Marciniszyn, Mike 2019-01-04 22:38 ` Jason Gunthorpe 1 sibling, 1 reply; 9+ messages in thread From: Marciniszyn, Mike @ 2019-01-04 18:39 UTC (permalink / raw) To: YueHaibing; +Cc: linux-kernel, linux-rdma, Dalessandro, Dennis, dledford, jgg > 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert 2019-01-04 18:39 ` Marciniszyn, Mike @ 2019-01-04 22:38 ` Jason Gunthorpe 0 siblings, 0 replies; 9+ messages in thread From: Jason Gunthorpe @ 2019-01-04 22:38 UTC (permalink / raw) To: Marciniszyn, Mike Cc: YueHaibing, linux-kernel, linux-rdma, Dalessandro, Dennis, dledford 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-01-04 22:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-21 2:19 [PATCH -next] IB/qib: Add missing err handle for qib_user_sdma_rb_insert YueHaibing 2019-01-02 17:12 ` Jason Gunthorpe 2019-01-02 18:40 ` Leon Romanovsky 2019-01-02 19:07 ` Jason Gunthorpe 2019-01-02 19:22 ` Leon Romanovsky 2019-01-04 6:08 ` YueHaibing 2019-01-03 2:05 ` YueHaibing 2019-01-04 18:39 ` Marciniszyn, Mike 2019-01-04 22:38 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).