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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Apr 28, 2026 at 11:47:16PM +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(), if kobject_init_and_add() fails, the error path frees p
> directly instead of releasing the kobject reference with kobject_put().
> This may leave the reference count of the embedded struct kobject
> unbalanced, resulting in a refcount leak and potentially leading to a
> use-after-free.
> 
> The issue was identified by a static analysis tool I developed and
> confirmed by manual review.
> 
> Fix this by using kobject_put(&p->kobj) in the kobject_init_and_add()
> failure path.
> 
> Fixes: c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
> 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 | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c
> index b8fa4ecfc961..38c64b5fb23a 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);
>  }
>  
> @@ -640,7 +645,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_kobj;
>  
>  	p->pkey_group.name  = "pkey_idx";
>  	p->pkey_group.attrs =
> @@ -687,6 +692,12 @@ static int add_port(struct mlx4_ib_dev *dev, int port_num, int slave)
>  	kobject_put(dev->dev_ports_parent[slave]);
>  	kfree(p);
>  	return ret;
> +
> +err_kobj:
> +	kobject_put(dev->dev_ports_parent[slave]);
> +	kobject_put(&p->kobj);
> +	return ret;
> +

Do not put double returns in goto-unwinds..

This should be fixed to open code the kobject_init() immediately after
the memory allocation so we never switch between kfree and put, and fix
all the release functions to tolerate half initialized objects.

Then you can remove the mess of kfrees which are all duplicated in the
release function.

Jason