linux-kernel - Re: [PATCH] PCI: pci_bus_add_device() should succeed even if creating sysfs files fails

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] [day] [month] [year] [list]

Message-ID: <20130417173229.GA23950@thinkpad.lan>
Date:	Wed, 17 Apr 2013 21:32:42 +0400
From:	Artem Savkov <artem.savkov@...il.com>
To:	Bjorn Helgaas <bhelgaas@...gle.com>
Cc:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Li Zhong <zhong@...ux.vnet.ibm.com>,
	linux-next list <linux-next@...r.kernel.org>,
	Yinghai Lu <yinghai@...nel.org>
Subject: Re: [PATCH] PCI: pci_bus_add_device() should succeed even if
 creating sysfs files fails

On Wed, Apr 17, 2013 at 10:42:12AM -0600, Bjorn Helgaas wrote:
> On Wed, Apr 17, 2013 at 9:54 AM, Bjorn Helgaas <bhelgaas@...gle.com> wrote:
> > On Wed, Apr 17, 2013 at 5:06 AM, Artem Savkov <artem.savkov@...il.com> wrote:
> >> Introduced in cc8f7f9e4e79a0940af6b4b6fdfbcf18a03aa9f4
> >> ("PCI: Fix __must_check annotation on pci_create_sysfs_dev_files()")
> >>
> >> pci_bus_add_device() should not fail to add device if
> >> pci_create_sysfs_dev_files() call fails, as sysfs files are not critical.
> >>
> >> pci_sysfs_init() is a "late_initcall" and is called after acpi_init, so
> >> pci_bus_add_device() fails to create devices during boot resulting in a
> >> bug during acpi_init. It is safe not to create sysfs_dev_files in
> >> pci_bus_add_device() as they are later created in pci_sysfs_ini().
> >> ...
> 
> > Sorry for the breakage.  Thanks for the report, analysis, and fix.
> > I'll fix this up
> 
> It's not clear to me what the best fix is, so I just dropped my original patch.
> 
> It's clearly wrong to have __must_check on the
> pci_create_sysfs_dev_files() definition, because it doesn't do
> anything there.
> 
> We could simply drop the __must_check altogether, as you suggest, but
> I'd rather structure things to avoid the failure in the first place
> rather than ignoring an error.
> 
> It would be ideal if we could get rid of the pci_sysfs_init()
> late_initcall and just make the pci_create_sysfs_dev_files() from
> pci_bus_add_device() work all the time, even at boot-time.  I didn't
> look hard enough to figure out why this needs to be a late_initcall.
> In any case, we can figure this out later, after we merge the current
> stuff for v3.10.

It looks like late_initcall is either due to "it belongs there" or maybe
pci_create_sysfs_dev_files() was initially only called from
pci_sysfs_init() and pci subsystem should have been initialized before
this for devices to be there to add to sysfs. I've tried changing
late_initcall to subsys_initcall and didn't see any immediate problems.

Anyway even with things properly aligned it still sounds like a bad idea
to fail pci_bus_add_device() call just because creating sysfs files
fails. Some kind of warning should be enough.

-- 
Regards,
Artem
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/