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

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

[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(), 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.

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, route failures through kobject_put(), and call kobject_del()
before kobject_put() on later failure paths after the kobject has been
successfully added.

Fixes: c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
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 | 39 ++++++++++++++++--------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c
index b8fa4ecfc961..e4c822c96ee6 100644
--- a/drivers/infiniband/hw/mlx4/sysfs.c
+++ b/drivers/infiniband/hw/mlx4/sysfs.c
@@ -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,41 @@ 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_del;
 
 	ret = add_vf_smi_entries(p);
 	if (ret)
-		goto err_free_gid;
+		goto err_del;
 
 	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_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;
 }
-- 
2.43.0

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

[Leon Romanovsky]

On Thu, May 14, 2026 at 07:01:39PM +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.
> 
> 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, route failures through kobject_put(), and call kobject_del()
> before kobject_put() on later failure paths after the kobject has been
> successfully added.
> 
> Fixes: c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
> 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 | 39 ++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c
> index b8fa4ecfc961..e4c822c96ee6 100644
> --- a/drivers/infiniband/hw/mlx4/sysfs.c
> +++ b/drivers/infiniband/hw/mlx4/sysfs.c
> @@ -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,41 @@ 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_del;

You should call to sysfs_remove_group() too.

Thanks

>  
>  	ret = add_vf_smi_entries(p);
>  	if (ret)
> -		goto err_free_gid;
> +		goto err_del;
>  
>  	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_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;
>  }
> -- 
> 2.43.0
> 

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

[Guangshuo Li]

Hi Leon,

Thanks for reviewing.

On Sun, 17 May 2026 at 22:27, Leon Romanovsky <leon@kernel.org> wrote:
>
> On Thu, May 14, 2026 at 07:01:39PM +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.
> >
> > 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, route failures through kobject_put(), and call kobject_del()
> > before kobject_put() on later failure paths after the kobject has been
> > successfully added.
> >
> > Fixes: c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")
> > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> > ---
> > 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 | 39 ++++++++++++++++--------------
> >  1 file changed, 21 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c
> > index b8fa4ecfc961..e4c822c96ee6 100644
> > --- a/drivers/infiniband/hw/mlx4/sysfs.c
> > +++ b/drivers/infiniband/hw/mlx4/sysfs.c
> > @@ -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,41 @@ 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_del;
>
> You should call to sysfs_remove_group() too.
>
> Thanks
>
I added sysfs_remove_group() for the successfully created
pkey/gid groups before kobject_del() and sent v7.