* Re: [PATCH v4] IB/mlx4: Fix refcount leak in add_port() error path
[not found] <20260428163014.379069-1-lgs201920130244@gmail.com>
@ 2026-05-11 11:22 ` Leon Romanovsky
2026-05-11 12:23 ` Guangshuo Li
0 siblings, 1 reply; 2+ messages in thread
From: Leon Romanovsky @ 2026-05-11 11:22 UTC (permalink / raw)
To: Guangshuo Li
Cc: Yishai Hadas, Jason Gunthorpe, Roland Dreier, Jack Morgenstein,
linux-rdma, linux-kernel
On Wed, Apr 29, 2026 at 12:30:14AM +0800, Guangshuo Li wrote:
> 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(), several failure paths after kobject_init_and_add() free
> struct mlx4_port directly instead of releasing the embedded kobject with
> kobject_put(). This leaves the kobject reference count unbalanced and can
> lead to incorrect lifetime handling.
AFAIK, you should call to kobject_put() if kobject_init_and_add() fails,
but in other case, you should call to kobject_del().
Thanks
>
> Fix this by routing all failures after kobject_init_and_add() through a
> single kobject_put() based error path. Since the release callback may now
> be called for partially initialized mlx4_port objects, make
> mlx4_port_release() tolerate NULL attribute arrays.
>
> The issue was identified by a static analysis tool I developed and
> confirmed by manual review.
>
> Fixes: c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
> 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 | 44 ++++++++++++++----------------
> 1 file changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c
> index b8fa4ecfc961..fe505e07849d 100644
> --- a/drivers/infiniband/hw/mlx4/sysfs.c
> +++ b/drivers/infiniband/hw/mlx4/sysfs.c
> @@ -380,12 +380,17 @@ static void mlx4_port_release(struct kobject *kobj)
> struct attribute *a;
> int i;
>
> - for (i = 0; (a = p->pkey_group.attrs[i]); ++i)
> - kfree(a);
> - kfree(p->pkey_group.attrs);
> - for (i = 0; (a = p->gid_group.attrs[i]); ++i)
> - kfree(a);
> - kfree(p->gid_group.attrs);
> + if (p->pkey_group.attrs) {
> + for (i = 0; (a = p->pkey_group.attrs[i]); ++i)
> + kfree(a);
> + kfree(p->pkey_group.attrs);
> + }
> +
> + if (p->gid_group.attrs) {
> + for (i = 0; (a = p->gid_group.attrs[i]); ++i)
> + kfree(a);
> + kfree(p->gid_group.attrs);
> + }
> kfree(p);
> }
>
> @@ -623,7 +628,6 @@ static void remove_vf_smi_entries(struct mlx4_port *p)
> static int add_port(struct mlx4_ib_dev *dev, int port_num, int slave)
> {
> struct mlx4_port *p;
> - int i;
> int ret;
> int is_eth = rdma_port_get_link_layer(&dev->ib_dev, port_num) ==
> IB_LINK_LAYER_ETHERNET;
> @@ -640,7 +644,7 @@ static int add_port(struct mlx4_ib_dev *dev, int port_num, int slave)
> kobject_get(dev->dev_ports_parent[slave]),
> "%d", port_num);
> if (ret)
> - goto err_alloc;
> + goto err_put;
>
> p->pkey_group.name = "pkey_idx";
> p->pkey_group.attrs =
> @@ -649,44 +653,36 @@ 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_put;
> }
>
> ret = sysfs_create_group(&p->kobj, &p->pkey_group);
> if (ret)
> - goto err_free_pkey;
> + goto err_put;
>
> p->gid_group.name = "gid_idx";
> p->gid_group.attrs = alloc_group_attrs(show_port_gid_idx, NULL, 1);
> if (!p->gid_group.attrs) {
> ret = -ENOMEM;
> - goto err_free_pkey;
> + goto err_put;
> }
>
> ret = sysfs_create_group(&p->kobj, &p->gid_group);
> if (ret)
> - goto err_free_gid;
> + goto err_put;
>
> ret = add_vf_smi_entries(p);
> if (ret)
> - goto err_free_gid;
> + goto err_put;
>
> 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_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:
> +err_put:
> kobject_put(dev->dev_ports_parent[slave]);
> - kfree(p);
> + kobject_put(&p->kobj);
> return ret;
> +
> }
>
> static int register_one_pkey_tree(struct mlx4_ib_dev *dev, int slave)
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread