[PATCH rdma-next v1] IB/srpt: Fix memory leak in srpt_add_one

[PATCH rdma-next v1] IB/srpt: Fix memory leak in srpt_add_one

[Leon Romanovsky]

From: Maor Gottlieb <maorg@nvidia.com>

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 <maorg@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
Changelog:
v1:
 * Fixed and updated commit message.
 * Remove port_cnt check from __srpt_unregister_mad_agent().
v0: https://lore.kernel.org/linux-rdma/20201026132737.1338171-1-leon@kernel.org

---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0065eb17ae36..2747a3af545f 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -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

Re: [PATCH rdma-next v1] IB/srpt: Fix memory leak in srpt_add_one

[Bart Van Assche]

On 10/26/20 10:59 PM, Leon Romanovsky wrote:
> +/**
> + * 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.

Re: [PATCH rdma-next v1] IB/srpt: Fix memory leak in srpt_add_one

[Leon Romanovsky]

On Tue, Oct 27, 2020 at 08:45:11PM -0700, Bart Van Assche wrote:
> On 10/26/20 10:59 PM, Leon Romanovsky wrote:
> > +/**
> > + * 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

>
> Thanks,
>
> Bart.