Random fixes to IB/core
cover Random fixes to IB/core
From: Leon Romanovsky <[email protected]>
Hi,
This is a random set of various fixes in IB/core, which were in my submission queue before LPC>
Thanks
Jack Morgenstein (2):
RDMA/cm: Fix memory leak in cm_add/remove_one
RDMA: Fix double-free in srq creation error flow
Leon Romanovsky (1):
RDMA/nldev: Reshuffle the code to avoid need to rebind QP in error
path
Mark Zhang (1):
RDMA/counter: Prevent QP counter manual binding in auto mode
drivers/infiniband/core/cm.c | 3 +++ drivers/infiniband/core/counters.c | 12 +++++++++++- drivers/infiniband/core/nldev.c | 12 +++++------- drivers/infiniband/core/uverbs_cmd.c | 3 ++- 4 files changed, 21 insertions(+), 9 deletions(-)
--
2.20.1
PATCH 1/4 RDMA/cm: Fix memory leak in cm_add/remove_one
From: Jack Morgenstein <[email protected]>
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.
--- drivers/infiniband/core/cm.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/infiniband/core/cm.c +3 -0
@@ -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);
--
2.20.1
Re: [PATCH 1/4] RDMA/cm: Fix memory leak in cm_add/remove_one
Quoted 46 lines · Leon Romanovsky
From: Jack Morgenstein <[email protected]>
In the process of moving the debug counters sysfs entries, the commit mentioned below eliminated the
cm_infinibandsysfs directory.This sysfs directory was tied to the
cm_portobject allocated in procedurecm_add_one().Before the commit below, this
cm_portobject was freed via a call tokobject_put(port->kobj)in procedurecm_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 thecm_portbuffers.Fix this by adding explicit kfree(port) calls to functions
cm_add_one()andcm_remove_one().Note: the kfree call in the first chunk below (in the
cm_add_oneerror 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 <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>drivers/infiniband/core/cm.c | 3 +++ 1 file changed, 3 insertions(+)drivers/infiniband/core/cm.c +3
-0+++ 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);
drivers/infiniband/core/cm.c static void cm_remove_one(struct ib_device *ib_device, void *client_data) · line 4466
Quoted 5 lines
drivers/infiniband/core/cm.c +1
-0@@ -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 <[email protected]>
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.
--- drivers/infiniband/core/cm.c | 187 ++++++++++++++++++----------------- 1 file changed, 94 insertions(+), 93 deletions(-)
drivers/infiniband/core/cm.c +94 -93
@@ -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 )
Signature · 1 line
2.23.0
Re: [PATCH 1/4] RDMA/cm: Fix memory leak in cm_add/remove_one
Quoted 56 lines · Jason Gunthorpe
Quoted 46 lines · Leon Romanovsky
From: Jack Morgenstein <[email protected]>
In the process of moving the debug counters sysfs entries, the commit mentioned below eliminated the
cm_infinibandsysfs directory.This sysfs directory was tied to the
cm_portobject allocated in procedurecm_add_one().Before the commit below, this
cm_portobject was freed via a call tokobject_put(port->kobj)in procedurecm_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 thecm_portbuffers.Fix this by adding explicit kfree(port) calls to functions
cm_add_one()andcm_remove_one().Note: the kfree call in the first chunk below (in the
cm_add_oneerror 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 <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>drivers/infiniband/core/cm.c | 3 +++ 1 file changed, 3 insertions(+)drivers/infiniband/core/cm.c +3
-0+++ 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);drivers/infiniband/core/cm.c static void cm_remove_one(struct ib_device *ib_device, void *client_data) · line 4466
Quoted 5 lines
drivers/infiniband/core/cm.c +1
-0@@ -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.
Quoted 1 line
How about this instead:
It looks OK to me.
Thanks
Quoted 286 lines
From
e8dad20c7b69436e63b18f16cd9457ea27da5bc1Mon Sep 17 00:00:00 2001
From: Jack Morgenstein <[email protected]>
Date: Mon, 16 Sep 2019 10:11:51 +0300
Subject: [PATCH] RDMA/cm: Fix memory leak in cm_add/remove_oneIn the process of moving the debug counters sysfs entries, the commit mentioned below eliminated the
cm_infinibandsysfs directory, and created some missing cases where the port pointers were not being freed as thekobject_putwas 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 <[email protected]>--- drivers/infiniband/core/cm.c | 187 ++++++++++++++++++----------------- 1 file changed, 94 insertions(+), 93 deletions(-)drivers/infiniband/core/cm.c +94
-94@@ -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
Re: [PATCH 1/4] RDMA/cm: Fix memory leak in cm_add/remove_one
In reply to Jason GunthorpeQuoted 75 lines · Jason Gunthorpe
Quoted 46 lines · Leon Romanovsky
From: Jack Morgenstein <[email protected]>
In the process of moving the debug counters sysfs entries, the commit mentioned below eliminated the
cm_infinibandsysfs directory.This sysfs directory was tied to the
cm_portobject allocated in procedurecm_add_one().Before the commit below, this
cm_portobject was freed via a call tokobject_put(port->kobj)in procedurecm_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 thecm_portbuffers.Fix this by adding explicit kfree(port) calls to functions
cm_add_one()andcm_remove_one().Note: the kfree call in the first chunk below (in the
cm_add_oneerror 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 <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>drivers/infiniband/core/cm.c | 3 +++ 1 file changed, 3 insertions(+)drivers/infiniband/core/cm.c +3
-0+++ 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);drivers/infiniband/core/cm.c static void cm_remove_one(struct ib_device *ib_device, void *client_data) · line 4466
Quoted 5 lines
drivers/infiniband/core/cm.c +1
-0@@ -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
e8dad20c7b69436e63b18f16cd9457ea27da5bc1Mon Sep 17 00:00:00 2001
From: Jack Morgenstein <[email protected]>
Date: Mon, 16 Sep 2019 10:11:51 +0300
Subject: [PATCH] RDMA/cm: Fix memory leak in cm_add/remove_oneIn the process of moving the debug counters sysfs entries, the commit mentioned below eliminated the
cm_infinibandsysfs directory, and created some missing cases where the port pointers were not being freed as thekobject_putwas 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.
Quoted 5 lines
Fixes c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")Signed-off-by Jason Gunthorpe <[email protected]>--- 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
Re: [PATCH 1/4] RDMA/cm: Fix memory leak in cm_add/remove_one
In reply to Leon RomanovskyQuoted 20 lines · Leon Romanovsky
From: Jack Morgenstein <[email protected]>
In the process of moving the debug counters sysfs entries, the commit mentioned below eliminated the
cm_infinibandsysfs directory.This sysfs directory was tied to the
cm_portobject allocated in procedurecm_add_one().Before the commit below, this
cm_portobject was freed via a call tokobject_put(port->kobj)in procedurecm_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 thecm_portbuffers.Fix this by adding explicit kfree(port) calls to functions
cm_add_one()andcm_remove_one().Note: the kfree call in the first chunk below (in the
cm_add_oneerror flow) fixes an old, undetected memory leak.
Quoted 7 lines
Fixes c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")Signed-off-by Jack Morgenstein <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>--- 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
AppliedRe: [PATCH 1/4] RDMA/cm: Fix memory leak in cm_add/remove_one
In reply to Leon RomanovskyQuoted 20 lines · Leon Romanovsky
From: Jack Morgenstein <[email protected]>
In the process of moving the debug counters sysfs entries, the commit mentioned below eliminated the
cm_infinibandsysfs directory.This sysfs directory was tied to the
cm_portobject allocated in procedurecm_add_one().Before the commit below, this
cm_portobject was freed via a call tokobject_put(port->kobj)in procedurecm_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 thecm_portbuffers.Fix this by adding explicit kfree(port) calls to functions
cm_add_one()andcm_remove_one().Note: the kfree call in the first chunk below (in the
cm_add_oneerror flow) fixes an old, undetected memory leak.
Quoted 7 lines
Fixes c87e65cfb97c ("RDMA/cm: Move debug counters to be under relevant IB device")Signed-off-by Jack Morgenstein <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>Signed-off-by Jason Gunthorpe <[email protected]>--- drivers/infiniband/core/cm.c | 3 +++ 1 file changed, 3 insertions(+)
Applied to for-rc, thanks
Jason
PATCH 2/4 RDMA/counter: Prevent QP counter manual binding in auto mode
In reply to Leon RomanovskyFrom: Mark Zhang <[email protected]>
If auto mode is configured, manual counter allocation and QP bind is not allowed.
--- drivers/infiniband/core/counters.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/infiniband/core/counters.c +11 -1
@@ -463,10 +463,15 @@ static struct rdma_counter *rdma_get_counter_by_id(struct ib_device *dev,int rdma_counter_bind_qpn (struct ib_device *dev,u8 port,u32 qp_num,u32 counter_id) { +struct rdma_port_counter *port_counter;struct rdma_counter *counter;struct ib_qp *qp;int ret; + port_counter = &dev->port_data[port].port_counter; +if (port_counter->mode.mode ==RDMA_COUNTER_MODE_AUTO ) +return -EINVAL ; + qp =rdma_counter_get_qp (dev, qp_num);if (!qp)return -ENOENT ;
@@ -503,6 +508,7 @@ int rdma_counter_bind_qpn(struct ib_device *dev, u8 port,int rdma_counter_bind_qpn_alloc (struct ib_device *dev,u8 port,u32 qp_num,u32 *counter_id) { +struct rdma_port_counter *port_counter;struct rdma_counter *counter;struct ib_qp *qp;int ret;
@@ -510,9 +516,13 @@ int rdma_counter_bind_qpn_alloc(struct ib_device *dev, u8 port,if (!rdma_is_port_valid (dev, port))return -EINVAL ; -if (!dev->port_data[port].port_counter.hstats) + port_counter = &dev->port_data[port].port_counter; +if (!port_counter->hstats)return -EOPNOTSUPP ; +if (port_counter->mode.mode ==RDMA_COUNTER_MODE_AUTO ) +return -EINVAL ; + qp =rdma_counter_get_qp (dev, qp_num);if (!qp)return -ENOENT ;
--
2.20.1
Jason Gunthorpe
AppliedRe: [PATCH 2/4] RDMA/counter: Prevent QP counter manual binding in auto mode
Quoted 11 lines · Leon Romanovsky
From: Mark Zhang <[email protected]>
If auto mode is configured, manual counter allocation and QP bind is not allowed.
Fixes 1bd8e0a9d0fd ("RDMA/counter: Allow manual mode configuration support")Signed-off-by Mark Zhang <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/counters.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
Applied to for-next
Quoted 2 lines
--
2.20.1
This line ended up in a weird place
Thanks,
Jason
PATCH 3/4 RDMA/nldev: Reshuffle the code to avoid need to rebind QP in error path
In reply to Leon RomanovskyFrom: Leon Romanovsky <[email protected]>
Properly unwind QP counter rebinding in case of failure.
--- drivers/infiniband/core/nldev.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/infiniband/core/nldev.c +5 -7
@@ -1860,24 +1860,22 @@ static int nldev_stat_del_doit(struct sk_buff *skb, struct nlmsghdr *nlh, cntn =nla_get_u32 (tb[RDMA_NLDEV_ATTR_STAT_COUNTER_ID ]); qpn =nla_get_u32 (tb[RDMA_NLDEV_ATTR_RES_LQPN ]); - ret =rdma_counter_unbind_qpn (device, port, qpn, cntn); -if (ret) - goto err_unbind; -if (fill_nldev_handle (msg, device) ||nla_put_u32 (msg,RDMA_NLDEV_ATTR_PORT_INDEX , port) ||nla_put_u32 (msg,RDMA_NLDEV_ATTR_STAT_COUNTER_ID , cntn) ||nla_put_u32 (msg,RDMA_NLDEV_ATTR_RES_LQPN , qpn)) { ret = -EMSGSIZE ; - goto err_fill; + goto err_unbind; } + ret =rdma_counter_unbind_qpn (device, port, qpn, cntn); +if (ret) + goto err_unbind; +nlmsg_end (msg, nlh);ib_device_put (device);return rdma_nl_unicast (sock_net (skb->sk), msg,NETLINK_CB (skb).portid); -err_fill: -rdma_counter_bind_qpn (device, port, qpn, cntn); err_unbind:nlmsg_free (msg); err:
--
2.20.1
Re: [PATCH 3/4] RDMA/nldev: Reshuffle the code to avoid need to rebind QP in error path
Quoted 3 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Properly unwind QP counter rebinding in case of failure.
What is the actual problem here? Calling 'bind' in an error unwind seems insane, is that the issue?
Quoted 14 lines
Fixes b389327df905 ("RDMA/nldev: Allow counter manual mode configration through RDMA netlink")Reviewed-by Mark Zhang <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>drivers/infiniband/core/nldev.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)drivers/infiniband/core/nldev.c +1
-1+++ b/drivers/infiniband/core/nldev.c@@ -1860,24 +1860,22 @@ static int nldev_stat_del_doit(struct sk_buff *skb, struct nlmsghdr *nlh, cntn =nla_get_u32 (tb[RDMA_NLDEV_ATTR_STAT_COUNTER_ID ]); qpn =nla_get_u32 (tb[RDMA_NLDEV_ATTR_RES_LQPN ]); - ret =rdma_counter_unbind_qpn (device, port, qpn, cntn);
drivers/infiniband/core/nldev.c static int nldev_stat_del_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 1868
Quoted 10 lines
drivers/infiniband/core/nldev.c +1
-4-if (ret) - goto err_unbind; -if (fill_nldev_handle (msg, device) ||nla_put_u32 (msg,RDMA_NLDEV_ATTR_PORT_INDEX , port) ||nla_put_u32 (msg,RDMA_NLDEV_ATTR_STAT_COUNTER_ID , cntn) ||nla_put_u32 (msg,RDMA_NLDEV_ATTR_RES_LQPN , qpn)) { ret = -EMSGSIZE ; - goto err_fill; + goto err_unbind;
These label names don't make much sense anymore
Jason
Re: [PATCH 3/4] RDMA/nldev: Reshuffle the code to avoid need to rebind QP in error path
Quoted 7 lines · Jason Gunthorpe
Quoted 3 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Properly unwind QP counter rebinding in case of failure.
What is the actual problem here? Calling 'bind' in an error unwind seems insane, is that the issue?
Yep
Quoted 29 lines
Quoted 15 lines
Fixes b389327df905 ("RDMA/nldev: Allow counter manual mode configration through RDMA netlink")Reviewed-by Mark Zhang <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>drivers/infiniband/core/nldev.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)drivers/infiniband/core/nldev.c +1
-1+++ b/drivers/infiniband/core/nldev.c@@ -1860,24 +1860,22 @@ static int nldev_stat_del_doit(struct sk_buff *skb, struct nlmsghdr *nlh, cntn =nla_get_u32 (tb[RDMA_NLDEV_ATTR_STAT_COUNTER_ID ]); qpn =nla_get_u32 (tb[RDMA_NLDEV_ATTR_RES_LQPN ]); - ret =rdma_counter_unbind_qpn (device, port, qpn, cntn);drivers/infiniband/core/nldev.c static int nldev_stat_del_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 1868
Quoted 10 lines
drivers/infiniband/core/nldev.c +1
-4-if (ret) - goto err_unbind; -if (fill_nldev_handle (msg, device) ||nla_put_u32 (msg,RDMA_NLDEV_ATTR_PORT_INDEX , port) ||nla_put_u32 (msg,RDMA_NLDEV_ATTR_STAT_COUNTER_ID , cntn) ||nla_put_u32 (msg,RDMA_NLDEV_ATTR_RES_LQPN , qpn)) { ret = -EMSGSIZE ; - goto err_fill; + goto err_unbind;These label names don't make much sense anymore
Jason
PATCH 4/4 RDMA: Fix double-free in srq creation error flow
In reply to Leon RomanovskyFrom: Jack Morgenstein <[email protected]>
The cited commit introduced a double-free of the srq buffer in the error flow of procedure __uverbs_create_xsrq().
The problem is that procedure ib_destroy_srq_user() called in the error flow also frees the srq buffer.
Thus, if uverbs_response() fails in __uverbs_create_srq(), the srq buffer will be freed twice.
--- drivers/infiniband/core/uverbs_cmd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/infiniband/core/uverbs_cmd.c +2 -1
@@ -3482,7 +3482,8 @@ static int __uverbs_create_xsrq(struct uverbs_attr_bundle *attrs, err_copy:ib_destroy_srq_user (srq,uverbs_get_cleared_udata (attrs)); - +/* It was released in ib_destroy_srq_user */ + srq =NULL ; err_free:kfree (srq); err_put:
--
2.20.1
Re: [PATCH 4/4] RDMA: Fix double-free in srq creation error flow
Quoted 21 lines · Leon Romanovsky
From: Jack Morgenstein <[email protected]>
The cited commit introduced a double-free of the srq buffer in the error flow of procedure
__uverbs_create_xsrq().The problem is that procedure
ib_destroy_srq_user()called in the error flow also frees the srq buffer.Thus, if
uverbs_response()fails in__uverbs_create_srq(), the srq buffer will be freed twice.Fixes 68e326dea1db ("RDMA: Handle SRQ allocations by IB/core")Signed-off-by Jack Morgenstein <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>drivers/infiniband/core/uverbs_cmd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/infiniband/core/uverbs_cmd.c +1
-0+++ b/drivers/infiniband/core/uverbs_cmd.c
drivers/infiniband/core/uverbs_cmd.c static int __uverbs_create_xsrq(struct uverbs_attr_bundle *attrs, · line 3486
Quoted 6 lines
drivers/infiniband/core/uverbs_cmd.c +2
-1@@ -3482,7 +3482,8 @@ static int __uverbs_create_xsrq(struct uverbs_attr_bundle *attrs, err_copy:ib_destroy_srq_user (srq,uverbs_get_cleared_udata (attrs)); - +/* It was released in ib_destroy_srq_user */ + srq =NULL ;
I really don't like that we ended up with such a mess of error unwind.
The proper outcome should be that uobj_alloc_abort() does a full clean up, including destroying and freeing the HW object if it was allocated.
When we forced the new uobj system into the write() path this was never cleaned up, instead the abort just disables the HW object clean up to avoid alot of code churn, while ioctl does actually clean the HW object on failure.
Ie only one clean up path for uobjects.
I also wonder if there is another race here, this is also missing the ib_uverbs_release_uevent() after destroying the HW object, but I don't know if it could be possible for an event to be stuffed in..
In any event, the double free is clearly bad, so applied to for-next
Thanks,
Jason