* [PATCH rdma-next] IB/srpt: Fix memory leak in srpt_add_one
@ 2020-10-26 13:27 Leon Romanovsky
2020-10-27 2:22 ` Bart Van Assche
0 siblings, 1 reply; 3+ messages in thread
From: Leon Romanovsky @ 2020-10-26 13:27 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Maor Gottlieb, Bart Van Assche, linux-rdma,
Nicholas A. Bellinger, target-devel
From: Maor Gottlieb <maorg@nvidia.com>
In case srpt_refresh_port failed for the second port, then
we don't unregister the MAD agnet.
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>
---
drivers/infiniband/ulp/srpt/ib_srpt.c | 30 ++++++++++++++++++---------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0065eb17ae36..cfe38996ea91 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,10 @@ 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++) {
+ if (!port_cnt)
+ return;
+
+ for (i = 1; i <= port_cnt; i++) {
sport = &sdev->port[i - 1];
WARN_ON(sport->port != i);
if (sport->mad_agent) {
@@ -644,6 +641,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 +3193,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 +3206,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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH rdma-next] IB/srpt: Fix memory leak in srpt_add_one
2020-10-26 13:27 [PATCH rdma-next] IB/srpt: Fix memory leak in srpt_add_one Leon Romanovsky
@ 2020-10-27 2:22 ` Bart Van Assche
2020-10-27 5:34 ` Leon Romanovsky
0 siblings, 1 reply; 3+ messages in thread
From: Bart Van Assche @ 2020-10-27 2:22 UTC (permalink / raw)
To: Leon Romanovsky, Doug Ledford, Jason Gunthorpe
Cc: Maor Gottlieb, linux-rdma, Nicholas A. Bellinger, target-devel
On 10/26/20 6:27 AM, Leon Romanovsky wrote:
> From: Maor Gottlieb <maorg@nvidia.com>
>
> In case srpt_refresh_port failed for the second port, then
> we don't unregister the MAD agnet.
^^^^^
agent?
The commit message is incomplete. Why does this patch have a Fixes tag?
The commit message should explain this but doesn't explain this.
What does this patch actually change? ib_unregister_mad_agent() is only
called by the current code if sport->mad_agent != NULL.
> -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,10 @@ 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++) {
> + if (!port_cnt)
> + return;
> +
> + for (i = 1; i <= port_cnt; i++) {
> sport = &sdev->port[i - 1];
> WARN_ON(sport->port != i);
> if (sport->mad_agent) {
If this patch is retained, please leave the if-test out if you agree
that it is not necessary. I'm concerned that it will confuse readers.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH rdma-next] IB/srpt: Fix memory leak in srpt_add_one
2020-10-27 2:22 ` Bart Van Assche
@ 2020-10-27 5:34 ` Leon Romanovsky
0 siblings, 0 replies; 3+ messages in thread
From: Leon Romanovsky @ 2020-10-27 5:34 UTC (permalink / raw)
To: Bart Van Assche
Cc: Doug Ledford, Jason Gunthorpe, Maor Gottlieb, linux-rdma,
Nicholas A. Bellinger, target-devel
On Mon, Oct 26, 2020 at 07:22:07PM -0700, Bart Van Assche wrote:
> On 10/26/20 6:27 AM, Leon Romanovsky wrote:
> > From: Maor Gottlieb <maorg@nvidia.com>
> >
> > In case srpt_refresh_port failed for the second port, then
> > we don't unregister the MAD agnet.
> ^^^^^
> agent?
>
> The commit message is incomplete. Why does this patch have a Fixes tag?
> The commit message should explain this but doesn't explain this.
>
> What does this patch actually change? ib_unregister_mad_agent() is only
> called by the current code if sport->mad_agent != NULL.
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.
This is what is written in the commit message.
>
> > -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,10 @@ 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++) {
> > + if (!port_cnt)
> > + return;
> > +
> > + for (i = 1; i <= port_cnt; i++) {
> > sport = &sdev->port[i - 1];
> > WARN_ON(sport->port != i);
> > if (sport->mad_agent) {
>
> If this patch is retained, please leave the if-test out if you agree
> that it is not necessary. I'm concerned that it will confuse readers.
No problem.
>
> Thanks,
>
> Bart.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-27 5:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26 13:27 [PATCH rdma-next] IB/srpt: Fix memory leak in srpt_add_one Leon Romanovsky
2020-10-27 2:22 ` Bart Van Assche
2020-10-27 5:34 ` Leon Romanovsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).