Skip to contentSkip to conversation

IB/mlx4: Fix refcount leak in add_port() error path

PATCH Applied Guangshuo Li 2 participants 2 messages May 18 – May 25

Subsystems (2)
InfiniBand Mellanox MLX4 IB driver
lore
Share & tools
lore

Guangshuo Li

author
#0 lore raw

PATCH v7 1/1 IB/mlx4: Fix refcount leak in add_port() error path

After kobject_init_and_add(), the lifetime of the embedded struct kobject is expected to be managed through the kobject core reference counting.

In add_port(), failure paths after kobject_init_and_add() must not free struct mlx4_port directly, because the embedded kobject is then managed by the kobject core. Freeing it directly leaves the kobject reference counting unbalanced and can lead to incorrect lifetime handling.

Allocate the pkey and gid attribute arrays before kobject_init_and_add(), so failures before kobject initialization can be handled by directly freeing the allocated memory. Once kobject_init_and_add() has been called, unwind later failures by removing any successfully created sysfs groups, calling kobject_del(), and then releasing the embedded kobject with kobject_put().

Fixes c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")
Signed-off-by Guangshuo Li <[email protected]>

---
v7:
- remove already created sysfs groups on add_port() error paths before deleting and putting the kobject

v6:
- drop the Cc stable tag
- allocate pkey and gid attribute arrays before kobject_init_and_add()
- keep the release callback unchanged by ensuring the attribute arrays are initialized before kobject_init_and_add()

v5:
- split the add_port() error paths after kobject_init_and_add()
- call kobject_del() before kobject_put() for failures after kobject_init_and_add() succeeds

v4:
- route all add_port() failures after kobject_init_and_add() through a single kobject_put() based error path
- remove duplicated attribute array frees from add_port()
- keep mlx4_port_release() tolerant of partially initialized objects

v3:
- make mlx4_port_release() tolerate NULL attribute arrays
- drop the parent kobject reference on the kobject_init_and_add() failure path before putting the embedded kobject

v2:
- note that the issue was identified by my static analysis tool
- and confirmed by manual review

 drivers/infiniband/hw/mlx4/sysfs.c | 45 ++++++++++++++++++------------
 1 file changed, 27 insertions(+), 18 deletions(-)
drivers/infiniband/hw/mlx4/sysfs.c +27 -18
index b8fa4ecfc961..e688ad66a895 100644
@@ -636,12 +636,6 @@ static int add_port(struct mlx4_ib_dev *dev, int port_num, int slave)
 	p->port_num = port_num;
 	p->slave = slave;
 
-	ret = kobject_init_and_add(&p->kobj, &port_type,
-				   kobject_get(dev->dev_ports_parent[slave]),
-				   "%d", port_num);
-	if (ret)
-		goto err_alloc;
-
 	p->pkey_group.name  = "pkey_idx";
 	p->pkey_group.attrs =
 		alloc_group_attrs(show_port_pkey,

@@ -649,13 +643,9 @@ static int add_port(struct mlx4_ib_dev *dev, int port_num, int slave)
 				  dev->dev->caps.pkey_table_len[port_num]);
 	if (!p->pkey_group.attrs) {
 		ret = -ENOMEM;
-		goto err_alloc;
+		goto err_free_port;
 	}
 
-	ret = sysfs_create_group(&p->kobj, &p->pkey_group);
-	if (ret)
-		goto err_free_pkey;
-
 	p->gid_group.name  = "gid_idx";
 	p->gid_group.attrs = alloc_group_attrs(show_port_gid_idx, NULL, 1);
 	if (!p->gid_group.attrs) {

@@ -663,28 +653,47 @@ static int add_port(struct mlx4_ib_dev *dev, int port_num, int slave)
 		goto err_free_pkey;
 	}
 
+	ret = kobject_init_and_add(&p->kobj, &port_type,
+				   kobject_get(dev->dev_ports_parent[slave]),
+				   "%d", port_num);
+	if (ret)
+		goto err_put;
+
+	ret = sysfs_create_group(&p->kobj, &p->pkey_group);
+	if (ret)
+		goto err_del;
+
 	ret = sysfs_create_group(&p->kobj, &p->gid_group);
 	if (ret)
-		goto err_free_gid;
+		goto err_remove_pkey;
 
 	ret = add_vf_smi_entries(p);
 	if (ret)
-		goto err_free_gid;
+		goto err_remove_gid;
 
 	list_add_tail(&p->kobj.entry, &dev->pkeys.pkey_port_list[slave]);
 	return 0;
 
-err_free_gid:
-	kfree(p->gid_group.attrs[0]);
-	kfree(p->gid_group.attrs);
+err_remove_gid:
+	sysfs_remove_group(&p->kobj, &p->gid_group);
+
+err_remove_pkey:
+	sysfs_remove_group(&p->kobj, &p->pkey_group);
+
+err_del:
+	kobject_del(&p->kobj);
+
+err_put:
+	kobject_put(dev->dev_ports_parent[slave]);
+	kobject_put(&p->kobj);
+	return ret;
 
 err_free_pkey:
 	for (i = 0; i < dev->dev->caps.pkey_table_len[port_num]; ++i)
 		kfree(p->pkey_group.attrs[i]);
 	kfree(p->pkey_group.attrs);
 
-err_alloc:
-	kobject_put(dev->dev_ports_parent[slave]);
+err_free_port:
 	kfree(p);
 	return ret;
 }
Signature · 1 line
2.43.0
maintainer Applied
#1 lore raw
Quoted 16 lines · Guangshuo Li

After kobject_init_and_add(), the lifetime of the embedded struct kobject is expected to be managed through the kobject core reference counting.

In add_port(), failure paths after kobject_init_and_add() must not free struct mlx4_port directly, because the embedded kobject is then managed by the kobject core. Freeing it directly leaves the kobject reference counting unbalanced and can lead to incorrect lifetime handling.

Allocate the pkey and gid attribute arrays before kobject_init_and_add(), so failures before kobject initialization can be handled by directly freeing the allocated memory. Once kobject_init_and_add() has been called, unwind later failures by removing any successfully created sysfs groups, calling kobject_del(), and then releasing the embedded kobject with kobject_put().

Quoted 5 lines
Fixes c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")
Signed-off-by Guangshuo Li <[email protected]>
 drivers/infiniband/hw/mlx4/sysfs.c | 45 ++++++++++++++++++------------
 1 file changed, 27 insertions(+), 18 deletions(-)

Applied to for-next, thanks

Jason