[1/4] RDMA/cm: Fix memory leak in cm_add/remove_one
diff mbox series

Message ID 20190916071154.20383-2-leon@kernel.org
State Accepted
Delegated to: Jason Gunthorpe
Headers show
Series
  • Random fixes to IB/core
Related show

Commit Message

Leon Romanovsky Sept. 16, 2019, 7:11 a.m. UTC
From: Jack Morgenstein <jackm@dev.mellanox.co.il>

In the process of moving the debug counters sysfs entries, the commit
mentioned below eliminated the cm_infiniband sysfs directory.

This sysfs directory was tied to the cm_port object allocated in procedure
cm_add_one().

Before the commit below, this cm_port object was freed via a call to
kobject_put(port->kobj) in procedure cm_remove_port_fs().

Since port no longer uses its kobj, kobject_put(port->kobj) was eliminated.
This, however, meant that kfree was never called for the cm_port buffers.

Fix this by adding explicit kfree(port) calls to functions cm_add_one()
and cm_remove_one().

Note: the kfree call in the first chunk below (in the cm_add_one error
flow) fixes an old, undetected memory leak.

Fixes: c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")
Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/cm.c | 3 +++
 1 file changed, 3 insertions(+)

--
2.20.1

Comments

Jason Gunthorpe Sept. 16, 2019, 6:45 p.m. UTC | #1
On Mon, Sep 16, 2019 at 10:11:51AM +0300, Leon Romanovsky wrote:
> From: Jack Morgenstein <jackm@dev.mellanox.co.il>
> 
> In the process of moving the debug counters sysfs entries, the commit
> mentioned below eliminated the cm_infiniband sysfs directory.
> 
> This sysfs directory was tied to the cm_port object allocated in procedure
> cm_add_one().
> 
> Before the commit below, this cm_port object was freed via a call to
> kobject_put(port->kobj) in procedure cm_remove_port_fs().
> 
> Since port no longer uses its kobj, kobject_put(port->kobj) was eliminated.
> This, however, meant that kfree was never called for the cm_port buffers.
> 
> Fix this by adding explicit kfree(port) calls to functions cm_add_one()
> and cm_remove_one().
> 
> Note: the kfree call in the first chunk below (in the cm_add_one error
> flow) fixes an old, undetected memory leak.
> 
> Fixes: c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")
> Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>  drivers/infiniband/core/cm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index da10e6ccb43c..5920c0085d35 100644
> +++ b/drivers/infiniband/core/cm.c
> @@ -4399,6 +4399,7 @@ static void cm_add_one(struct ib_device *ib_device)
>  error1:
>  	port_modify.set_port_cap_mask = 0;
>  	port_modify.clr_port_cap_mask = IB_PORT_CM_SUP;
> +	kfree(port);
>  	while (--i) {
>  		if (!rdma_cap_ib_cm(ib_device, i))
>  			continue;
> @@ -4407,6 +4408,7 @@ static void cm_add_one(struct ib_device *ib_device)
>  		ib_modify_port(ib_device, port->port_num, 0, &port_modify);
>  		ib_unregister_mad_agent(port->mad_agent);
>  		cm_remove_port_fs(port);
> +		kfree(port);
>  	}
>  free:
>  	kfree(cm_dev);
> @@ -4460,6 +4462,7 @@ static void cm_remove_one(struct ib_device *ib_device, void *client_data)
>  		spin_unlock_irq(&cm.state_lock);
>  		ib_unregister_mad_agent(cur_mad_agent);
>  		cm_remove_port_fs(port);
> +		kfree(port);
>  	}

This whole thing is looking pretty goofy now, and I suspect there are
more error unwind bugs here.

How about this instead:

From e8dad20c7b69436e63b18f16cd9457ea27da5bc1 Mon Sep 17 00:00:00 2001
From: Jack Morgenstein <jackm@dev.mellanox.co.il>
Date: Mon, 16 Sep 2019 10:11:51 +0300
Subject: [PATCH] RDMA/cm: Fix memory leak in cm_add/remove_one

In the process of moving the debug counters sysfs entries, the commit
mentioned below eliminated the cm_infiniband sysfs directory, and created
some missing cases where the port pointers were not being freed as the
kobject_put was also eliminated.

Rework this to not allocate port pointers and consolidate all the error
unwind into one sequence.

This also fixes unlikely racey bugs where error-unwind after unregistering
the MAD handler would miss flushing the WQ and other clean up that is
necessary once concurrency starts.

Fixes: c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/infiniband/core/cm.c | 187 ++++++++++++++++++-----------------
 1 file changed, 94 insertions(+), 93 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index da10e6ccb43cd0..30a764e763dec1 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -223,7 +223,7 @@ struct cm_device {
 	struct ib_device *ib_device;
 	u8 ack_delay;
 	int going_down;
-	struct cm_port *port[0];
+	struct cm_port port[];
 };
 
 struct cm_av {
@@ -520,7 +520,7 @@ get_cm_port_from_path(struct sa_path_rec *path, const struct ib_gid_attr *attr)
 		read_lock_irqsave(&cm.device_lock, flags);
 		list_for_each_entry(cm_dev, &cm.device_list, list) {
 			if (cm_dev->ib_device == attr->device) {
-				port = cm_dev->port[attr->port_num - 1];
+				port = &cm_dev->port[attr->port_num - 1];
 				break;
 			}
 		}
@@ -539,7 +539,7 @@ get_cm_port_from_path(struct sa_path_rec *path, const struct ib_gid_attr *attr)
 					     sa_conv_pathrec_to_gid_type(path),
 					     NULL);
 			if (!IS_ERR(attr)) {
-				port = cm_dev->port[attr->port_num - 1];
+				port = &cm_dev->port[attr->port_num - 1];
 				break;
 			}
 		}
@@ -4319,23 +4319,99 @@ static void cm_remove_port_fs(struct cm_port *port)
 
 }
 
-static void cm_add_one(struct ib_device *ib_device)
+static void cm_destroy_one_port(struct cm_device *cm_dev, unsigned int port_num)
 {
-	struct cm_device *cm_dev;
-	struct cm_port *port;
+	struct cm_port *port = &cm_dev->port[port_num - 1];
+	struct ib_port_modify port_modify = {
+		.clr_port_cap_mask = IB_PORT_CM_SUP
+	};
+	struct ib_mad_agent *cur_mad_agent;
+	struct cm_id_private *cm_id_priv;
+
+	if (!rdma_cap_ib_cm(cm_dev->ib_device, port_num))
+		return;
+
+	ib_modify_port(cm_dev->ib_device, port_num, 0, &port_modify);
+
+	/* Mark all the cm_id's as not valid */
+	spin_lock_irq(&cm.lock);
+	list_for_each_entry (cm_id_priv, &port->cm_priv_altr_list, altr_list)
+		cm_id_priv->altr_send_port_not_ready = 1;
+	list_for_each_entry (cm_id_priv, &port->cm_priv_prim_list, prim_list)
+		cm_id_priv->prim_send_port_not_ready = 1;
+	spin_unlock_irq(&cm.lock);
+
+	/*
+	 * We flush the queue here after the going_down set, this verifies
+	 * that no new works will be queued in the recv handler, after that we
+	 * can call the unregister_mad_agent
+	 */
+	flush_workqueue(cm.wq);
+
+	spin_lock_irq(&cm.state_lock);
+	cur_mad_agent = port->mad_agent;
+	port->mad_agent = NULL;
+	spin_unlock_irq(&cm.state_lock);
+
+	if (cur_mad_agent)
+		ib_unregister_mad_agent(cur_mad_agent);
+
+	cm_remove_port_fs(port);
+}
+
+static int cm_init_one_port(struct cm_device *cm_dev, unsigned int port_num)
+{
+	struct cm_port *port = &cm_dev->port[port_num - 1];
 	struct ib_mad_reg_req reg_req = {
 		.mgmt_class = IB_MGMT_CLASS_CM,
 		.mgmt_class_version = IB_CM_CLASS_VERSION,
 	};
 	struct ib_port_modify port_modify = {
-		.set_port_cap_mask = IB_PORT_CM_SUP
+		.set_port_cap_mask = IB_PORT_CM_SUP,
 	};
-	unsigned long flags;
 	int ret;
+
+	if (!rdma_cap_ib_cm(cm_dev->ib_device, port_num))
+		return 0;
+
+	set_bit(IB_MGMT_METHOD_SEND, reg_req.method_mask);
+
+	port->cm_dev = cm_dev;
+	port->port_num = port_num;
+
+	INIT_LIST_HEAD(&port->cm_priv_prim_list);
+	INIT_LIST_HEAD(&port->cm_priv_altr_list);
+
+	ret = cm_create_port_fs(port);
+	if (ret)
+		return ret;
+
+	port->mad_agent =
+		ib_register_mad_agent(cm_dev->ib_device, port_num, IB_QPT_GSI,
+				      &reg_req, 0, cm_send_handler,
+				      cm_recv_handler, port, 0);
+	if (IS_ERR(port->mad_agent)) {
+		cm_destroy_one_port(cm_dev, port_num);
+		return PTR_ERR(port->mad_agent);
+	}
+
+	ret = ib_modify_port(cm_dev->ib_device, port_num, 0, &port_modify);
+	if (ret) {
+		cm_destroy_one_port(cm_dev, port_num);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void cm_add_one(struct ib_device *ib_device)
+{
+	struct cm_device *cm_dev;
+	unsigned long flags;
 	int count = 0;
 	u8 i;
 
-	cm_dev = kzalloc(struct_size(cm_dev, port, ib_device->phys_port_cnt),
+	cm_dev = kvzalloc(struct_size(cm_dev, port, ib_device->phys_port_cnt),
 			 GFP_KERNEL);
 	if (!cm_dev)
 		return;
@@ -4344,41 +4420,9 @@ static void cm_add_one(struct ib_device *ib_device)
 	cm_dev->ack_delay = ib_device->attrs.local_ca_ack_delay;
 	cm_dev->going_down = 0;
 
-	set_bit(IB_MGMT_METHOD_SEND, reg_req.method_mask);
 	for (i = 1; i <= ib_device->phys_port_cnt; i++) {
-		if (!rdma_cap_ib_cm(ib_device, i))
-			continue;
-
-		port = kzalloc(sizeof *port, GFP_KERNEL);
-		if (!port)
-			goto error1;
-
-		cm_dev->port[i-1] = port;
-		port->cm_dev = cm_dev;
-		port->port_num = i;
-
-		INIT_LIST_HEAD(&port->cm_priv_prim_list);
-		INIT_LIST_HEAD(&port->cm_priv_altr_list);
-
-		ret = cm_create_port_fs(port);
-		if (ret)
-			goto error1;
-
-		port->mad_agent = ib_register_mad_agent(ib_device, i,
-							IB_QPT_GSI,
-							&reg_req,
-							0,
-							cm_send_handler,
-							cm_recv_handler,
-							port,
-							0);
-		if (IS_ERR(port->mad_agent))
-			goto error2;
-
-		ret = ib_modify_port(ib_device, i, 0, &port_modify);
-		if (ret)
-			goto error3;
-
+		if (!cm_init_one_port(cm_dev, i))
+			goto error;
 		count++;
 	}
 
@@ -4392,35 +4436,16 @@ static void cm_add_one(struct ib_device *ib_device)
 	write_unlock_irqrestore(&cm.device_lock, flags);
 	return;
 
-error3:
-	ib_unregister_mad_agent(port->mad_agent);
-error2:
-	cm_remove_port_fs(port);
-error1:
-	port_modify.set_port_cap_mask = 0;
-	port_modify.clr_port_cap_mask = IB_PORT_CM_SUP;
-	while (--i) {
-		if (!rdma_cap_ib_cm(ib_device, i))
-			continue;
-
-		port = cm_dev->port[i-1];
-		ib_modify_port(ib_device, port->port_num, 0, &port_modify);
-		ib_unregister_mad_agent(port->mad_agent);
-		cm_remove_port_fs(port);
-	}
+error:
+	while (--i)
+		cm_destroy_one_port(cm_dev, i);
 free:
-	kfree(cm_dev);
+	kvfree(cm_dev);
 }
 
 static void cm_remove_one(struct ib_device *ib_device, void *client_data)
 {
 	struct cm_device *cm_dev = client_data;
-	struct cm_port *port;
-	struct cm_id_private *cm_id_priv;
-	struct ib_mad_agent *cur_mad_agent;
-	struct ib_port_modify port_modify = {
-		.clr_port_cap_mask = IB_PORT_CM_SUP
-	};
 	unsigned long flags;
 	int i;
 
@@ -4435,34 +4460,10 @@ static void cm_remove_one(struct ib_device *ib_device, void *client_data)
 	cm_dev->going_down = 1;
 	spin_unlock_irq(&cm.lock);
 
-	for (i = 1; i <= ib_device->phys_port_cnt; i++) {
-		if (!rdma_cap_ib_cm(ib_device, i))
-			continue;
-
-		port = cm_dev->port[i-1];
-		ib_modify_port(ib_device, port->port_num, 0, &port_modify);
-		/* Mark all the cm_id's as not valid */
-		spin_lock_irq(&cm.lock);
-		list_for_each_entry(cm_id_priv, &port->cm_priv_altr_list, altr_list)
-			cm_id_priv->altr_send_port_not_ready = 1;
-		list_for_each_entry(cm_id_priv, &port->cm_priv_prim_list, prim_list)
-			cm_id_priv->prim_send_port_not_ready = 1;
-		spin_unlock_irq(&cm.lock);
-		/*
-		 * We flush the queue here after the going_down set, this
-		 * verify that no new works will be queued in the recv handler,
-		 * after that we can call the unregister_mad_agent
-		 */
-		flush_workqueue(cm.wq);
-		spin_lock_irq(&cm.state_lock);
-		cur_mad_agent = port->mad_agent;
-		port->mad_agent = NULL;
-		spin_unlock_irq(&cm.state_lock);
-		ib_unregister_mad_agent(cur_mad_agent);
-		cm_remove_port_fs(port);
-	}
+	for (i = 1; i <= ib_device->phys_port_cnt; i++)
+		cm_destroy_one_port(cm_dev, i);
 
-	kfree(cm_dev);
+	kvfree(cm_dev);
 }
 
 static int __init ib_cm_init(void)
Leon Romanovsky Sept. 18, 2019, 11:36 a.m. UTC | #2
On Mon, Sep 16, 2019 at 06:45:48PM +0000, Jason Gunthorpe wrote:
> On Mon, Sep 16, 2019 at 10:11:51AM +0300, Leon Romanovsky wrote:
> > From: Jack Morgenstein <jackm@dev.mellanox.co.il>
> >
> > In the process of moving the debug counters sysfs entries, the commit
> > mentioned below eliminated the cm_infiniband sysfs directory.
> >
> > This sysfs directory was tied to the cm_port object allocated in procedure
> > cm_add_one().
> >
> > Before the commit below, this cm_port object was freed via a call to
> > kobject_put(port->kobj) in procedure cm_remove_port_fs().
> >
> > Since port no longer uses its kobj, kobject_put(port->kobj) was eliminated.
> > This, however, meant that kfree was never called for the cm_port buffers.
> >
> > Fix this by adding explicit kfree(port) calls to functions cm_add_one()
> > and cm_remove_one().
> >
> > Note: the kfree call in the first chunk below (in the cm_add_one error
> > flow) fixes an old, undetected memory leak.
> >
> > Fixes: c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")
> > Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/core/cm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > index da10e6ccb43c..5920c0085d35 100644
> > +++ b/drivers/infiniband/core/cm.c
> > @@ -4399,6 +4399,7 @@ static void cm_add_one(struct ib_device *ib_device)
> >  error1:
> >  	port_modify.set_port_cap_mask = 0;
> >  	port_modify.clr_port_cap_mask = IB_PORT_CM_SUP;
> > +	kfree(port);
> >  	while (--i) {
> >  		if (!rdma_cap_ib_cm(ib_device, i))
> >  			continue;
> > @@ -4407,6 +4408,7 @@ static void cm_add_one(struct ib_device *ib_device)
> >  		ib_modify_port(ib_device, port->port_num, 0, &port_modify);
> >  		ib_unregister_mad_agent(port->mad_agent);
> >  		cm_remove_port_fs(port);
> > +		kfree(port);
> >  	}
> >  free:
> >  	kfree(cm_dev);
> > @@ -4460,6 +4462,7 @@ static void cm_remove_one(struct ib_device *ib_device, void *client_data)
> >  		spin_unlock_irq(&cm.state_lock);
> >  		ib_unregister_mad_agent(cur_mad_agent);
> >  		cm_remove_port_fs(port);
> > +		kfree(port);
> >  	}
>
> This whole thing is looking pretty goofy now, and I suspect there are
> more error unwind bugs here.
>
> How about this instead:

It looks OK to me.

Thanks

>
> From e8dad20c7b69436e63b18f16cd9457ea27da5bc1 Mon Sep 17 00:00:00 2001
> From: Jack Morgenstein <jackm@dev.mellanox.co.il>
> Date: Mon, 16 Sep 2019 10:11:51 +0300
> Subject: [PATCH] RDMA/cm: Fix memory leak in cm_add/remove_one
>
> In the process of moving the debug counters sysfs entries, the commit
> mentioned below eliminated the cm_infiniband sysfs directory, and created
> some missing cases where the port pointers were not being freed as the
> kobject_put was also eliminated.
>
> Rework this to not allocate port pointers and consolidate all the error
> unwind into one sequence.
>
> This also fixes unlikely racey bugs where error-unwind after unregistering
> the MAD handler would miss flushing the WQ and other clean up that is
> necessary once concurrency starts.
>
> Fixes: c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>  drivers/infiniband/core/cm.c | 187 ++++++++++++++++++-----------------
>  1 file changed, 94 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index da10e6ccb43cd0..30a764e763dec1 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -223,7 +223,7 @@ struct cm_device {
>  	struct ib_device *ib_device;
>  	u8 ack_delay;
>  	int going_down;
> -	struct cm_port *port[0];
> +	struct cm_port port[];
>  };
>
>  struct cm_av {
> @@ -520,7 +520,7 @@ get_cm_port_from_path(struct sa_path_rec *path, const struct ib_gid_attr *attr)
>  		read_lock_irqsave(&cm.device_lock, flags);
>  		list_for_each_entry(cm_dev, &cm.device_list, list) {
>  			if (cm_dev->ib_device == attr->device) {
> -				port = cm_dev->port[attr->port_num - 1];
> +				port = &cm_dev->port[attr->port_num - 1];
>  				break;
>  			}
>  		}
> @@ -539,7 +539,7 @@ get_cm_port_from_path(struct sa_path_rec *path, const struct ib_gid_attr *attr)
>  					     sa_conv_pathrec_to_gid_type(path),
>  					     NULL);
>  			if (!IS_ERR(attr)) {
> -				port = cm_dev->port[attr->port_num - 1];
> +				port = &cm_dev->port[attr->port_num - 1];
>  				break;
>  			}
>  		}
> @@ -4319,23 +4319,99 @@ static void cm_remove_port_fs(struct cm_port *port)
>
>  }
>
> -static void cm_add_one(struct ib_device *ib_device)
> +static void cm_destroy_one_port(struct cm_device *cm_dev, unsigned int port_num)
>  {
> -	struct cm_device *cm_dev;
> -	struct cm_port *port;
> +	struct cm_port *port = &cm_dev->port[port_num - 1];
> +	struct ib_port_modify port_modify = {
> +		.clr_port_cap_mask = IB_PORT_CM_SUP
> +	};
> +	struct ib_mad_agent *cur_mad_agent;
> +	struct cm_id_private *cm_id_priv;
> +
> +	if (!rdma_cap_ib_cm(cm_dev->ib_device, port_num))
> +		return;
> +
> +	ib_modify_port(cm_dev->ib_device, port_num, 0, &port_modify);
> +
> +	/* Mark all the cm_id's as not valid */
> +	spin_lock_irq(&cm.lock);
> +	list_for_each_entry (cm_id_priv, &port->cm_priv_altr_list, altr_list)
> +		cm_id_priv->altr_send_port_not_ready = 1;
> +	list_for_each_entry (cm_id_priv, &port->cm_priv_prim_list, prim_list)
> +		cm_id_priv->prim_send_port_not_ready = 1;
> +	spin_unlock_irq(&cm.lock);
> +
> +	/*
> +	 * We flush the queue here after the going_down set, this verifies
> +	 * that no new works will be queued in the recv handler, after that we
> +	 * can call the unregister_mad_agent
> +	 */
> +	flush_workqueue(cm.wq);
> +
> +	spin_lock_irq(&cm.state_lock);
> +	cur_mad_agent = port->mad_agent;
> +	port->mad_agent = NULL;
> +	spin_unlock_irq(&cm.state_lock);
> +
> +	if (cur_mad_agent)
> +		ib_unregister_mad_agent(cur_mad_agent);
> +
> +	cm_remove_port_fs(port);
> +}
> +
> +static int cm_init_one_port(struct cm_device *cm_dev, unsigned int port_num)
> +{
> +	struct cm_port *port = &cm_dev->port[port_num - 1];
>  	struct ib_mad_reg_req reg_req = {
>  		.mgmt_class = IB_MGMT_CLASS_CM,
>  		.mgmt_class_version = IB_CM_CLASS_VERSION,
>  	};
>  	struct ib_port_modify port_modify = {
> -		.set_port_cap_mask = IB_PORT_CM_SUP
> +		.set_port_cap_mask = IB_PORT_CM_SUP,
>  	};
> -	unsigned long flags;
>  	int ret;
> +
> +	if (!rdma_cap_ib_cm(cm_dev->ib_device, port_num))
> +		return 0;
> +
> +	set_bit(IB_MGMT_METHOD_SEND, reg_req.method_mask);
> +
> +	port->cm_dev = cm_dev;
> +	port->port_num = port_num;
> +
> +	INIT_LIST_HEAD(&port->cm_priv_prim_list);
> +	INIT_LIST_HEAD(&port->cm_priv_altr_list);
> +
> +	ret = cm_create_port_fs(port);
> +	if (ret)
> +		return ret;
> +
> +	port->mad_agent =
> +		ib_register_mad_agent(cm_dev->ib_device, port_num, IB_QPT_GSI,
> +				      &reg_req, 0, cm_send_handler,
> +				      cm_recv_handler, port, 0);
> +	if (IS_ERR(port->mad_agent)) {
> +		cm_destroy_one_port(cm_dev, port_num);
> +		return PTR_ERR(port->mad_agent);
> +	}
> +
> +	ret = ib_modify_port(cm_dev->ib_device, port_num, 0, &port_modify);
> +	if (ret) {
> +		cm_destroy_one_port(cm_dev, port_num);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void cm_add_one(struct ib_device *ib_device)
> +{
> +	struct cm_device *cm_dev;
> +	unsigned long flags;
>  	int count = 0;
>  	u8 i;
>
> -	cm_dev = kzalloc(struct_size(cm_dev, port, ib_device->phys_port_cnt),
> +	cm_dev = kvzalloc(struct_size(cm_dev, port, ib_device->phys_port_cnt),
>  			 GFP_KERNEL);
>  	if (!cm_dev)
>  		return;
> @@ -4344,41 +4420,9 @@ static void cm_add_one(struct ib_device *ib_device)
>  	cm_dev->ack_delay = ib_device->attrs.local_ca_ack_delay;
>  	cm_dev->going_down = 0;
>
> -	set_bit(IB_MGMT_METHOD_SEND, reg_req.method_mask);
>  	for (i = 1; i <= ib_device->phys_port_cnt; i++) {
> -		if (!rdma_cap_ib_cm(ib_device, i))
> -			continue;
> -
> -		port = kzalloc(sizeof *port, GFP_KERNEL);
> -		if (!port)
> -			goto error1;
> -
> -		cm_dev->port[i-1] = port;
> -		port->cm_dev = cm_dev;
> -		port->port_num = i;
> -
> -		INIT_LIST_HEAD(&port->cm_priv_prim_list);
> -		INIT_LIST_HEAD(&port->cm_priv_altr_list);
> -
> -		ret = cm_create_port_fs(port);
> -		if (ret)
> -			goto error1;
> -
> -		port->mad_agent = ib_register_mad_agent(ib_device, i,
> -							IB_QPT_GSI,
> -							&reg_req,
> -							0,
> -							cm_send_handler,
> -							cm_recv_handler,
> -							port,
> -							0);
> -		if (IS_ERR(port->mad_agent))
> -			goto error2;
> -
> -		ret = ib_modify_port(ib_device, i, 0, &port_modify);
> -		if (ret)
> -			goto error3;
> -
> +		if (!cm_init_one_port(cm_dev, i))
> +			goto error;
>  		count++;
>  	}
>
> @@ -4392,35 +4436,16 @@ static void cm_add_one(struct ib_device *ib_device)
>  	write_unlock_irqrestore(&cm.device_lock, flags);
>  	return;
>
> -error3:
> -	ib_unregister_mad_agent(port->mad_agent);
> -error2:
> -	cm_remove_port_fs(port);
> -error1:
> -	port_modify.set_port_cap_mask = 0;
> -	port_modify.clr_port_cap_mask = IB_PORT_CM_SUP;
> -	while (--i) {
> -		if (!rdma_cap_ib_cm(ib_device, i))
> -			continue;
> -
> -		port = cm_dev->port[i-1];
> -		ib_modify_port(ib_device, port->port_num, 0, &port_modify);
> -		ib_unregister_mad_agent(port->mad_agent);
> -		cm_remove_port_fs(port);
> -	}
> +error:
> +	while (--i)
> +		cm_destroy_one_port(cm_dev, i);
>  free:
> -	kfree(cm_dev);
> +	kvfree(cm_dev);
>  }
>
>  static void cm_remove_one(struct ib_device *ib_device, void *client_data)
>  {
>  	struct cm_device *cm_dev = client_data;
> -	struct cm_port *port;
> -	struct cm_id_private *cm_id_priv;
> -	struct ib_mad_agent *cur_mad_agent;
> -	struct ib_port_modify port_modify = {
> -		.clr_port_cap_mask = IB_PORT_CM_SUP
> -	};
>  	unsigned long flags;
>  	int i;
>
> @@ -4435,34 +4460,10 @@ static void cm_remove_one(struct ib_device *ib_device, void *client_data)
>  	cm_dev->going_down = 1;
>  	spin_unlock_irq(&cm.lock);
>
> -	for (i = 1; i <= ib_device->phys_port_cnt; i++) {
> -		if (!rdma_cap_ib_cm(ib_device, i))
> -			continue;
> -
> -		port = cm_dev->port[i-1];
> -		ib_modify_port(ib_device, port->port_num, 0, &port_modify);
> -		/* Mark all the cm_id's as not valid */
> -		spin_lock_irq(&cm.lock);
> -		list_for_each_entry(cm_id_priv, &port->cm_priv_altr_list, altr_list)
> -			cm_id_priv->altr_send_port_not_ready = 1;
> -		list_for_each_entry(cm_id_priv, &port->cm_priv_prim_list, prim_list)
> -			cm_id_priv->prim_send_port_not_ready = 1;
> -		spin_unlock_irq(&cm.lock);
> -		/*
> -		 * We flush the queue here after the going_down set, this
> -		 * verify that no new works will be queued in the recv handler,
> -		 * after that we can call the unregister_mad_agent
> -		 */
> -		flush_workqueue(cm.wq);
> -		spin_lock_irq(&cm.state_lock);
> -		cur_mad_agent = port->mad_agent;
> -		port->mad_agent = NULL;
> -		spin_unlock_irq(&cm.state_lock);
> -		ib_unregister_mad_agent(cur_mad_agent);
> -		cm_remove_port_fs(port);
> -	}
> +	for (i = 1; i <= ib_device->phys_port_cnt; i++)
> +		cm_destroy_one_port(cm_dev, i);
>
> -	kfree(cm_dev);
> +	kvfree(cm_dev);
>  }
>
>  static int __init ib_cm_init(void)
> --
> 2.23.0
>
Leon Romanovsky Sept. 25, 2019, 6:16 a.m. UTC | #3
On Mon, Sep 16, 2019 at 06:45:48PM +0000, Jason Gunthorpe wrote:
> On Mon, Sep 16, 2019 at 10:11:51AM +0300, Leon Romanovsky wrote:
> > From: Jack Morgenstein <jackm@dev.mellanox.co.il>
> >
> > In the process of moving the debug counters sysfs entries, the commit
> > mentioned below eliminated the cm_infiniband sysfs directory.
> >
> > This sysfs directory was tied to the cm_port object allocated in procedure
> > cm_add_one().
> >
> > Before the commit below, this cm_port object was freed via a call to
> > kobject_put(port->kobj) in procedure cm_remove_port_fs().
> >
> > Since port no longer uses its kobj, kobject_put(port->kobj) was eliminated.
> > This, however, meant that kfree was never called for the cm_port buffers.
> >
> > Fix this by adding explicit kfree(port) calls to functions cm_add_one()
> > and cm_remove_one().
> >
> > Note: the kfree call in the first chunk below (in the cm_add_one error
> > flow) fixes an old, undetected memory leak.
> >
> > Fixes: c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")
> > Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/core/cm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > index da10e6ccb43c..5920c0085d35 100644
> > +++ b/drivers/infiniband/core/cm.c
> > @@ -4399,6 +4399,7 @@ static void cm_add_one(struct ib_device *ib_device)
> >  error1:
> >  	port_modify.set_port_cap_mask = 0;
> >  	port_modify.clr_port_cap_mask = IB_PORT_CM_SUP;
> > +	kfree(port);
> >  	while (--i) {
> >  		if (!rdma_cap_ib_cm(ib_device, i))
> >  			continue;
> > @@ -4407,6 +4408,7 @@ static void cm_add_one(struct ib_device *ib_device)
> >  		ib_modify_port(ib_device, port->port_num, 0, &port_modify);
> >  		ib_unregister_mad_agent(port->mad_agent);
> >  		cm_remove_port_fs(port);
> > +		kfree(port);
> >  	}
> >  free:
> >  	kfree(cm_dev);
> > @@ -4460,6 +4462,7 @@ static void cm_remove_one(struct ib_device *ib_device, void *client_data)
> >  		spin_unlock_irq(&cm.state_lock);
> >  		ib_unregister_mad_agent(cur_mad_agent);
> >  		cm_remove_port_fs(port);
> > +		kfree(port);
> >  	}
>
> This whole thing is looking pretty goofy now, and I suspect there are
> more error unwind bugs here.
>
> How about this instead:
>
> From e8dad20c7b69436e63b18f16cd9457ea27da5bc1 Mon Sep 17 00:00:00 2001
> From: Jack Morgenstein <jackm@dev.mellanox.co.il>
> Date: Mon, 16 Sep 2019 10:11:51 +0300
> Subject: [PATCH] RDMA/cm: Fix memory leak in cm_add/remove_one
>
> In the process of moving the debug counters sysfs entries, the commit
> mentioned below eliminated the cm_infiniband sysfs directory, and created
> some missing cases where the port pointers were not being freed as the
> kobject_put was also eliminated.
>
> Rework this to not allocate port pointers and consolidate all the error
> unwind into one sequence.
>
> This also fixes unlikely racey bugs where error-unwind after unregistering
> the MAD handler would miss flushing the WQ and other clean up that is
> necessary once concurrency starts.
>
> Fixes: c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>  drivers/infiniband/core/cm.c | 187 ++++++++++++++++++-----------------
>  1 file changed, 94 insertions(+), 93 deletions(-)

The proposed patch doesn't pass sanity check and unfortunately, I don't
have time now to debug it.

$ ./rping -V -S 256 -C 100 -p 19663 -P -a 192.168.0.1 -s &
$ ./rping -V -S 256 -C 100 -p 19663 -a 192.168.0.1 -c -I 192.168.0.1
   rdma_connect: Invalid argument
   connect error -1
   <hang>

Thanks
Leon Romanovsky Oct. 2, 2019, 11:58 a.m. UTC | #4
On Mon, Sep 16, 2019 at 10:11:51AM +0300, Leon Romanovsky wrote:
> From: Jack Morgenstein <jackm@dev.mellanox.co.il>
>
> In the process of moving the debug counters sysfs entries, the commit
> mentioned below eliminated the cm_infiniband sysfs directory.
>
> This sysfs directory was tied to the cm_port object allocated in procedure
> cm_add_one().
>
> Before the commit below, this cm_port object was freed via a call to
> kobject_put(port->kobj) in procedure cm_remove_port_fs().
>
> Since port no longer uses its kobj, kobject_put(port->kobj) was eliminated.
> This, however, meant that kfree was never called for the cm_port buffers.
>
> Fix this by adding explicit kfree(port) calls to functions cm_add_one()
> and cm_remove_one().
>
> Note: the kfree call in the first chunk below (in the cm_add_one error
> flow) fixes an old, undetected memory leak.
>
> Fixes: c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")
> Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/core/cm.c | 3 +++
>  1 file changed, 3 insertions(+)
>

Can we take this patch till you will have time to debug better one?

Thanks
Jason Gunthorpe Oct. 4, 2019, 5:59 p.m. UTC | #5
On Mon, Sep 16, 2019 at 10:11:51AM +0300, Leon Romanovsky wrote:
> From: Jack Morgenstein <jackm@dev.mellanox.co.il>
> 
> In the process of moving the debug counters sysfs entries, the commit
> mentioned below eliminated the cm_infiniband sysfs directory.
> 
> This sysfs directory was tied to the cm_port object allocated in procedure
> cm_add_one().
> 
> Before the commit below, this cm_port object was freed via a call to
> kobject_put(port->kobj) in procedure cm_remove_port_fs().
> 
> Since port no longer uses its kobj, kobject_put(port->kobj) was eliminated.
> This, however, meant that kfree was never called for the cm_port buffers.
> 
> Fix this by adding explicit kfree(port) calls to functions cm_add_one()
> and cm_remove_one().
> 
> Note: the kfree call in the first chunk below (in the cm_add_one error
> flow) fixes an old, undetected memory leak.
> 
> Fixes: c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")
> Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>  drivers/infiniband/core/cm.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied to for-rc, thanks

Jason

Patch
diff mbox series

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index da10e6ccb43c..5920c0085d35 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -4399,6 +4399,7 @@  static void cm_add_one(struct ib_device *ib_device)
 error1:
 	port_modify.set_port_cap_mask = 0;
 	port_modify.clr_port_cap_mask = IB_PORT_CM_SUP;
+	kfree(port);
 	while (--i) {
 		if (!rdma_cap_ib_cm(ib_device, i))
 			continue;
@@ -4407,6 +4408,7 @@  static void cm_add_one(struct ib_device *ib_device)
 		ib_modify_port(ib_device, port->port_num, 0, &port_modify);
 		ib_unregister_mad_agent(port->mad_agent);
 		cm_remove_port_fs(port);
+		kfree(port);
 	}
 free:
 	kfree(cm_dev);
@@ -4460,6 +4462,7 @@  static void cm_remove_one(struct ib_device *ib_device, void *client_data)
 		spin_unlock_irq(&cm.state_lock);
 		ib_unregister_mad_agent(cur_mad_agent);
 		cm_remove_port_fs(port);
+		kfree(port);
 	}

 	kfree(cm_dev);