Re: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-16 Thread Doug Ledford 
On Tue, 2017-08-08 at 19:48 +, Parav Pandit wrote:
> Hi,
> 
> I need to top post because comments are unrelated to past discussion.
> 
> rdma_ah_retrieve_dmac() can never fail for RoCE as its returning
> pointer from structure ah_attr.
> Provider driver doesn't need to check for null pointer as ib/core
> would never call provider if it's not RoCE provider.
> So this memory leak only exist in theory.
> 
> When its null, driver should WARN_ON/BUG_ON in extreme case, but
> that's not necessary either.
> 
> I have patch is progress under internal review that does nice small
> cleanup in many provider drivers that eliminates the check
> completely.
> Waiting for Moni to finish the review.

This sounds like a nice patch to push into for-next, but in the
meantime I took the V2 of this patch as it silences a checker warning.

-- 
Doug Ledford 
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Re: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-16 Thread Doug Ledford 
On Tue, 2017-08-08 at 19:48 +, Parav Pandit wrote:
> Hi,
> 
> I need to top post because comments are unrelated to past discussion.
> 
> rdma_ah_retrieve_dmac() can never fail for RoCE as its returning
> pointer from structure ah_attr.
> Provider driver doesn't need to check for null pointer as ib/core
> would never call provider if it's not RoCE provider.
> So this memory leak only exist in theory.
> 
> When its null, driver should WARN_ON/BUG_ON in extreme case, but
> that's not necessary either.
> 
> I have patch is progress under internal review that does nice small
> cleanup in many provider drivers that eliminates the check
> completely.
> Waiting for Moni to finish the review.

This sounds like a nice patch to push into for-next, but in the
meantime I took the V2 of this patch as it silences a checker warning.

-- 
Doug Ledford 
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

RE: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Parav Pandit 
Hi,

I need to top post because comments are unrelated to past discussion.

rdma_ah_retrieve_dmac() can never fail for RoCE as its returning pointer from 
structure ah_attr.
Provider driver doesn't need to check for null pointer as ib/core would never 
call provider if it's not RoCE provider.
So this memory leak only exist in theory.

When its null, driver should WARN_ON/BUG_ON in extreme case, but that's not 
necessary either.

I have patch is progress under internal review that does nice small cleanup in 
many provider drivers that eliminates the check completely.
Waiting for Moni to finish the review.

Parav

> -Original Message-
> From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
> ow...@vger.kernel.org] On Behalf Of Johannes Thumshirn
> Sent: Tuesday, August 08, 2017 7:59 AM
> To: Colin Ian King <colin.k...@canonical.com>
> Cc: Lijun Ou <ouli...@huawei.com>; Wei Hu <xavier.hu...@huawei.com>;
> Doug Ledford <dledf...@redhat.com>; Sean Hefty <sean.he...@intel.com>;
> Hal Rosenstock <hal.rosenst...@gmail.com>; linux-r...@vger.kernel.org;
> kernel-janit...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] IB/core: fix memory leak on ah on error return path
> 
> On Tue, Aug 08, 2017 at 11:28:16AM +0100, Colin Ian King wrote:
> > I was using the same subject start as the patch that introduced the
> > memory leak and touched the same portion of code. I can resend if necessary.
> 
> I think having the hns prefix makes it clearer, as the patch doesn't touch IB 
> core
> code but hns code. The reference to the patch which introduced the leak is 
> given
> byu the fixes line.
> 
> Thanks,
>   Johannes
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the 
> body
> of a message to majord...@vger.kernel.org More majordomo info at
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
> rnel.org%2Fmajordomo-
> info.html=02%7C01%7Cparav%40mellanox.com%7C4b396976866648806
> a1008d4de5d512c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63
> 6377939837218186=l9u%2Bc6N9E31L2I%2BRDBLNh7KuPi%2Fb49yAlbvw
> qZvGruk%3D=0

RE: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Parav Pandit 
Hi,

I need to top post because comments are unrelated to past discussion.

rdma_ah_retrieve_dmac() can never fail for RoCE as its returning pointer from 
structure ah_attr.
Provider driver doesn't need to check for null pointer as ib/core would never 
call provider if it's not RoCE provider.
So this memory leak only exist in theory.

When its null, driver should WARN_ON/BUG_ON in extreme case, but that's not 
necessary either.

I have patch is progress under internal review that does nice small cleanup in 
many provider drivers that eliminates the check completely.
Waiting for Moni to finish the review.

Parav

> -Original Message-
> From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
> ow...@vger.kernel.org] On Behalf Of Johannes Thumshirn
> Sent: Tuesday, August 08, 2017 7:59 AM
> To: Colin Ian King 
> Cc: Lijun Ou ; Wei Hu ;
> Doug Ledford ; Sean Hefty ;
> Hal Rosenstock ; linux-r...@vger.kernel.org;
> kernel-janit...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] IB/core: fix memory leak on ah on error return path
> 
> On Tue, Aug 08, 2017 at 11:28:16AM +0100, Colin Ian King wrote:
> > I was using the same subject start as the patch that introduced the
> > memory leak and touched the same portion of code. I can resend if necessary.
> 
> I think having the hns prefix makes it clearer, as the patch doesn't touch IB 
> core
> code but hns code. The reference to the patch which introduced the leak is 
> given
> byu the fixes line.
> 
> Thanks,
>   Johannes
> 
> --
> Johannes Thumshirn  Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the 
> body
> of a message to majord...@vger.kernel.org More majordomo info at
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
> rnel.org%2Fmajordomo-
> info.html=02%7C01%7Cparav%40mellanox.com%7C4b396976866648806
> a1008d4de5d512c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63
> 6377939837218186=l9u%2Bc6N9E31L2I%2BRDBLNh7KuPi%2Fb49yAlbvw
> qZvGruk%3D=0

Re: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Johannes Thumshirn 
On Tue, Aug 08, 2017 at 11:28:16AM +0100, Colin Ian King wrote:
> I was using the same subject start as the patch that introduced the
> memory leak and touched the same portion of code. I can resend if necessary.

I think having the hns prefix makes it clearer, as the patch doesn't touch IB
core code but hns code. The reference to the patch which introduced the leak
is given byu the fixes line.

Thanks,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Johannes Thumshirn 
On Tue, Aug 08, 2017 at 11:28:16AM +0100, Colin Ian King wrote:
> I was using the same subject start as the patch that introduced the
> memory leak and touched the same portion of code. I can resend if necessary.

I think having the hns prefix makes it clearer, as the patch doesn't touch IB
core code but hns code. The reference to the patch which introduced the leak
is given byu the fixes line.

Thanks,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Colin Ian King 
On 08/08/17 11:20, Johannes Thumshirn wrote:
> Shouldn't the subject start with "IB/hns:" given it's touching
> drivers/infiniband/hw/hns/hns_roce_ah.c?

I was using the same subject start as the patch that introduced the
memory leak and touched the same portion of code. I can resend if necessary.

> 
> Byte,
>   Johannes
>

Re: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Colin Ian King 
On 08/08/17 11:20, Johannes Thumshirn wrote:
> Shouldn't the subject start with "IB/hns:" given it's touching
> drivers/infiniband/hw/hns/hns_roce_ah.c?

I was using the same subject start as the patch that introduced the
memory leak and touched the same portion of code. I can resend if necessary.

> 
> Byte,
>   Johannes
>

Re: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Johannes Thumshirn 
Shouldn't the subject start with "IB/hns:" given it's touching
drivers/infiniband/hw/hns/hns_roce_ah.c?

Byte,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Johannes Thumshirn 
Shouldn't the subject start with "IB/hns:" given it's touching
drivers/infiniband/hw/hns/hns_roce_ah.c?

Byte,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Colin King 
From: Colin Ian King 

When dmac is NULL, ah is not being freed on the error return path. Fix
this by kfree'ing it.

Detected by CoverityScan, CID#1452636 ("Resource Leak")

Fixes: d8966fcd4c25 ("IB/core: Use rdma_ah_attr accessor functions")
Signed-off-by: Colin Ian King 
---
 drivers/infiniband/hw/hns/hns_roce_ah.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_ah.c 
b/drivers/infiniband/hw/hns/hns_roce_ah.c
index f78a733a63ec..d545302b8ef8 100644
--- a/drivers/infiniband/hw/hns/hns_roce_ah.c
+++ b/drivers/infiniband/hw/hns/hns_roce_ah.c
@@ -64,8 +64,10 @@ struct ib_ah *hns_roce_create_ah(struct ib_pd *ibpd,
} else {
u8 *dmac = rdma_ah_retrieve_dmac(ah_attr);
 
-   if (!dmac)
+   if (!dmac) {
+   kfree(ah);
return ERR_PTR(-EINVAL);
+   }
memcpy(ah->av.mac, dmac, ETH_ALEN);
}
 
-- 
2.11.0

[PATCH] IB/core: fix memory leak on ah on error return path

2017-08-08 Thread Colin King 
From: Colin Ian King 

When dmac is NULL, ah is not being freed on the error return path. Fix
this by kfree'ing it.

Detected by CoverityScan, CID#1452636 ("Resource Leak")

Fixes: d8966fcd4c25 ("IB/core: Use rdma_ah_attr accessor functions")
Signed-off-by: Colin Ian King 
---
 drivers/infiniband/hw/hns/hns_roce_ah.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_ah.c 
b/drivers/infiniband/hw/hns/hns_roce_ah.c
index f78a733a63ec..d545302b8ef8 100644
--- a/drivers/infiniband/hw/hns/hns_roce_ah.c
+++ b/drivers/infiniband/hw/hns/hns_roce_ah.c
@@ -64,8 +64,10 @@ struct ib_ah *hns_roce_create_ah(struct ib_pd *ibpd,
} else {
u8 *dmac = rdma_ah_retrieve_dmac(ah_attr);
 
-   if (!dmac)
+   if (!dmac) {
+   kfree(ah);
return ERR_PTR(-EINVAL);
+   }
memcpy(ah->av.mac, dmac, ETH_ALEN);
}
 
-- 
2.11.0