linux-kernel - Re: [PATCH v2 1/3] gpio: introduce utilities for synchronous fake device creation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fcfanp2bfokggj3dhzdceaza2bhxo7xrbru5cy36ciaumpfua2@kvabbos3znjt>
Date: Thu, 20 Feb 2025 22:39:44 +0900
From: Koichiro Den <koichiro.den@...onical.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: linux-gpio@...r.kernel.org, geert+renesas@...der.be, 
	linus.walleij@...aro.org, maciej.borzecki@...onical.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] gpio: introduce utilities for synchronous fake
 device creation

On Thu, Feb 20, 2025 at 02:13:42PM GMT, Bartosz Golaszewski wrote:
> On Thu, Feb 20, 2025 at 2:07 PM Koichiro Den <koichiro.den@...onical.com> wrote:
> >
> > On Thu, Feb 20, 2025 at 12:06:33PM GMT, Bartosz Golaszewski wrote:
> > > On Tue, Feb 18, 2025 at 5:04 PM Koichiro Den <koichiro.den@...onical.com> wrote:
> > > >
> > > > Both gpio-sim and gpio-virtuser share a mechanism to instantiate a
> > > > platform device, wait for probe completion, and retrieve the probe
> > > > success or error status synchronously. With gpio-aggregator planned to
> > > > adopt this approach for its configfs interface, it's time to factor
> > > > out the common code.
> 
> [snip]
> 
> > > > +void dev_sync_probe_init(struct dev_sync_probe_data *data)
> > > > +{
> > > > +       memset(data, 0, sizeof(*data));
> > > > +       init_completion(&data->probe_completion);
> > > > +       data->bus_notifier.notifier_call = dev_sync_probe_notifier_call;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(dev_sync_probe_init);
> > > > +
> > > > +int dev_sync_probe_register(struct dev_sync_probe_data *data,
> > > > +                           struct platform_device_info *pdevinfo)
> > > > +{
> > > > +       struct platform_device *pdev;
> > > > +       char *name;
> > > > +
> > > > +       name = kasprintf(GFP_KERNEL, "%s.%u", pdevinfo->name, pdevinfo->id);
> > >
> > > pdevinfo->id is a signed integer
> > >
> > > I'm also wondering if we could avoid the allocation here and keep on
> > > using snprintf() like in the existing drivers? On the other hand,
> > > memory is cheap so no big deal.
> >
> > Are you assuming the following change?
> >
> >    struct dev_sync_probe_data {
> >           struct platform_device *pdev;
> >   -       const char *name;
> >   +       char name[32];
> >
> >           /* Synchronize with probe */
> >           struct notifier_block bus_notifier;
> >
> 
> No, I was thinking about a local buffer in the notifier handler, like
> what we do currently in gpio-sim, but no worries, you can keep it this
> way.
> 
> > >
> > > > +       if (!name)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       data->driver_bound = false;
> > > > +       data->name = name;
> > > > +       reinit_completion(&data->probe_completion);
> > > > +       bus_register_notifier(&platform_bus_type, &data->bus_notifier);
> > > > +
> > > > +       pdev = platform_device_register_full(pdevinfo);
> > > > +       if (IS_ERR(pdev)) {
> > > > +               bus_unregister_notifier(&platform_bus_type, &data->bus_notifier);
> > > > +               kfree(data->name);
> > >
> > > We could probably simplify it by using __free(kfree) with the name
> > > variable and just setting it at the end with no_free_ptr().
> >
> > platform_device_register_full() call path might finish probe so before
> > calling it, we need to make sure the 'name' is filled in. That's why I
> > didn't used __free(kfree).
> >
> 
> Not sure I understand this. Would you mind rephrasing?

dev_sync_probe_notifier_call() references dev_sync_probe_data's 'name'
field. In dev_sync_probe_register(), platform_device_register_full()
invocation can possibly succeed in the initial probe, meaning that
dev_sync_probe_notifier_call() can be invoked before
platform_device_register_full() returns. So, 'name' field must be set
beforehand, and I located 'data->name = name' as shown above;

If I used __free(kfree), the number of which I needed to write
'no_free_ptr(data->name);' would be the same (= 2 times). So I thought that
calling kfree(data->name) without __free(kfree) was simpler and better.

> 
> Bart