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

[netdev-bot+sashiko@kernel.org]

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

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

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] Surprise removal of a V2 VF now permanently retains the
  driver's objects, and each removal cycle leaks a new set.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  edits the exact lines involved and leaves it in place.

--

enic: retain admin-channel DMA until queues stop

This commit tracks the SR-IOV admin channel as DOWN, LIVE or QUARANTINED.
The admin WQ/RQ/CQ DMA allocations are freed only after both queues report
RUNNING == 0; otherwise the channel is quarantined, reset workers close the
netdev, and probe unwind and removal keep the netdev, vnic_dev and admin
allocations until reboot. It also checks the enic_open() return value in
the reset workers and detaches the netdev during removal.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index 61c82b48044d0..8a063c563a41e 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c

[ ... ]

> @@ -600,60 +608,51 @@ int enic_admin_channel_open(struct enic *enic)

[ ... ]

> +static int enic_admin_wq_disable(struct enic *enic)
>  {
> -	int err;
> +	struct vnic_wq *wq = &enic->admin_wq;
> +	u32 running;
>  

[ ... ]

> +	iowrite32(0, &wq->ctrl->enable);
> +	return read_poll_timeout(ioread32, running, !running, 10, 500000,
> +				 false, &wq->ctrl->running);
> +}

[Severity: Medium]
What happens here after a surprise removal of the VF, for example through
pciehp or when the VF is pulled out from under a guest? MMIO reads then
return 0xffffffff, so !running never becomes true and read_poll_timeout()
returns -ETIMEDOUT on every pass. enic_admin_rq_disable() behaves the same
way.

On removal the sequence looks like this:

enic_remove()
  enic_admin_channel_close()
    enic_admin_qp_type_set(QP_DISABLE)       fails fast
    enic_admin_wq_disable()/rq_disable()     time out, retried once
    admin_chan_state = ENIC_ADMIN_CHAN_QUARANTINED
  unregister_netdev(), enic_dev_deinit(), vnic_dev_close()
  enic_admin_channel_final_recheck()         times out again
  quarantine branch:
      pci_dev_get(pdev);
      pci_clear_master(pdev);
      pci_disable_device(pdev);
      pci_set_drvdata(pdev, NULL);
      ...
      return;

That return skips vnic_dev_unregister(), enic_iounmap(),
pci_release_regions() and free_netdev(). Does every surprise removal of a
V2 VF now keep the following until reboot?

  - the netdev
  - the vnic_dev and its coherent buffers
  - the admin rings and RQ buffers
  - a pinned pci_dev

Removal also gets a few seconds of extra polling. Before this patch, all
of these were freed.

The kept BAR reservation also doesn't seem to block a re-added device.
pci_destroy_dev()->pci_free_resources() detaches the old resource from the
iomem tree. A device re-enumerated in the same slot could then probe a new
enic instance and leak another set of objects on its next removal.

The probe unwind at err_out_vnic_unregister behaves the same way if the
device disappears during probe.

The commit message says:

  An all-ones MMIO read or a recoverable PCI channel-offline state is not
  accepted as queue-stop proof.

However, nothing in drivers/net/ethernet/cisco/enic/ checks
pci_dev_is_disconnected(), pci_channel_offline() or
pci_channel_io_perm_failure. Should a permanently removed device, which
can no longer bus-master, count as stopped so that normal teardown runs?

The later commit "enic: require DEVCMD2 and set the V2 VF mailbox source"
changes the retention check to enic_control_dma_unresolved(), but it does
not change this behaviour.

[ ... ]

> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 6b5824f45a14c..899e0b1e14be2 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c

[ ... ]

> @@ -2264,17 +2276,27 @@ static void enic_reset(struct work_struct *work)

[ ... ]

> +	if (enic_admin_channel_is_quarantined(enic)) {
> +		/* Core close excludes netpoll and clears __LINK_STATE_START first. */
> +		dev_close(enic->netdev);
> +		goto reset_out;
> +	}
>  
>  	enic_stop(enic->netdev);
>  	if (enic_is_sriov_vf_v2(enic))
> @@ -2287,7 +2309,8 @@ static void enic_reset(struct work_struct *work)
>  	enic_dev_set_ig_vlan_rewrite_mode(enic);
>  	enic_ext_cq(enic);
>  
> -	enic_open(enic->netdev);
> +	if (enic_open(enic->netdev))
> +		goto reset_out;

[Severity: High]
This isn't a bug introduced by this patch, but this patch changes these
exact lines. If enic_open() fails here, the netdev keeps IFF_UP and
__LINK_STATE_START. By then, enic_stop() above has already disabled every
NAPI, freed the IRQs and destroyed the page pools.

enic_open() can fail in any of these:

  - enic_request_intr()
  - enic_dev_notify_set()
  - page_pool_create()
  - the RQ fill
  - enic_dev_enable()

Each of those failure paths leaves NAPI disabled.

A later dev_close(), or unregister_netdev() from enic_remove(), then runs
enic_stop() a second time:

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

The first disable left those bits set. Could this loop spin forever while
holding rtnl_lock and the netdev instance lock? Even if the loop exited,
enic_free_intr() would call free_irq() on vectors whose msix[i].requested
flag was never cleared.

Should the netdev be closed with dev_close() on this failure, as the
quarantined case just above already does? The same applies to
enic_open() in enic_tx_hang_reset().

[ ... ]

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