[PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node

[PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node

[Xiyu Yang]

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

Re: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node

[Bernard Metzler]

-----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>

Re: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node

[Jason Gunthorpe]

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;

RE: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node

[Bernard Metzler]

-----"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.

Re: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node

[Jason Gunthorpe]

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

Re: [PATCH] RDMA/siw: Fix potential siw_mem refcnt leak in nr_add_node

[Markus Elfring]

> 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