linux-kernel - Re: [patch V2 05/10] PCI/MSI: Switch to MSI descriptor locking to guard()

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: <20250313155015.000037f5@huawei.com>
Date: Thu, 13 Mar 2025 15:50:15 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Thomas Gleixner <tglx@...utronix.de>
CC: LKML <linux-kernel@...r.kernel.org>, Marc Zyngier <maz@...nel.org>, Bjorn
 Helgaas <bhelgaas@...gle.com>, <linux-pci@...r.kernel.org>, "Peter Zijlstra"
	<peterz@...radead.org>, Nishanth Menon <nm@...com>, Dhruva Gole
	<d-gole@...com>, Tero Kristo <kristo@...nel.org>, Santosh Shilimkar
	<ssantosh@...nel.org>, Logan Gunthorpe <logang@...tatee.com>, Dave Jiang
	<dave.jiang@...el.com>, Jon Mason <jdmason@...zu.us>, Allen Hubbe
	<allenbh@...il.com>, <ntb@...ts.linux.dev>, Michael Kelley
	<mhklinux@...look.com>, Wei Liu <wei.liu@...nel.org>, Haiyang Zhang
	<haiyangz@...rosoft.com>, <linux-hyperv@...r.kernel.org>, Wei Huang
	<wei.huang2@....com>, Manivannan Sadhasivam
	<manivannan.sadhasivam@...aro.org>, "James E.J. Bottomley"
	<James.Bottomley@...senPartnership.com>, "Martin K. Petersen"
	<martin.petersen@...cle.com>, <linux-scsi@...r.kernel.org>, Jonathan Cameron
	<Jonathan.Cameron@...ei.com>
Subject: Re: [patch V2 05/10] PCI/MSI: Switch to MSI descriptor locking to
 guard()

On Thu, 13 Mar 2025 14:03:44 +0100 (CET)
Thomas Gleixner <tglx@...utronix.de> wrote:

> Convert the code to use the new guard(msi_descs_lock).
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: Bjorn Helgaas <bhelgaas@...gle.com>
> Cc: linux-pci@...r.kernel.org
> ---
> V2: Remove the gotos - Jonathan
Hi Thomas,

There is a bit of the original code that is carried forwards here
that superficially seemed overly complex.  However as far as I can tell
this is functionally the same as you intended.  So with that in mind
if my question isn't complete garbage, maybe a readability issue for
another day.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>

> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c

> +static int msix_setup_interrupts(struct pci_dev *dev, struct msix_entry *entries,
> +				 int nvec, struct irq_affinity *affd)
> +{
> +	struct irq_affinity_desc *masks __free(kfree) =
> +		affd ? irq_create_affinity_masks(nvec, affd) : NULL;
> +
> +	guard(msi_descs_lock)(&dev->dev);
> +	int ret = __msix_setup_interrupts(dev, entries, nvec, masks);
> +	if (ret)
> +		pci_free_msi_irqs(dev);

It's not immediately obvious what this is undoing (i.e. where the alloc
is).  I think it is at least mostly the pci_msi_setup_msi_irqs in
__msix_setup_interrupts

Why not handle the error in __msix_setup_interrupts and make that function
side effect free.  Does require gotos but in a function that isn't
doing any cleanup magic so should be fine.

Mind you I'm not following the logic in msix_setup_interrupts()
before this series either. i.e. why doesn't msix_setup_msi_descs()
clean up after itself on failure (i.e. undo loop iterations that
weren't failures) as that at least superficially looks like it
would give more readable code.

So this is the same as current and as such the patch is fine I think.

>  	return ret;
>  }
>