Re: [PATCH v3] PCI: vmd: Only assign OSC features in bare metal OS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Flawed VM detection heuristic breaks intended OSC flag assignment in edge cases.

Pre-existing issues:
- [Medium] Memory leak of dynamically allocated SRCU structures on error paths.
- [Medium] Leak of dynamically allocated resource entries and IRQ domain on early return.
--

--- Patch [1]: [PATCH v3] PCI: vmd: Only assign OSC features in bare metal OS ---
commit 7b1a0a2085cd70ccc13f43497ed84956b2e0637c
Author: Nirmal Patel <nirmal.patel@intel.com>

PCI: vmd: Only assign OSC features in bare metal OS

Copy valid OSC flags from root bridge only when OS can provide correct
values. In virtual machine, all of the control features are disabled as
hypervisor will not passthrough root bridge information to VM.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -862,14 +863,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		.parent = res,
>  	};
>  
> +	/* Non-zero offset means guest/direct assign view. */
> +	vmd_in_guest = offset[0] || offset[1];
> +

[Severity: High]
Does this heuristic correctly account for bare-metal BAR reassignment or 
identity-mapped VMs?

If the Linux PCI subsystem reassigns the VMD device's BARs on bare metal,
for example due to resource conflicts, the offsets would become non-zero.
This would cause the driver to incorrectly identify the system as a guest 
and skip copying OSC control flags on bare metal.

Conversely, if a VM uses 1:1 identity-mapped direct assignment where the
guest physical address equals the host physical address, the offset would 
be zero. The driver would then treat the VM as bare metal, copying the 
disabled or invalid OSC flags from the virtual host bridge.

>  	/*
>  	 * Currently MSI remapping must be enabled in guest passthrough mode
>  	 * due to some missing interrupt remapping plumbing. This is probably
>  	 * acceptable because the guest is usually CPU-limited and MSI
>  	 * remapping doesn't become a performance bottleneck.
>  	 */
> -	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> -	    offset[0] || offset[1]) {
> +	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) || vmd_in_guest) {
>  		ret = vmd_alloc_irqs(vmd);

[Severity: Medium]
This is a pre-existing issue, but does vmd_alloc_irqs() leak dynamically
allocated SRCU structures on error paths?

If devm_request_irq() fails inside vmd_alloc_irqs(), the function returns
the error directly without calling cleanup_srcu_struct() on the SRCU
structures initialized in previous loop iterations or the current one.

Additionally, if vmd_enable_domain() fails and propagates an error back to
vmd_probe(), the out_release_instance error path in vmd_probe() does not
call vmd_cleanup_srcu(), which seems to leak the SRCU per-CPU data.

>  		if (ret)
>  			return ret;

[ ... ]

> @@ -910,8 +913,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		return -ENODEV;
>  	}
>  
> -	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> -				   to_pci_host_bridge(vmd->bus->bridge));
> +	/* Don't copy _OSC control flags in VM, it disables features. */
> +	if (!vmd_in_guest)
> +		vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> +					 to_pci_host_bridge(vmd->bus->bridge));

[Severity: Medium]
This is a pre-existing issue, but does vmd_enable_domain() leak dynamically
allocated resource entries and the IRQ domain on early return?

When pci_bus_find_emul_domain_nr() is called a few lines before this hunk
in vmd_enable_domain():

        sd->domain = pci_bus_find_emul_domain_nr(0, 0x10000, INT_MAX);
        if (sd->domain < 0)
                return sd->domain;

If this fails, it returns early without calling pci_free_resource_list() 
or vmd_remove_irq_domain(), stranding the memory allocated for the 
resources list and the IRQ domain created earlier in the function.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713220844.561357-1-nirmal.patel@intel.com?part=1