* [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: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
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
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).