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: <20231019193246.GA16112@wunner.de>
Date:   Thu, 19 Oct 2023 21:32:46 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Bjorn Helgaas <helgaas@...nel.org>
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 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.


> > +		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".
> 
> Suggest lower-case "%04lx:%02lx" either way.
> 
> FWIW, there's no "0x" prefix on the hex vendor IDs in "lspci -n"
> output and dmesg messages like this:
> 
>   pci 0000:01:00.0: [10de:13b6] type 00

The existing attributes "vendor", "device" etc do emit the "0x".

>From drivers/pci/pci-sysfs.c:

pci_config_attr(vendor, "0x%04x\n");
pci_config_attr(device, "0x%04x\n");
pci_config_attr(subsystem_vendor, "0x%04x\n");
pci_config_attr(subsystem_device, "0x%04x\n");
pci_config_attr(revision, "0x%02x\n");
pci_config_attr(class, "0x%06x\n");


> 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?

Thanks,

Lukas