Re: [PATCH net v2 1/2] enic: retain admin-channel DMA until queues stop

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

Critical: 0 · High: 0 (1) · Medium: 5 (1) · Low: 5

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message understates what users will see when the
  admin channel is quarantined.
- [Medium] enic_admin_channel_close() now takes
  mutex_lock(&enic->mbox_lock), which can be held by a sender for up to…
- [Medium] The quarantine checks use different conditions.
- [Medium] enic_admin_wq_disable() and enic_admin_rq_disable() treat only
  `running == 0` as stopped.
- [Medium] The concern says the retained BAR reservation does not fence
  the device: vfio-pci or pci-stub can bind without requesting regions,…
- [Low] The patch adds early exits to enic_reset() and
  enic_tx_hang_reset(): `if (quarantined) { dev_close(); goto reset_out;…
- [Low] The probe and remove paths handle a quarantined channel
  differently.
- [Low] The commit message does not describe several behaviour changes
  that apply even when no admin queue ever times out: (1) a new `if…
- [Low] The new comment in the quarantine branch of enic_reset() says
  "Core close excludes netpoll and clears IFF_UP before ndo_stop." That…
- [Low] Two problems around mbox_tx_poisoned.

Pre-existing issues:
- [High] This bug was not introduced by this patch, but the patch
  rewrites the exact line involved and leaves it in place.
- [Medium] This problem wasn't introduced by this patch, but: enic_stop()
  returns early when vnic_wq_disable() or vnic_rq_disable() on a…

--

enic: retain admin-channel DMA until queues stop

This patch tracks the enic admin channel as DOWN, LIVE or QUARANTINED. When
an admin WQ or RQ disable times out, the admin allocations are now kept
instead of freed. The reset paths close the netdev rather than reopen a
quarantined channel, and enic_open() refuses to open a quarantined V2 VF.
On remove, a final queue recheck is made. If that recheck fails, the
netdev, vnic_dev, admin allocations and BAR reservation are kept until
reboot.

[ ... ]

[ ... ]

[Severity: Low]
Is this comment accurate? enic_admin_channel_close() also sets
mbox_tx_poisoned whenever an admin WQ or RQ disable times out, even if no
send is pending:

	if (wq_err || rq_err) {
		WRITE_ONCE(enic->admin_chan_state, ENIC_ADMIN_CHAN_QUARANTINED);
		WRITE_ONCE(enic->mbox_tx_poisoned, true);

A related question: can the new -EIO check in enic_admin_channel_open()
ever be reached?

The flag is only set while the state is LIVE (send timeout) or
QUARANTINED (close). Both release paths set the state to DOWN and clear
the flag under mbox_lock. That means this earlier check always returns
first:

	if (READ_ONCE(enic->admin_chan_state) != ENIC_ADMIN_CHAN_DOWN) {
		...
		return -EBUSY;
	}

The only way to hit "Refusing to reopen poisoned admin channel" is an open
that races a release, between WRITE_ONCE(admin_chan_state, DOWN) in
enic_admin_channel_release() and the WRITE_ONCE(mbox_tx_poisoned, false)
that follows it. In that case the refusal would be spurious.

[ ... ]

[ ... ]

[ ... ]

[Severity: Medium]
What happens here if the function is gone, for example after a surprise
hot-unplug, an AER or link-down event, or a host-side PF reset seen by a
guest? ioread32() on the running register would return 0xFFFFFFFF. The
!running condition then never holds, and read_poll_timeout() returns
-ETIMEDOUT. enic_admin_rq_disable() has the same problem.

Elsewhere the driver already treats an all-ones read from this control
block as hardware gone, for example in vnic_rq_clean():

	if (fetch_index == 0xFFFFFFFF) { /* check for hardware gone  */

With this patch, that read leads to a decision that cannot be undone:

- enic_admin_channel_close() sets QUARANTINED and mbox_tx_poisoned.
- enic_admin_channel_recheck_on_remove() fails the same way.
- enic_remove() takes the retain path. It keeps the pci_dev reference,
  netdev, vnic_dev, admin allocations and DMA mappings for a device that
  cannot be doing DMA.

Before the patch, the same read only produced a warning and normal
teardown.

Each pass also now waits 500 ms instead of about 10 ms. A surprise-removed
VF therefore spends several seconds under rtnl and mbox_lock, plus the 5 s
unregister send poll, before its resources are leaked.

Could an all-ones value be treated as device gone in these helpers?

[ ... ]

[ ... ]

[Severity: Medium]
How long can rtnl be held across this?

mutex_lock(&enic->mbox_lock) can wait up to ENIC_MBOX_POLL_TIMEOUT_US (5 s)
for an in-flight sender. After that, the sleeping polls run with the mutex
held:

- WQ: up to 500 ms, plus one retry.
- RQ: two 500 ms passes, plus one retry.

That is roughly 3 s per close, compared with about 10 ms before.

enic_reset() and enic_tx_hang_reset() hold rtnl across this close.
enic_remove() now also takes rtnl around both the VF unregister and this
close:

		rtnl_lock();
		netif_device_detach(netdev);
		...
				int unreg_err = enic_mbox_vf_unregister(enic);
		...
			enic_admin_channel_close(enic);
		}
		rtnl_unlock();

enic_mbox_vf_unregister() can spend up to 5 s in the send poll plus a
3000 ms reply wait. A device that misbehaves can therefore hold the global
RTNL for more than 10 s during a single remove.

Does rtnl need to cover more than netif_device_detach() in enic_remove()?

The commit message only says "Poll for up to 500 ms per pass" and gives no
total. Could it state the worst case?

[ ... ]

[Severity: Medium]
The commit message says:

  Keep bus mastering enabled at runtime so the independent datapath and
  command transport remain usable.

Is that accurate for a V2 VF? Once the channel is QUARANTINED, this check
makes every enic_open() return -EBUSY. The only way out of QUARANTINED is
a successful enic_admin_channel_recheck_on_remove(). Until reboot,
"ip link set up" keeps failing.

enic_reset() and enic_tx_hang_reset() also call dev_close() as soon as
they see quarantine. That happens either right after
enic_admin_channel_close() or after enic_admin_chan_reopen() fails.

For a PF, every later soft or hang reset becomes a plain dev_close(), and
the hardware reset is skipped.

Could the commit message describe these effects? The patch carries a
Fixes: tag and may be picked up for stable.

[ ... ]

[Severity: Low]
Is the ordering in this comment right? __dev_close_many() clears
__LINK_STATE_START before ndo_stop. IFF_UP is only cleared after ndo_stop
returns:

		if (ops->ndo_stop)
			ops->ndo_stop(dev);

		netif_rx_mode_cancel_retry(dev);
		netif_set_up(dev, false);

So IFF_UP is still set while enic_stop() runs from this dev_close().

[Severity: Medium]
Should this check also be limited to enic_is_sriov_vf_v2()?

The close above runs under enic_has_admin_chan(), which also covers a PF
with SR-IOV V2 enabled. For a quarantined PF, this branch calls dev_close()
and skips enic_dev_soft_reset().

The check after reopen further down is VF-only, and so is the gate in
enic_open():

	if (enic_is_sriov_vf_v2(enic) &&
	    enic_admin_channel_is_quarantined(enic))
		return -EBUSY;

So userspace can bring the PF straight back up on hardware that never got
its reset. enic_admin_channel_close() keeps returning -EBUSY and the state
stays QUARANTINED, so each later reset request becomes another dev_close().
A PF hang then never gets recovered.

It also goes the other way. If enic_admin_chan_reopen() ends up
quarantining the channel, a VF is closed but a PF keeps its datapath open.

The same unqualified check is in enic_tx_hang_reset(). This is latent
today because .sriov_configure is not wired up, so a PF never opens the
admin channel.

[Severity: High]
This isn't a bug introduced by this patch, but since this line is being
rewritten: what state is the netdev left in when enic_open() fails here?

IFF_UP and __LINK_STATE_START are still set. enic_open() has already
undone its own work: enic_free_intr(), enic_dev_notify_unset(), and the
page pools destroyed. The NAPI instances are still disabled from the
enic_stop() call earlier in the reset.

The next ndo_stop runs enic_stop() again. That can come from "ip link set
down", or from unregister_netdev() in enic_remove(), since
__dev_close_many() allows ndo_stop after a DETACH:

enic_stop()
    napi_disable(&enic->napi[i])
        napi_disable_locked()
            while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
                usleep_range(20, 200);
                val = READ_ONCE(n->state);
            }

A napi_disable() that has completed leaves SCHED and NPSVC set. Can this
loop spin forever with rtnl held?

Even if the loop were passed, enic_free_intr() would call free_irq() a
second time, because msix[i].requested is never cleared on free.

Any of these failures in enic_open() can trigger this:

- page_pool_create() returning ENOMEM
- vnic_rq_fill() producing no buffers
- enic_dev_notify_set() failing
- request_irq() failing
- enic_dev_enable() failing

The quarantine branch above calls dev_close(). Should this open-failure
branch do the same? enic_tx_hang_reset() has the same pattern.

[Severity: Low]
This exit and the quarantine exit above both skip
enic_admin_chan_reopen(). On the reset path, that is the only place that
clears vf_registered:

	if (enic_is_sriov_vf_v2(enic))
		WRITE_ONCE(enic->vf_registered, false);

According to its comment, this stops the VF from believing it still has a
usable PF registration over a dead channel. On these exits, vf_registered
stays true while the channel is DOWN or QUARANTINED and
mbox_send_disabled is set.

Later, at remove or in the probe err_out_admin_close path,
enic_mbox_vf_unregister() sends a request that is rejected with -ENODEV.
The driver then logs a misleading "Failed to unregister from PF: -19"
warning.

Should vf_registered also be cleared before these early exits? The same
applies to enic_tx_hang_reset().

[ ... ]

[Severity: Low]
This isn't a bug, but the commit message does not mention several changes
that apply even when no admin queue ever times out:

- This new netif_running() check, and the one added under rtnl in
  enic_reset(). A queued TX-hang reset is now dropped if the interface has
  gone down.
- The netif_device_present() check at the top of enic_open(), which
  applies to every enic device.
- The new rtnl_lock()/netif_device_detach() section in enic_remove(),
  which wraps the VF unregister request/reply wait and the admin close.

Could the commit message describe these? That would let stable
backporters tell them apart from the DMA-lifetime fix.

[Severity: Medium]
This is a pre-existing issue, but the new dev_close() unwind here and in
enic_reset() depends on enic_stop() running to completion. enic_stop()
returns early when a datapath queue disable times out:

	for (i = 0; i < enic->wq_count; i++) {
		err = vnic_wq_disable(&enic->wq[i].vwq);
		if (err)
			return err;
	}

That early return skips enic_dev_notify_unset(), enic_unset_affinity_hint(),
enic_free_intr() and the ring and page pool cleanup.

dev_close() clears __LINK_STATE_START and IFF_UP whatever ndo_stop
returns. unregister_netdev() then never calls enic_stop() again, so the
datapath irqactions and page pools appear to be leaked.

The commit message says it "does not repair the separate generic datapath
queue-disable error path". Quarantine requires MSI-X, and
enic_dev_deinit()->enic_clear_intr_mode() tears down the vectors, so this
looks like a leak rather than a use-after-free. Before this patch, the same
failure left the device running and led to re-entering enic_stop().

[ ... ]

[Severity: Low]
Should probe also call enic_admin_channel_recheck_on_remove() before it
decides to retain? enic_remove() does:

		if (enic_admin_channel_is_quarantined(enic))
			enic_admin_channel_recheck_on_remove(enic);

This path retains straight away, even though enic_dev_deinit() and
vnic_dev_close() (CMD_CLOSE) have just run and may have stopped the
queues.

Quarantine during probe can come from the partial-open unwind, a
capability-check failure or a registration failure. In each case the
netdev, vnic_dev, admin graph and BAR reservation are kept, and rebind is
blocked until reboot. Close has already polled for about 3 s at this
point, so a recheck would probably rarely change the outcome.

[ ... ]

[ ... ]

[Severity: Medium]
Does the retained BAR reservation keep the device away from a later owner?
vfio-pci or pci-stub can bind without requesting regions. Remove/rescan or
kexec drops the reservation and probes again, and enic_probe() then does:

	err = pci_request_regions(pdev, dev_name(dev));
	...
	pci_set_master(pdev);

There is no function reset first. Could an admin queue that is still
running then DMA through stale IOVAs into memory that now has a new owner?

Most of this looks covered today:

- Both retain paths call pci_clear_master() first.
- vfio-pci resets the function before userspace can turn bus mastering
  back on, and pci-stub never sets it.
- A V2 VF cannot be re-enumerated without toggling VF Enable, which
  resets it.
- The commit message limits the guarantee to "while the PCI device
  remains enumerated".

The remaining case seems to be a future PF quarantine followed by PF
remove/rescan and a probe without a reset. It also depends on the hardware
resuming queue DMA once bus mastering is set again.

Would a function reset before pci_set_master() in that case, or a
documented limitation, be worth considering?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v2-0-8db19b4a714e%40cisco.com