Re: [PATCH 1/4] RDMA/cm: Fix memory leak in cm_add/remove_one

[Leon Romanovsky <leon@kernel.org>]

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
>