Re: PCI: Race condition in pci_create_sysfs_dev_files

[Pali Rohár]

On Thursday 08 October 2020 14:59:07 Bjorn Helgaas wrote:
> On Wed, Oct 07, 2020 at 11:14:34AM -0500, Bjorn Helgaas wrote:
> > On Wed, Oct 07, 2020 at 10:14:00AM +0200, Pali Rohár wrote:
> > > On Wednesday 07 October 2020 12:47:40 Oliver O'Halloran wrote:
> > > > On Wed, Oct 7, 2020 at 10:26 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > >
> > > > > I'm not really a fan of this because pci_sysfs_init() is a bit of a
> > > > > hack to begin with, and this makes it even more complicated.
> > > > >
> > > > > It's not obvious from the code why we need pci_sysfs_init(), but
> > > > > Yinghai hinted [1] that we need to create sysfs after assigning
> > > > > resources.  I experimented by removing pci_sysfs_init() and skipping
> > > > > the ROM BAR sizing.  In that case, we create sysfs files in
> > > > > pci_bus_add_device() and later assign space for the ROM BAR, so we
> > > > > fail to create the "rom" sysfs file.
> > > > >
> > > > > The current solution to that is to delay the sysfs files until
> > > > > pci_sysfs_init(), a late_initcall(), which runs after resource
> > > > > assignments.  But I think it would be better if we could create the
> > > > > sysfs file when we assign the BAR.  Then we could get rid of the
> > > > > late_initcall() and that implicit ordering requirement.
> > > > 
> > > > You could probably fix that by using an attribute_group to control
> > > > whether the attribute shows up in sysfs or not. The .is_visible() for
> > > > the group can look at the current state of the device and hide the rom
> > > > attribute if the BAR isn't assigned or doesn't exist. That way we
> > > > don't need to care when the actual assignment occurs.
> > > 
> > > And cannot we just return e.g. -ENODATA (or other error code) for those
> > > problematic sysfs nodes until late_initcall() is called?
> > 
> > I really like Oliver's idea and I think we should push on that to see
> > if it can be made to work.  If so, we can remove the late_initcall()
> > completely.
> > 
> > > > > But I haven't tried to code it up, so it's probably more complicated
> > > > > than this.  I guess ideally we would assign all the resources before
> > > > > pci_bus_add_device().  If we could do that, we could just remove
> > > > > pci_sysfs_init() and everything would just work, but I think that's a
> > > > > HUGE can of worms.
> > > > 
> > > > I was under the impression the whole point of pci_bus_add_device() was
> > > > to handle any initialisation that needed to be done after resources
> > > > were assigned. Is the ROM BAR being potentially unassigned an x86ism
> > > > or is there some bigger point I'm missing?
> > 
> > We can't assign resources for each device as we enumerate it because
> > we don't know what's in use by other devices yet to be enumerated.
> > That part is generic, not x86-specific.
> > 
> > The part that is x86-specific (or at least specific to systems using
> > ACPI) is that the ACPI core doesn't reserve resources used by ACPI
> > devices.  Sometimes those resources are included in the PCI host
> > bridge windows, and we don't want to assign them to PCI devices.
> > 
> > I didn't trace this all the way, but the pcibios_assign_resources()
> > and pnp_system_init() comments look relevant.  It's a little concerning
> > that they're both fs_initcalls() and the ordering looks important, but
> > it would only be by accident of link ordering that pnp_system_init()
> > happens first.
> 
> Pali, what's your thought on this?  Do you plan to work on this
> yourself?  If not and if you can live with your workaround a while
> longer, I think Krzysztof might be interested in taking a crack at it.
> I would just hate to see you guys duplicate each others' work :)

Hello Bjorn!

If Krzysztof wants and would be working on this issue I can let it as is
for now.

But we should think how to deliver fix for this issue also into stable
kernels where this race condition is happening.

I think that my workaround avoid those two race conditions and if proper
fix (= removal of pci_sysfs_init function) would take a long, what about
trying to workaround that race condition for now?

My "fix" is relatively small and simple, so it should not be much hard
to review it.

Krzysztof, what do you think?