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

[Guangshuo Li]

Hi Leon,

Thanks for reviewing.

On Thu, 14 May 2026 at 15:10, Leon Romanovsky <leon@xxxxxxxxxx> wrote:
>
> On Mon, May 11, 2026 at 08:16:49PM +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.
> >
> > Fix this by routing the kobject_init_and_add() failure path through
> > kobject_put(), and by calling kobject_del() before kobject_put() on
> > later failure paths after the kobject has been successfully added. Since
> > the release callback may now be called for partially initialized
> > mlx4_port objects, make mlx4_port_release() tolerate NULL attribute
> > arrays.
> >
> > The duplicated attribute array frees in add_port() are removed, as the
> > release callback now handles them.
> >
> > Fixes: c1e7e466120b ("IB/mlx4: Add iov directory in sysfs under the ib device")
> > Cc: stable@xxxxxxxxxxxxxxx
>
> This line is not needed.
>
> > Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> > ---
> > 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 | 44 ++++++++++++++----------------
> >  1 file changed, 21 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c
> > index b8fa4ecfc961..224a6a1c289d 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);
> > +     }
>
> You should reorder the add_port() function to make sure that
> kobject_init_and_add() is called after alloc_group_attrs().
>
> Thanks
>
> >       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,43 +653,37 @@ 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_del;
> >       }
> >
> >       ret = sysfs_create_group(&p->kobj, &p->pkey_group);
> >       if (ret)
> > -             goto err_free_pkey;
> > +             goto err_del;
> >
> >       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_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_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_del:
> > +     kobject_del(&p->kobj);
> >
> > -err_alloc:
> > +err_put:
> >       kobject_put(dev->dev_ports_parent[slave]);
> > -     kfree(p);
> > +     kobject_put(&p->kobj);
> >       return ret;
> >  }
> >
> > --
> > 2.43.0
> >

I have sent the v6 version according to your suggestion.

Best regards,
Guangshuo