linux-kernel - Re: [PATCH v9 2/3] PCI/DOE: Expose the DOE features via sysfs

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: <20231019200110.GA1410324@bhelgaas>
Date:   Thu, 19 Oct 2023 15:01:10 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     Alistair Francis <alistair23@...il.com>, bhelgaas@...gle.com,
        linux-pci@...r.kernel.org, Jonathan.Cameron@...wei.com,
        alex.williamson@...hat.com, christian.koenig@....com,
        kch@...dia.com, gregkh@...uxfoundation.org, logang@...tatee.com,
        linux-kernel@...r.kernel.org, chaitanyak@...dia.com,
        rdunlap@...radead.org, Alistair Francis <alistair.francis@....com>
Subject: Re: [PATCH v9 2/3] PCI/DOE: Expose the DOE features via sysfs

On Thu, Oct 19, 2023 at 09:32:46PM +0200, Lukas Wunner wrote:
> On Thu, Oct 19, 2023 at 11:58:29AM -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 13, 2023 at 01:41:57PM +1000, Alistair Francis wrote:
> > > +	xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > > +		xa_for_each(&doe_mb->feats, j, entry)
> > > +			return a->mode;
> > > +	}
> > > +
> > > +	return 0;
> > 
> > The nested loops that don't test anything look a little weird and
> > maybe I'm missing something, but this looks like it returns a->mode if
> > any mailbox with a feature exists, and 0 otherwise.
> > 
> > Is that the same as this:
> > 
> >   if (pdev->doe_mbs)
> >     return a->mode;
> > 
> >   return 0;
> > 
> > since it sounds like a mailbox must support at least one feature?
> 
> In theory it's the same, in practice there *might* be non-compliant
> devices which lack support for the discovery feature.

Is there any point in setting ->doe_mbs if there's no feature?

> > > +		attrs[i].attr.name = kasprintf(GFP_KERNEL,
> > > +					       "0x%04lX:%02lX", vid, type);
> > 
> > What's the rationale for using "0x" on the vendor ID but not on the
> > type?  "0x1234:10" hints that the "10" might be decimal since it lacks
> > "0x".

This is my main question.  Seems like it should be both or neither.

> > I try hard to avoid calling *anything* from the
> > pci_create_sysfs_dev_files() path because it has the nasty
> > "sysfs_initialized" check and the associated pci_sysfs_init()
> > initcall.
> 
> What's the purpose of sysfs_initialized anyway?
> 
> It was introduced by this historic commit:
> https://git.kernel.org/tglx/history/c/f6d553444da2
> 
> Can PCI_ROM_RESOURCEs appear after device enumeration but before
> the late_initcall stage?
> 
> If sysfs_initialized is only needed for PCI_ROM_RESOURCEs, can we
> constrain pci_sysfs_init() to those and avoid creating all the
> other runtime sysfs attributes in the initcall?

I think pci_sysfs_init() is already constrained to only the BARs and
ROM.  Constraining it to only the ROM would be an improvement, but I'd
really like to get rid of it altogether.  Krzysztof W. moved a lot of
stuff out of pci_sysfs_init() a while ago, but the BARs are harder
because of some arch/alpha wrinkles, IIRC.

I think the reason for pci_sysfs_init() exists in the first place is
because those resources may be assigned after pci_device_add(), and
(my memory is hazy here) it seems like changing the size of binary
attributes is hard, which might fit with the
pci_remove_resource_files() and pci_create_resource_files() in the
resource##n##_resize_store() macro:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-sysfs.c?id=v6.5#n1440

Bjorn