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

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

[Koichiro Den]

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.

ntb_netdev can hide this device-level state bug because ntb_transport uses
a separate per-QP LINK_DOWN_FLAG message. Use ntb_tool to observe the
device link state directly:

After bringing both sides up, start one waiter on each side:

  HOST# echo N > /sys/kernel/debug/ntb_tool/<H-device>/peer0/link_event
  VHOST# echo N > /sys/kernel/debug/ntb_tool/<V-device>/peer0/link_event

While both waiters are blocked, disable the link on the VHOST:

  VHOST# echo N > /sys/kernel/debug/ntb_tool/<V-device>/link

Without this patch, the link-disable command succeeds, but peer0/link
remains Y on both sides and neither waiter returns.

Track HOST and VHOST enablement separately and report the effective link
as up only when both sides have enabled it, as pci-epf-ntb does for its
two physical hosts. Both NTB clients observe this effective state, so
notify both sides only when it changes, including the side whose request
completes the transition. Serialize the two paths because HOST commands
run from delayed work while the VHOST callbacks may run concurrently.

Order a link-status change caused by a HOST command before publishing
command completion.

With this patch, peer0/link changes to N on both sides and both waiters
return.

Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - Notify both NTB clients when the effective link state changes. (Sashiko)
  - Use devm_mutex_init(). (Frank)
  - Update the commit message + reproducer accordingly.

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

 drivers/pci/endpoint/functions/pci-epf-vntb.c | 92 +++++++++++++++----
 1 file changed, 75 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
index fba65abfb6b2..f921c7c7019a 100644
--- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
@@ -41,6 +41,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/slab.h>
 
 #include <linux/pci-ep-msi.h>
@@ -147,7 +148,10 @@ struct epf_ntb {
 	u16 vntb_pid;
 	u16 vntb_vid;
 
-	bool linkup;
+	/* Serialize HOST and VHOST link state changes. */
+	struct mutex link_lock;
+	bool host_linkup;
+	bool vhost_linkup;
 
 	/*
 	 * True when doorbells are interrupt-driven (MSI or embedded), false
@@ -178,24 +182,57 @@ static struct pci_epf_header epf_ntb_header = {
 	.interrupt_pin	= PCI_INTERRUPT_INTA,
 };
 
+static void epf_ntb_update_link(struct epf_ntb *ntb)
+{
+	u16 link_status = READ_ONCE(ntb->reg->link_status);
+
+	/* The link is usable only after both sides have enabled it. */
+	if (ntb->host_linkup && ntb->vhost_linkup)
+		link_status |= LINK_STATUS_UP;
+	else
+		link_status &= ~LINK_STATUS_UP;
+
+	WRITE_ONCE(ntb->reg->link_status, link_status);
+}
+
+static int epf_ntb_notify_link(struct epf_ntb *ntb)
+{
+	struct pci_epf *epf = ntb->epf;
+	int ret;
+
+	ntb_link_event(&ntb->ntb);
+
+	ret = pci_epc_raise_irq(epf->epc, epf->func_no, epf->vfunc_no,
+				PCI_IRQ_MSI, EPF_IRQ_LINK + 1);
+	if (ret)
+		dev_err(&epf->dev, "Failed to raise link event IRQ: %d\n", ret);
+
+	return ret;
+}
+
 /**
- * epf_ntb_link_up() - Raise link_up interrupt to Virtual Host (VHOST)
+ * epf_ntb_link_up() - Update the HOST link state
  * @ntb: NTB device that facilitates communication between HOST and VHOST
- * @link_up: true or false indicating Link is UP or Down
- *
- * Once NTB function in HOST invoke ntb_link_enable(),
- * this NTB function driver will trigger a link event to VHOST.
+ * @link_up: true when the HOST has enabled the link
  *
  * Returns: Zero for success, or an error code in case of failure
  */
 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);
+	}
 
-	ntb_link_event(&ntb->ntb);
 	return 0;
 }
 
@@ -320,7 +357,6 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 		ctrl->command_status = COMMAND_STATUS_OK;
 		break;
 	case COMMAND_LINK_UP:
-		ntb->linkup = true;
 		ret = epf_ntb_link_up(ntb, true);
 		if (ret < 0)
 			ctrl->command_status = COMMAND_STATUS_ERROR;
@@ -328,7 +364,6 @@ static void epf_ntb_cmd_handler(struct work_struct *work)
 			ctrl->command_status = COMMAND_STATUS_OK;
 		goto reset_handler;
 	case COMMAND_LINK_DOWN:
-		ntb->linkup = false;
 		ret = epf_ntb_link_up(ntb, false);
 		if (ret < 0)
 			ctrl->command_status = COMMAND_STATUS_ERROR;
@@ -1456,11 +1491,27 @@ static int vntb_epf_peer_mw_get_addr(struct ntb_dev *ndev, int idx,
 	return 0;
 }
 
+static int vntb_epf_set_link(struct epf_ntb *ntb, bool link_up)
+{
+	bool notify;
+
+	scoped_guard(mutex, &ntb->link_lock) {
+		notify = ntb->vhost_linkup != link_up && ntb->host_linkup;
+		ntb->vhost_linkup = link_up;
+		epf_ntb_update_link(ntb);
+	}
+
+	if (!notify)
+		return 0;
+
+	return epf_ntb_notify_link(ntb);
+}
+
 static int vntb_epf_link_enable(struct ntb_dev *ntb,
 			enum ntb_speed max_speed,
 			enum ntb_width max_width)
 {
-	return 0;
+	return vntb_epf_set_link(ntb_ndev(ntb), true);
 }
 
 static u32 vntb_epf_spad_read(struct ntb_dev *ndev, int idx)
@@ -1620,7 +1671,7 @@ static u64 vntb_epf_link_is_up(struct ntb_dev *ndev,
 {
 	struct epf_ntb *ntb = ntb_ndev(ndev);
 
-	return ntb->reg->link_status;
+	return READ_ONCE(ntb->reg->link_status);
 }
 
 static int vntb_epf_db_clear_mask(struct ntb_dev *ndev, u64 db_bits)
@@ -1638,7 +1689,7 @@ static int vntb_epf_db_clear(struct ntb_dev *ndev, u64 db_bits)
 
 static int vntb_epf_link_disable(struct ntb_dev *ntb)
 {
-	return 0;
+	return vntb_epf_set_link(ntb_ndev(ntb), false);
 }
 
 static struct device *vntb_epf_get_dma_dev(struct ntb_dev *ndev)
@@ -1750,6 +1801,10 @@ static int epf_ntb_bind(struct pci_epf *epf)
 		goto err_bar_alloc;
 	}
 
+	ntb->host_linkup = false;
+	ntb->vhost_linkup = false;
+	ntb->reg->link_status = 0;
+
 	ret = epf_ntb_epc_init(ntb);
 	if (ret) {
 		dev_err(dev, "Failed to initialize EPC\n");
@@ -1822,7 +1877,7 @@ static int epf_ntb_probe(struct pci_epf *epf,
 {
 	struct epf_ntb *ntb;
 	struct device *dev;
-	int i;
+	int ret, i;
 
 	dev = &epf->dev;
 
@@ -1833,6 +1888,9 @@ static int epf_ntb_probe(struct pci_epf *epf,
 	epf->header = &epf_ntb_header;
 	ntb->epf = epf;
 	ntb->vbus_number = 0xff;
+	ret = devm_mutex_init(dev, &ntb->link_lock);
+	if (ret)
+		return ret;
 
 	INIT_WORK(&ntb->peer_db_work, vntb_epf_peer_db_work);
 	disable_work(&ntb->peer_db_work);

base-commit: 68fcdf30a1582574fe67e0b02488f8ba0d4d1242
-- 
2.51.0

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

[sashiko-bot]

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

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

[Koichiro Den]

On Fri, Sep 04, 2026 at 04:33:54PM +0000, sashiko-bot@kernel.org wrote:
> 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?

Hm, the concern seems valid to some extent, but I don't think simply rolling
back the link state is the right fix. The VHOST client has already received
ntb_link_event() by the time raising the MSI IRQ fails.

pci-epf-ntb also keeps the updated state when raising an IRQ fails, but it
allows the notification to be retried. This v2 implementation does not, because
the local 'notify' variable evaluates to false on the retry. If the first 'echo
N > link' fails because the MSI IRQ could not be raised, repeating it succeeds
without retrying the notification, unlike pci-epf-ntb.

I think I should make such a retry possible, in line with pci-epf-ntb.
If anyone has a better idea, any comments would be appreciated.

Best regards,
Koichiro

> 
> [ ... ]
> 
> 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