Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1706294 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] IB/core: fix memory leak on ah on error return path Colin King <colin.king@canonical.com> - 2017-08-08 12:20 +0200
    Re: [PATCH] IB/core: fix memory leak on ah on error return path Johannes Thumshirn <jthumshirn@suse.de> - 2017-08-08 12:30 +0200
      Re: [PATCH] IB/core: fix memory leak on ah on error return path Colin Ian King <colin.king@canonical.com> - 2017-08-08 12:30 +0200
        Re: [PATCH] IB/core: fix memory leak on ah on error return path Johannes Thumshirn <jthumshirn@suse.de> - 2017-08-08 15:00 +0200
          RE: [PATCH] IB/core: fix memory leak on ah on error return path Parav Pandit <parav@mellanox.com> - 2017-08-08 21:50 +0200

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

From: Colin Ian King <colin.king@canonical.com>

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 <colin.king@canonical.com>
---
 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

[toc] | [next] | [standalone]

#1706296

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

[toc] | [prev] | [next] | [standalone]

#1706299

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
> 

[toc] | [prev] | [next] | [standalone]

#1706523

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

[toc] | [prev] | [next] | [standalone]

#1706848

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-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Johannes Thumshirn
> Sent: Tuesday, August 08, 2017 7:59 AM
> To: Colin Ian King <colin.king@canonical.com>
> Cc: Lijun Ou <oulijun@huawei.com>; Wei Hu <xavier.huwei@huawei.com>;
> Doug Ledford <dledford@redhat.com>; Sean Hefty <sean.hefty@intel.com>;
> Hal Rosenstock <hal.rosenstock@gmail.com>; linux-rdma@vger.kernel.org;
> kernel-janitors@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
> jthumshirn@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 majordomo@vger.kernel.org More majordomo info at
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
> rnel.org%2Fmajordomo-
> info.html&data=02%7C01%7Cparav%40mellanox.com%7C4b396976866648806
> a1008d4de5d512c%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63
> 6377939837218186&sdata=l9u%2Bc6N9E31L2I%2BRDBLNh7KuPi%2Fb49yAlbvw
> qZvGruk%3D&reserved=0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web