Re: [PATCH v4] IB/mlx4: Fix refcount leak in add_port() error path

Re: [PATCH v4] IB/mlx4: Fix refcount leak in add_port() error path

[Leon Romanovsky]

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
> 
> 

Re: [PATCH v4] IB/mlx4: Fix refcount leak in add_port() error path

[Guangshuo Li]

Hi Leon,

On Mon, 11 May 2026 at 19:22, Leon Romanovsky <leon@kernel.org> wrote:
>
> 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

Thanks, I agree. I will address this in v5 by splitting the error paths:
kobject_init_and_add() failure will go through kobject_put(), while later
failures after a successful add will call kobject_del() before dropping
the references.

Thanks,
Guangshuo