linux-kernel - Re: [PATCH v1 1/1] ACPI: scan: Use unique number for instance

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]

Date:   Fri, 19 Mar 2021 20:06:18 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>
Subject: Re: [PATCH v1 1/1] ACPI: scan: Use unique number for instance_no

On Fri, Mar 19, 2021 at 06:00:38PM +0100, Rafael J. Wysocki wrote:
> On Fri, Mar 12, 2021 at 5:02 PM Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> >
> > Current mechanism of incrementing and decrementing plain integer
> > to get a next free instance_no when creating an ACPI device is fragile.
> >
> > In case of hot plug event or namespace removal of the device instances
> > with the low numbers the plain integer counter can't cover the gaps
> > and become desynchronized with real state of affairs. If during next
> > hot plug event or namespace injection the new instances of
> > the devices need to be instantiated, the counter may mistakenly point
> > to the existing instance_no and kernel will complain:
> > "sysfs: cannot create duplicate filename '/bus/acpi/devices/XXXX1234:02'"
> 
> This is a slightly convoluted way of stating that there is a bug in
> acpi_device_del().

Any suggestion how to massage the above?
But in the dry end, yes, decrementing is a bug.

> Yes, there is one, the instance_no decrementation is clearly incorrect.
> 
> > Replace plain integer approach by using IDA framework.
> 
> Also the general idea of using IDA for the instance numbering is a good one IMO.

...

> > -       unsigned int instance_no;
> > +       struct ida no;
> 
> struct ida instance_ida; ?

Will rename.

...

> > +       p = strrchr(dev_name(&device->dev), ':');
> > +       if (!p)
> > +               return -ENODATA;
> > +
> > +       error = kstrtoint(p + 1, 16, &result);
> > +       if (error)
> > +               return error;
> > +
> > +       return result;
> 
> I don't like the above at all.
> 
> I would just store the instance number in struct acpi_device_pnp (say).

TBH, I simply didn't know which struct to touch and left this one and I also
don't like it. Lemme see if acpi_device_pnp is good enough for that.

-- 
With Best Regards,
Andy Shevchenko