Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 23 Nov 2022 20:25:53 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Yang Yingliang <yangyingliang@huawei.com>,
        linux-pm@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] powercap: fix possible name leak while device_register()
 fails
Message-ID: <Y35zwffl7bUdjs93@kroah.com>
References: <20221112094048.3614365-1-yangyingliang@huawei.com>
 <CAJZ5v0g6bct5pTruxHjjJPQpwDNR8WgqhLWaZWo8NKeRH3n6iw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJZ5v0g6bct5pTruxHjjJPQpwDNR8WgqhLWaZWo8NKeRH3n6iw@mail.gmail.com>
Precedence: bulk

On Wed, Nov 23, 2022 at 08:00:14PM +0100, Rafael J. Wysocki wrote:
> On Sat, Nov 12, 2022 at 10:42 AM Yang Yingliang
> <yangyingliang@huawei.com> wrote:
> >
> > If device_register() returns error, the name allocated by
> > dev_set_name() need be freed. In technical, we should call
> > put_device() to give up the reference and free the name in
> > driver core, but in some cases the device is not intizalized,
> > put_device() can not be called, so don't complicate the code,
> > just call kfree_const() to free name in the error path.
> >
> > Fixes: 75d2364ea0ca ("PowerCap: Add class driver")
> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> > ---
> >  drivers/powercap/powercap_sys.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c
> > index f0654a932b37..11e742dc83b9 100644
> > --- a/drivers/powercap/powercap_sys.c
> > +++ b/drivers/powercap/powercap_sys.c
> > @@ -572,6 +572,7 @@ struct powercap_zone *powercap_register_zone(
> >  err_name_alloc:
> >         idr_remove(power_zone->parent_idr, power_zone->id);
> >  err_idr_alloc:
> > +       kfree_const(dev_name(&power_zone->dev));
> >         if (power_zone->allocated)
> >                 kfree(power_zone);
> >         mutex_unlock(&control_type->lock);
> > @@ -622,6 +623,7 @@ struct powercap_control_type *powercap_register_control_type(
> >         dev_set_name(&control_type->dev, "%s", name);
> >         result = device_register(&control_type->dev);
> >         if (result) {
> > +               kfree_const(dev_name(&control_type->dev));
> 
> Why is it necessary to free a device name explicitly after a failing
> device_register()?
> 
> If it is really necessary, then there is a problem in
> device_register() itself AFAICS, because it uses dev_set_name() at
> least in the dev->init_name present case.

I think we already fixed this in the driver core, so these types of
patches should not be applied.

Yang, can you make sure you respond to all of them and say "this is not
needed anymore!" and if any got merged, send reverts for them?

thanks,

greg k-h