Re: [PATCH v2] PCI: endpoint: pci-epf-vntb: Track link state from both sides

[sashiko-bot@kernel.org]

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

New issues:
- [High] Internal state is not rolled back if the link notification fails.

Pre-existing issues:
- [High] Missing ntb_unregister_device() causes NTB device leak and subsequent Use-After-Free.
- [High] Virtual PCI root bus allocated in vpci_scan_bus() is never removed.
- [High] Missing dma_rmb() between reading `command` and `argument` from the shared DMA memory.
--

commit 31e5bb87c3524261d2384c7e27a492debb83840b
Author: Koichiro Den <den@valinux.co.jp>

    PCI: endpoint: pci-epf-vntb: Track link state from both sides

    The control-region link status is currently updated only by
    COMMAND_LINK_UP and COMMAND_LINK_DOWN from the HOST. The virtual
    NTB link callbacks are empty. Consequently, the HOST can see the link as
    up before the VHOST has enabled it, and ntb_link_disable() on the VHOST
    leaves LINK_STATUS_UP set without notifying either NTB client.

Link: https://lore.kernel.org/r/20260904065335.3059625-1-den@valinux.co.jp/

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index fba65abfb6b21..f921c7c7019aa 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
[ ... ]
> @@ -178,24 +182,57 @@ static struct pci_epf_header epf_ntb_header = {
[ ... ]
>  static int epf_ntb_link_up(struct epf_ntb *ntb, bool link_up)
>  {
> -	if (link_up)
> -		ntb->reg->link_status |= LINK_STATUS_UP;
> -	else
> -		ntb->reg->link_status &= ~LINK_STATUS_UP;
> +	bool notify;
> +
> +	scoped_guard(mutex, &ntb->link_lock) {
> +		notify = ntb->host_linkup != link_up && ntb->vhost_linkup;
> +		ntb->host_linkup = link_up;
> +		epf_ntb_update_link(ntb);
> +	}
> +
> +	if (notify) {
> +		/* Publish link status before completing the HOST command. */
> +		dma_wmb();
> +		return epf_ntb_notify_link(ntb);
> +	}

[Severity: High]
If epf_ntb_notify_link() fails (e.g. failing to raise the MSI IRQ), does the
internal state machine become permanently desynchronized? The effective link
state (host_linkup) is updated in the locked section, but there is no
rollback on failure.

When the host retries the command, would notify evaluate to false, skipping
the notification entirely and leaving the event swallowed?

[ ... ]

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_cmd_handler() {
    ...
	ctrl = ntb->reg;
	command = ctrl->command;
	if (!command)
		goto reset_handler;
	argument = ctrl->argument;
    ...
}

[Severity: High]
This is a pre-existing issue, but does missing a dma_rmb() between reading
the command and the argument allow the CPU to fetch a stale argument for a
newly issued command?

Since the CPU can reorder reads, can it fetch a stale argument from the
shared DMA memory? Because the argument is used without bounds checking in
operations like COMMAND_CONFIGURE_MW, could reading a stale (and potentially
large) argument cause an out-of-bounds array access (ntb->vpci_mw_phy[argument])
leading to a kernel panic?


drivers/pci/endpoint/functions/pci-epf-vntb.c:pci_vntb_probe() {
    ...
	ret = ntb_register_device(&ndev->ntb);
	if (ret) {
    ...
}

static struct pci_driver vntb_pci_driver = {
	.name           = "pci-vntb",
	.id_table       = pci_vntb_table,
	.probe          = pci_vntb_probe,
};

[Severity: High]
This is a pre-existing issue, but does vntb_pci_driver lack a .remove callback
to unregister the NTB device?

Is the NTB device never unregistered from the NTB core during device teardown?
If the parent EPF device is unbound and devm-allocated memory is freed, could
this lead to a use-after-free because ntb_unregister_device() is skipped?


drivers/pci/endpoint/functions/pci-epf-vntb.c:vpci_scan_bus() {
    ...
	vpci_bus = pci_scan_bus(ndev->vbus_number, &vpci_ops, sysdata);
	if (!vpci_bus) {
		pr_err("create pci bus failed\n");
		return -EINVAL;
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but is the virtual PCI root bus allocated in
vpci_scan_bus() ever removed?

Does this lead to a resource leak and use-after-free? When the EPF is unbound,
it appears pci_remove_root_bus() is never called, which would leave the virtual
PCI root bus registered with a dangling sysdata pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904162049.3432464-1-den@valinux.co.jp?part=1