IB/srpt: Fix memory leak in srpt_add_one

PATCH Superseded Leon Romanovsky 2 participants 3 messages Oct 27, 2020 – Oct 28, 2020

Subsystems (3)

InfiniBand SCSI target SCSI RDMA protocol (SRP) target

lore

Share & tools

lore

Leon Romanovsky

author

2020-10-27 05:59 UTC #0 lore raw

PATCH 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:
v1:
* 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 | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

drivers/infiniband/ulp/srpt/ib_srpt.c +17 ~~-10~~

index 0065eb17ae36..2747a3af545f 100644
@@ -619,13 +619,7 @@ static int srpt_refresh_port(struct srpt_port *sport)
 	return 0;
 }
 
-/**
- * srpt_unregister_mad_agent - unregister MAD callback functions
- * @sdev: SRPT HCA pointer.
- *
- * 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 +627,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) {

@@ -644,6 +638,17 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev)
 	}
 }
 
+/**
+ * srpt_unregister_mad_agent - unregister MAD callback functions
+ * @sdev: SRPT HCA pointer.
+ *
+ * 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)
+{
+	__srpt_unregister_mad_agent(sdev, sdev->device->phys_port_cnt);
+}
+
 /**
  * srpt_alloc_ioctx - allocate a SRPT I/O context structure
  * @sdev: SRPT HCA pointer.

@@ -3185,7 +3190,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 +3203,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)

--
2.26.2

Bart Van Assche

maintainer

2020-10-28 03:45 UTC #1 lore raw

drivers/infiniband/ulp/srpt/ib_srpt.c static void srpt_unregister_mad_agent(struct srpt_device *sdev) · line 650

Quoted 10 lines · Leon Romanovsky

drivers/infiniband/ulp/srpt/ib_srpt.c +10 -0

+/**
+ * srpt_unregister_mad_agent - unregister MAD callback functions
+ * @sdev: SRPT HCA pointer.
+ *
+ * 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)
+{
+	__srpt_unregister_mad_agent(sdev, sdev->device->phys_port_cnt);
+}

As far as I can see with this patch applied srpt_unregister_mad_agent() has no callers. So please add an argument to srpt_unregister_mad_agent() instead of introducing __srpt_unregister_mad_agent().

Thanks,

Bart.

Leon Romanovsky

author

2020-10-28 05:18 UTC #2 lore raw

Quoted 15 lines · Bart Van Assche

drivers/infiniband/ulp/srpt/ib_srpt.c static void srpt_unregister_mad_agent(struct srpt_device *sdev) · line 650
Quoted 10 lines · Leon Romanovsky
drivers/infiniband/ulp/srpt/ib_srpt.c +10 -0
+/**
+ * srpt_unregister_mad_agent - unregister MAD callback functions
+ * @sdev: SRPT HCA pointer.
+ *
+ * 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)
+{
+	__srpt_unregister_mad_agent(sdev, sdev->device->phys_port_cnt);
+}
As far as I can see with this patch applied srpt_unregister_mad_agent() has no callers. So please add an argument to srpt_unregister_mad_agent() instead of introducing __srpt_unregister_mad_agent().

srpt_unregister_mad_agent() is called in srpt_remove_one(), but will change to get extra parameter.

Thanks

Quoted 4 lines

Thanks,
Bart.