| Message ID | 20190916071154.20383-2-leon@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Random fixes to IB/core | expand |
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, + ®_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, - ®_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)
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, > + ®_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, > - ®_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 >
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
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
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
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);