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 <[email protected]>
Signed-off-by: Xin Tan <[email protected]>
---
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
[email protected] wrote: -----
>To: "Bernard Metzler" <[email protected]>, "Doug Ledford"
><[email protected]>, "Jason Gunthorpe" <[email protected]>,
>[email protected], [email protected]
>From: "Xiyu Yang"
>Sent by: [email protected]
>Date: 04/15/2020 10:46AM
>Cc: [email protected], [email protected], "Xiyu Yang"
><[email protected]>, "Xin Tan" <[email protected]>
>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 <[email protected]>
>Signed-off-by: Xin Tan <[email protected]>
>---
> 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 <[email protected]>
> 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
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 <[email protected]>
> Signed-off-by: Xin Tan <[email protected]>
> 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;
-----"Jason Gunthorpe" <[email protected]> wrote: -----
>To: "Xiyu Yang" <[email protected]>
>From: "Jason Gunthorpe" <[email protected]>
>Date: 04/15/2020 04:09PM
>Cc: "Bernard Metzler" <[email protected]>, "Doug Ledford"
><[email protected]>, [email protected],
>[email protected], [email protected], [email protected],
>"Xin Tan" <[email protected]>
>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 <[email protected]>
>> Signed-off-by: Xin Tan <[email protected]>
>> 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.
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