* [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node
@ 2020-04-15 8:39 Xiyu Yang
2020-04-15 9:19 ` Bernard Metzler
2020-04-15 14:09 ` Jason Gunthorpe
0 siblings, 2 replies; 6+ messages in thread
From: Xiyu Yang @ 2020-04-15 8:39 UTC (permalink / raw)
To: Bernard Metzler, Doug Ledford, Jason Gunthorpe, linux-rdma,
linux-kernel
Cc: yuanxzhang, kjlu, Xiyu Yang, Xin Tan
siw_fastreg_mr() invokes siw_mem_id2obj(), which returns a local
reference of the siw_mem object to "mem" with increased refcnt.
When siw_fastreg_mr() returns, "mem" becomes invalid, so the refcount
should be decreased to keep refcount balanced.
The issue happens in one error path of siw_fastreg_mr(). When "base_mr"
equals to NULL but "mem" is not NULL, the function forgets to decrease
the refcnt increased by siw_mem_id2obj() and causes a refcnt leak.
Fix this issue by calling siw_mem_put() on this error path when mem is
not NULL.
Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
---
drivers/infiniband/sw/siw/siw_qp_tx.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index ae92c8080967..86044a44b83b 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -926,6 +926,8 @@ static int siw_fastreg_mr(struct ib_pd *pd, struct siw_sqe *sqe)
siw_dbg_pd(pd, "STag 0x%08x\n", sqe->rkey);
if (unlikely(!mem || !base_mr)) {
+ if (mem)
+ siw_mem_put(mem);
pr_warn("siw: fastreg: STag 0x%08x unknown\n", sqe->rkey);
return -EINVAL;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node
2020-04-15 8:39 [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node Xiyu Yang
@ 2020-04-15 9:19 ` Bernard Metzler
2020-04-15 14:09 ` Jason Gunthorpe
1 sibling, 0 replies; 6+ messages in thread
From: Bernard Metzler @ 2020-04-15 9:19 UTC (permalink / raw)
To: Xiyu Yang
Cc: Doug Ledford, Jason Gunthorpe, linux-rdma, linux-kernel,
yuanxzhang, kjlu, Xin Tan
-----linux-rdma-owner@vger.kernel.org wrote: -----
>To: "Bernard Metzler" <bmt@zurich.ibm.com>, "Doug Ledford"
><dledford@redhat.com>, "Jason Gunthorpe" <jgg@ziepe.ca>,
>linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
>From: "Xiyu Yang"
>Sent by: linux-rdma-owner@vger.kernel.org
>Date: 04/15/2020 10:46AM
>Cc: yuanxzhang@fudan.edu.cn, kjlu@umn.edu, "Xiyu Yang"
><xiyuyang19@fudan.edu.cn>, "Xin Tan" <tanxin.ctf@gmail.com>
>Subject: [EXTERNAL] [PATCH] RDMA/siw: Fix potential siw_mem refcnt
>leak in nr_add_node
>
>siw_fastreg_mr() invokes siw_mem_id2obj(), which returns a local
>reference of the siw_mem object to "mem" with increased refcnt.
>When siw_fastreg_mr() returns, "mem" becomes invalid, so the refcount
>should be decreased to keep refcount balanced.
>
>The issue happens in one error path of siw_fastreg_mr(). When
>"base_mr"
>equals to NULL but "mem" is not NULL, the function forgets to
>decrease
>the refcnt increased by siw_mem_id2obj() and causes a refcnt leak.
>
>Fix this issue by calling siw_mem_put() on this error path when mem
>is
>not NULL.
>
>Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
>Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
>---
> drivers/infiniband/sw/siw/siw_qp_tx.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
>b/drivers/infiniband/sw/siw/siw_qp_tx.c
>index ae92c8080967..86044a44b83b 100644
>--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
>+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
>@@ -926,6 +926,8 @@ static int siw_fastreg_mr(struct ib_pd *pd,
>struct siw_sqe *sqe)
> siw_dbg_pd(pd, "STag 0x%08x\n", sqe->rkey);
>
> if (unlikely(!mem || !base_mr)) {
>+ if (mem)
>+ siw_mem_put(mem);
> pr_warn("siw: fastreg: STag 0x%08x unknown\n", sqe->rkey);
> return -EINVAL;
> }
>--
I agree - thanks for the fix!
Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node
2020-04-15 8:39 [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node Xiyu Yang
2020-04-15 9:19 ` Bernard Metzler
@ 2020-04-15 14:09 ` Jason Gunthorpe
2020-04-15 14:16 ` Bernard Metzler
1 sibling, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2020-04-15 14:09 UTC (permalink / raw)
To: Xiyu Yang
Cc: Bernard Metzler, Doug Ledford, linux-rdma, linux-kernel,
yuanxzhang, kjlu, Xin Tan
On Wed, Apr 15, 2020 at 04:39:08PM +0800, Xiyu Yang wrote:
> siw_fastreg_mr() invokes siw_mem_id2obj(), which returns a local
> reference of the siw_mem object to "mem" with increased refcnt.
> When siw_fastreg_mr() returns, "mem" becomes invalid, so the refcount
> should be decreased to keep refcount balanced.
>
> The issue happens in one error path of siw_fastreg_mr(). When "base_mr"
> equals to NULL but "mem" is not NULL, the function forgets to decrease
> the refcnt increased by siw_mem_id2obj() and causes a refcnt leak.
>
> Fix this issue by calling siw_mem_put() on this error path when mem is
> not NULL.
>
> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
> drivers/infiniband/sw/siw/siw_qp_tx.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
> index ae92c8080967..86044a44b83b 100644
> +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
> @@ -926,6 +926,8 @@ static int siw_fastreg_mr(struct ib_pd *pd, struct siw_sqe *sqe)
> siw_dbg_pd(pd, "STag 0x%08x\n", sqe->rkey);
>
> if (unlikely(!mem || !base_mr)) {
> + if (mem)
> + siw_mem_put(mem);
> pr_warn("siw: fastreg: STag 0x%08x unknown\n", sqe->rkey);
> return -EINVAL;
> }
I think I prefer this version, which is what I'll use if nobody has concerns:
diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c b/drivers/infiniband/sw/siw/siw_qp_tx.c
index ae92c8080967c5..0580bbf535ceb7 100644
--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
@@ -920,20 +920,28 @@ static int siw_fastreg_mr(struct ib_pd *pd, struct siw_sqe *sqe)
{
struct ib_mr *base_mr = (struct ib_mr *)(uintptr_t)sqe->base_mr;
struct siw_device *sdev = to_siw_dev(pd->device);
- struct siw_mem *mem = siw_mem_id2obj(sdev, sqe->rkey >> 8);
+ struct siw_mem *mem;
int rv = 0;
siw_dbg_pd(pd, "STag 0x%08x\n", sqe->rkey);
- if (unlikely(!mem || !base_mr)) {
+ if (unlikely(!base_mr)) {
pr_warn("siw: fastreg: STag 0x%08x unknown\n", sqe->rkey);
return -EINVAL;
}
+
if (unlikely(base_mr->rkey >> 8 != sqe->rkey >> 8)) {
pr_warn("siw: fastreg: STag 0x%08x: bad MR\n", sqe->rkey);
+ return -EINVAL;
+ }
+
+ mem = siw_mem_id2obj(sdev, sqe->rkey >> 8);
+ if (unlikely(!mem)) {
+ pr_warn("siw: fastreg: STag 0x%08x unknown\n", sqe->rkey);
rv = -EINVAL;
goto out;
}
+
if (unlikely(mem->pd != pd)) {
pr_warn("siw: fastreg: PD mismatch\n");
rv = -EINVAL;
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node
2020-04-15 14:09 ` Jason Gunthorpe
@ 2020-04-15 14:16 ` Bernard Metzler
2020-04-15 14:27 ` Jason Gunthorpe
0 siblings, 1 reply; 6+ messages in thread
From: Bernard Metzler @ 2020-04-15 14:16 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Xiyu Yang, Doug Ledford, linux-rdma, linux-kernel, yuanxzhang,
kjlu, Xin Tan
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: -----
>To: "Xiyu Yang" <xiyuyang19@fudan.edu.cn>
>From: "Jason Gunthorpe" <jgg@ziepe.ca>
>Date: 04/15/2020 04:09PM
>Cc: "Bernard Metzler" <bmt@zurich.ibm.com>, "Doug Ledford"
><dledford@redhat.com>, linux-rdma@vger.kernel.org,
>linux-kernel@vger.kernel.org, yuanxzhang@fudan.edu.cn, kjlu@umn.edu,
>"Xin Tan" <tanxin.ctf@gmail.com>
>Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix potential siw_mem
>refcnt leak in nr_add_node
>
>On Wed, Apr 15, 2020 at 04:39:08PM +0800, Xiyu Yang wrote:
>> siw_fastreg_mr() invokes siw_mem_id2obj(), which returns a local
>> reference of the siw_mem object to "mem" with increased refcnt.
>> When siw_fastreg_mr() returns, "mem" becomes invalid, so the
>refcount
>> should be decreased to keep refcount balanced.
>>
>> The issue happens in one error path of siw_fastreg_mr(). When
>"base_mr"
>> equals to NULL but "mem" is not NULL, the function forgets to
>decrease
>> the refcnt increased by siw_mem_id2obj() and causes a refcnt leak.
>>
>> Fix this issue by calling siw_mem_put() on this error path when mem
>is
>> not NULL.
>>
>> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
>> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
>> drivers/infiniband/sw/siw/siw_qp_tx.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
>b/drivers/infiniband/sw/siw/siw_qp_tx.c
>> index ae92c8080967..86044a44b83b 100644
>> +++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
>> @@ -926,6 +926,8 @@ static int siw_fastreg_mr(struct ib_pd *pd,
>struct siw_sqe *sqe)
>> siw_dbg_pd(pd, "STag 0x%08x\n", sqe->rkey);
>>
>> if (unlikely(!mem || !base_mr)) {
>> + if (mem)
>> + siw_mem_put(mem);
>> pr_warn("siw: fastreg: STag 0x%08x unknown\n", sqe->rkey);
>> return -EINVAL;
>> }
>
>I think I prefer this version, which is what I'll use if nobody has
>concerns:
>
>diff --git a/drivers/infiniband/sw/siw/siw_qp_tx.c
>b/drivers/infiniband/sw/siw/siw_qp_tx.c
>index ae92c8080967c5..0580bbf535ceb7 100644
>--- a/drivers/infiniband/sw/siw/siw_qp_tx.c
>+++ b/drivers/infiniband/sw/siw/siw_qp_tx.c
>@@ -920,20 +920,28 @@ static int siw_fastreg_mr(struct ib_pd *pd,
>struct siw_sqe *sqe)
> {
> struct ib_mr *base_mr = (struct ib_mr *)(uintptr_t)sqe->base_mr;
> struct siw_device *sdev = to_siw_dev(pd->device);
>- struct siw_mem *mem = siw_mem_id2obj(sdev, sqe->rkey >> 8);
>+ struct siw_mem *mem;
> int rv = 0;
>
> siw_dbg_pd(pd, "STag 0x%08x\n", sqe->rkey);
>
>- if (unlikely(!mem || !base_mr)) {
>+ if (unlikely(!base_mr)) {
> pr_warn("siw: fastreg: STag 0x%08x unknown\n", sqe->rkey);
> return -EINVAL;
> }
>+
> if (unlikely(base_mr->rkey >> 8 != sqe->rkey >> 8)) {
> pr_warn("siw: fastreg: STag 0x%08x: bad MR\n", sqe->rkey);
>+ return -EINVAL;
>+ }
>+
>+ mem = siw_mem_id2obj(sdev, sqe->rkey >> 8);
>+ if (unlikely(!mem)) {
>+ pr_warn("siw: fastreg: STag 0x%08x unknown\n", sqe->rkey);
> rv = -EINVAL;
> goto out;
> }
>+
Fine with me in principle, but we would have to return
directly here as well - since we do not have a valid mem
to be put back.
Thanks
Bernard.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node
2020-04-15 14:16 ` Bernard Metzler
@ 2020-04-15 14:27 ` Jason Gunthorpe
0 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2020-04-15 14:27 UTC (permalink / raw)
To: Bernard Metzler
Cc: Xiyu Yang, Doug Ledford, linux-rdma, linux-kernel, yuanxzhang,
kjlu, Xin Tan
On Wed, Apr 15, 2020 at 02:16:54PM +0000, Bernard Metzler wrote:
> Fine with me in principle, but we would have to return
> directly here as well - since we do not have a valid mem
> to be put back.
Woops, yes, thanks
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node
@ 2020-04-15 11:00 Markus Elfring
0 siblings, 0 replies; 6+ messages in thread
From: Markus Elfring @ 2020-04-15 11:00 UTC (permalink / raw)
To: Xiyu Yang, Xin Tan, linux-rdma
Cc: linux-kernel, Bernard Metzler, Doug Ledford, Jason Gunthorpe,
Kangjie Lu, Yuan Zhang
> The issue happens in one error path of siw_fastreg_mr(). When "base_mr"
> equals to NULL but "mem" is not NULL, the function forgets to decrease
> the refcnt increased by siw_mem_id2obj() and causes a refcnt leak.
How do you think about to mention the terms “exception handling”
and “reference counting” in the commit message?
Would you like to add the tag “Fixes” to the change description?
Regards,
Markus
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-15 14:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-15 8:39 [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node Xiyu Yang
2020-04-15 9:19 ` Bernard Metzler
2020-04-15 14:09 ` Jason Gunthorpe
2020-04-15 14:16 ` Bernard Metzler
2020-04-15 14:27 ` Jason Gunthorpe
-- strict thread matches above, loose matches on Subject: below --
2020-04-15 11:00 Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox