Skip to contentSkip to conversation

IB/srpt: Fix memory leak in srpt_add_one

PATCH Applied Leon Romanovsky 3 participants 3 messages Oct 28, 2020 – Nov 2, 2020

Subsystems (3)
InfiniBand SCSI target SCSI RDMA protocol (SRP) target
lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

PATCH v2 1/1 IB/srpt: Fix memory leak in srpt_add_one

From: Maor Gottlieb <[email protected]>

Failure in srpt_refresh_port() for the second port will leave MAD registered for the first one, however, the srpt_add_one() will be marked as "failed" and SRPT will leak resources for that registered but not used and released first port.

Unregister the MAD agent for all ports in case of failure.

Fixes a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1")
Signed-off-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>

---
Changelog:
v2:
* Added an extra parameter to srpt_unregister_mad_agent() to eliminate an extra obfuscation call.
v1:
https://lore.kernel.org/linux-rdma/[email protected]
* Fixed and updated commit message.
* Remove port_cnt check from __srpt_unregister_mad_agent().
v0:
https://lore.kernel.org/linux-rdma/[email protected]

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
drivers/infiniband/ulp/srpt/ib_srpt.c +8 -5
index 0065eb17ae36..1b096305de1a 100644
@@ -622,10 +622,11 @@ static int srpt_refresh_port(struct srpt_port *sport)
 /**
  * srpt_unregister_mad_agent - unregister MAD callback functions
  * @sdev: SRPT HCA pointer.
+ * #port_cnt: number of ports with registered MAD
  *
  * Note: It is safe to call this function more than once for the same device.
  */
-static void srpt_unregister_mad_agent(struct srpt_device *sdev)
+static void srpt_unregister_mad_agent(struct srpt_device *sdev, int port_cnt)
 {
 	struct ib_port_modify port_modify = {
 		.clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP,

@@ -633,7 +634,7 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev)
 	struct srpt_port *sport;
 	int i;
 
-	for (i = 1; i <= sdev->device->phys_port_cnt; i++) {
+	for (i = 1; i <= port_cnt; i++) {
 		sport = &sdev->port[i - 1];
 		WARN_ON(sport->port != i);
 		if (sport->mad_agent) {

@@ -3185,7 +3186,8 @@ static int srpt_add_one(struct ib_device *device)
 		if (ret) {
 			pr_err("MAD registration failed for %s-%d.\n",
 			       dev_name(&sdev->device->dev), i);
-			goto err_event;
+			i--;
+			goto err_port;
 		}
 	}
 

@@ -3197,7 +3199,8 @@ static int srpt_add_one(struct ib_device *device)
 	pr_debug("added %s.\n", dev_name(&device->dev));
 	return 0;
 
-err_event:
+err_port:
+	srpt_unregister_mad_agent(sdev, i);
 	ib_unregister_event_handler(&sdev->event_handler);
 err_cm:
 	if (sdev->cm_id)

@@ -3221,7 +3224,7 @@ static void srpt_remove_one(struct ib_device *device, void *client_data)
 	struct srpt_device *sdev = client_data;
 	int i;
 
-	srpt_unregister_mad_agent(sdev);
+	srpt_unregister_mad_agent(sdev, sdev->device->phys_port_cnt);
 
 	ib_unregister_event_handler(&sdev->event_handler);

--
2.28.0

maintainer Reviewed-by on v2 1/1
#1 lore raw
Quoted 6 lines · Leon Romanovsky

Failure in srpt_refresh_port() for the second port will leave MAD registered for the first one, however, the srpt_add_one() will be marked as "failed" and SRPT will leak resources for that registered but not used and released first port.

Unregister the MAD agent for all ports in case of failure.

Reviewed-by Bart Van Assche <[email protected]>
maintainer Applied
#2 lore raw
In reply to Leon Romanovsky
Quoted 24 lines · Leon Romanovsky

From: Maor Gottlieb <[email protected]>

Failure in srpt_refresh_port() for the second port will leave MAD registered for the first one, however, the srpt_add_one() will be marked as "failed" and SRPT will leak resources for that registered but not used and released first port.

Unregister the MAD agent for all ports in case of failure.

Fixes a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1")
Signed-off-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
Reviewed-by Bart Van Assche <[email protected]>

---
Changelog:
v2:
* Added an extra parameter to srpt_unregister_mad_agent() to eliminate an extra obfuscation call.
v1:
https://lore.kernel.org/linux-rdma/[email protected]
* Fixed and updated commit message.
* Remove port_cnt check from __srpt_unregister_mad_agent().
v0:
https://lore.kernel.org/linux-rdma/[email protected]

Quoted 3 lines
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Applied to for-rc, thanks

Jason