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

Commit Message

Colin King Aug. 8, 2017, 10:10 a.m. UTC

From: Colin Ian King <[e-mail]>

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 <[e-mail]>
---
 drivers/infiniband/hw/hns/hns_roce_ah.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Johannes Thumshirn Aug. 8, 2017, 10:20 a.m. UTC | #1

Shouldn't the subject start with "IB/hns:" given it's touching
drivers/infiniband/hw/hns/hns_roce_ah.c?

Byte,
	Johannes

Colin King Aug. 8, 2017, 10:28 a.m. UTC | #2

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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Johannes Thumshirn Aug. 8, 2017, 12:59 p.m. UTC | #3

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

Parav Pandit Aug. 8, 2017, 7:48 p.m. UTC | #4

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: [e-mail] [mailto:linux-rdma-
> [e-mail]] On Behalf Of Johannes Thumshirn
> Sent: Tuesday, August 08, 2017 7:59 AM
> To: Colin Ian King <[e-mail]>
> Cc: Lijun Ou <[e-mail]>; Wei Hu <[e-mail]>;
> Doug Ledford <[e-mail]>; Sean Hefty <[e-mail]>;
> Hal Rosenstock <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]
> 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
> [e-mail]                                [phone]
> 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 [e-mail] 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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Doug Ledford Aug. 16, 2017, 3:32 p.m. UTC | #5

On Tue, 2017-08-08 at 19:48 +0000, 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.

Patch

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);
 	}