linux-kernel - Re: [patch 02/10] genirq/msi: Use lock guards for MSI descriptor locking

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: <20250311180017.00003fcc@huawei.com>
Date: Tue, 11 Mar 2025 18:00:17 +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>,
	Nishanth Menon <nm@...com>, Tero Kristo <kristo@...nel.org>, "Santosh
 Shilimkar" <ssantosh@...nel.org>, Jon Mason <jdmason@...zu.us>, Dave Jiang
	<dave.jiang@...el.com>, Allen Hubbe <allenbh@...il.com>,
	<ntb@...ts.linux.dev>, Bjorn Helgaas <bhelgaas@...gle.com>,
	<linux-pci@...r.kernel.org>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu
	<wei.liu@...nel.org>, <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>, Dan Williams
	<dan.j.williams@...el.com>
Subject: Re: [patch 02/10] genirq/msi: Use lock guards for MSI descriptor
 locking

On Sun,  9 Mar 2025 09:41:44 +0100 (CET)
Thomas Gleixner <tglx@...utronix.de> wrote:

> Provide a lock guard for MSI descriptor locking and update the core code
> accordingly.
> 
> No functional change intended.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
One case in here made me go back to the helpful docs Dan wrote
after early confusion on how to use this stuff.  We might
want to consider updating as you've found a case that I at least
didn't consider when reviewing those.  +CC Dan.

Anyhow, this seems fine to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>


>  
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -443,7 +443,6 @@ EXPORT_SYMBOL_GPL(msi_next_desc);

>  
> @@ -1037,25 +1032,23 @@ bool msi_create_device_irq_domain(struct
>  	if (msi_setup_device_data(dev))

Hmm. We might want to make the docs in cleanup.h more nuanced.
They specifically say to not mix goto and auto cleanup, but 
in the case of scoped_guard() unlikely almost any other case
it should be fine.

>  		goto free_fwnode;
>  
> -	msi_lock_descs(dev);
> -
> -	if (WARN_ON_ONCE(msi_get_device_domain(dev, domid)))
> -		goto fail;
> -
> -	if (!pops->init_dev_msi_info(dev, parent, parent, &bundle->info))
> -		goto fail;
> -
> -	domain = __msi_create_irq_domain(fwnode, &bundle->info, IRQ_DOMAIN_FLAG_MSI_DEVICE, parent);
> -	if (!domain)
> -		goto fail;
> -
> -	domain->dev = dev;
> -	dev->msi.data->__domains[domid].domain = domain;
> -	msi_unlock_descs(dev);
> -	return true;
> +	scoped_guard(msi_descs_lock, dev) {
> +		if (WARN_ON_ONCE(msi_get_device_domain(dev, domid)))
> +			goto free_fwnode;
> +
> +		if (!pops->init_dev_msi_info(dev, parent, parent, &bundle->info))
> +			goto free_fwnode;
> +
> +		domain = __msi_create_irq_domain(fwnode, &bundle->info, IRQ_DOMAIN_FLAG_MSI_DEVICE,
> +						 parent);
> +		if (!domain)
> +			goto free_fwnode;
> +
> +		domain->dev = dev;
> +		dev->msi.data->__domains[domid].domain = domain;
> +		return true;
> +	}
>  
> -fail:
> -	msi_unlock_descs(dev);
>  free_fwnode:
>  	irq_domain_free_fwnode(fwnalloced);
>  free_bundle: