Re: [PATCH iwl-net v5 3/3] idpf: keep the mailbox up while tearing down vports on shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] idpf: NULL pointer dereference in idpf_tx_start during shutdown
- [High] idpf: SW marker consumption race causes cascading timeouts
- [High] idpf: DMA memory corruption on slow CP during shutdown
--

commit 0b30345598a4b0c67b0f2410c2aaf6bf5c4a2f30
Author: Tian Xun Ng <tianxun.ng@bytedance.com>

idpf: keep the mailbox up while tearing down vports on shutdown

Since commit 4c9106f4906a ("idpf: fix adapter NULL pointer dereference
on reboot"), idpf_shutdown() calls idpf_vc_core_deinit() directly instead
of idpf_remove(), so IDPF_REMOVE_IN_PROG is not set on shutdown.
idpf_vc_core_deinit() uses that flag to decide when to shut the virtchnl
transaction manager down. Without it, libie_ctlq_xn_shutdown() runs
before idpf_deinit_task() tears the vports down.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c b/drivers/net/ethernet/intel/idpf/idpf_main.c
> index 129bccaa6baa8..21be83dc82b1b 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_main.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_main.c
> @@ -194,8 +194,18 @@ static void idpf_shutdown(struct pci_dev *pdev)
>  {
>  	struct idpf_adapter *adapter = pci_get_drvdata(pdev);
>  
> +	/* As in idpf_remove(): keep the event task from being requeued, and
> +	 * destroy the vports while the mailbox is still up.
> +	 */
> +	set_bit(IDPF_REMOVE_IN_PROG, adapter->flags);
>  	cancel_delayed_work_sync(&adapter->serv_task);
>  	cancel_delayed_work_sync(&adapter->vc_event_task);
> +
> +	/* Bound the teardown transactions started from here on, so that an
> +	 * unresponsive control plane costs a single timeout. A reset or a
> +	 * transaction that is already running keeps its own timeouts.
> +	 */
> +	WRITE_ONCE(adapter->vc_xn_timeout_ms, IDPF_VC_XN_SHUTDOWN_TIMEOUT_MSEC);
>  	idpf_vc_core_deinit(adapter);

[Severity: High]
If a hardware reset is in progress (IDPF_HR_RESET_IN_PROG is set),
idpf_vport_dealloc() skips calling idpf_vport_stop() and idpf_decfg_netdev().

This leaves the netdev fully registered with active transmit queues, while the
underlying vport is freed and np->vport is set to NULL:

drivers/net/ethernet/intel/idpf/idpf_lib.c:idpf_vport_dealloc() {
    if (!test_bit(IDPF_HR_RESET_IN_PROG, adapter->flags)) {
        idpf_vport_stop(vport, true);
        idpf_decfg_netdev(vport);
    }
    ...
    if (adapter->netdevs[i]) {
        struct idpf_netdev_priv *np = netdev_priv(adapter->netdevs[i]);
        np->vport = NULL;
    }
    idpf_vport_rel(vport);
}

Unlike idpf_remove(), the idpf_shutdown() function lacks a fallback loop to
unregister stale netdevs or detach the device.

If the networking stack transmits a packet before the system finishes halting,
idpf_tx_start() will dereference the NULL vport:

drivers/net/ethernet/intel/idpf/idpf_txrx.c:idpf_tx_start() {
    const struct idpf_vport *vport = idpf_netdev_to_vport(netdev);
    ...
    if (unlikely(skb_get_queue_mapping(skb) >=
                 vport->num_txq - vport->num_xdp_txq)) {

Can this lead to a NULL pointer dereference kernel panic during shutdown?

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> index 2cfd216ae92b1..331d56faee253 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c

[ ... ]

> @@ -218,10 +230,15 @@ int idpf_send_mb_msg(struct idpf_adapter *adapter,
>  	xn_params->xnm = adapter->xnm;
>  	xn_params->ctlq = xn_params->ctlq ? xn_params->ctlq : adapter->asq;
>  	xn_params->rel_tx_buf = kfree;
> +	xn_params->timeout_ms = min_t(u64, xn_params->timeout_ms, timeout_ms);
>  
>  	idpf_mb_clean(xn_params->ctlq, false);
>  
> -	return libie_ctlq_xn_send(xn_params);
> +	err = libie_ctlq_xn_send(xn_params);
> +	if (err == -ETIMEDOUT && timeout_ms < IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC)
> +		WRITE_ONCE(adapter->vc_xn_timeout_ms, 0);
> +
> +	return err;
>  }

[Severity: High]
If a teardown transaction times out on a slow control plane,
adapter->vc_xn_timeout_ms is set to 0.

This causes subsequent teardown messages (like VIRTCHNL2_OP_DEL_QUEUES) to
fail immediately without being sent.

However, idpf_vport_stop() ignores these failures and unconditionally
frees the descriptor rings' DMA memory via idpf_vport_queues_rel():

drivers/net/ethernet/intel/idpf/idpf_lib.c:idpf_vport_stop() {
    if (test_and_clear_bit(IDPF_VPORT_DEL_QUEUES, vport->flags))
        idpf_send_delete_queues_msg(adapter, chunks, vport_id);
    ...
    idpf_vport_queues_rel(vport, rsrc);
}

If the hardware is still active and eventually processes the delayed
disable queues command, it could write SW_MARKER to the unmapped DMA memory.

Could this lead to DMA memory corruption or an IOMMU fault during shutdown?

[ ... ]

> @@ -380,7 +402,8 @@ static int idpf_wait_for_marker_event_set(const struct idpf_queue_set *qs)
>  			netdev = txq->netdev;
>  
>  			idpf_queue_set(SW_MARKER, txq);
> -			idpf_wait_for_sw_marker_completion(txq);
> +			if (markers_rcvd || wait_all)
> +				idpf_wait_for_sw_marker_completion(txq);
>  			markers_rcvd &= !idpf_queue_has(SW_MARKER, txq);
>  			break;

[Severity: High]
When multiple Tx queues share a completion queue,
idpf_wait_for_sw_marker_completion() polls the ring and consumes any
SW markers it encounters, clearing their SW_MARKER flags:

drivers/net/ethernet/intel/idpf/idpf_txrx.c:idpf_wait_for_sw_marker_completion() {
    ...
    target = complq->txq_grp->txqs[id];
    idpf_queue_clear(SW_MARKER, target);
    ...
}

Because idpf_queue_set(SW_MARKER, txq) is set sequentially for each queue
right before waiting, won't the polling loop for an earlier queue consume
the markers for subsequent queues?

Since the subsequent queues haven't had their SW_MARKER flag set yet by
the outer loop, clearing them is a no-op, and their markers are permanently
lost.

Could this amplify a single delayed or out-of-order marker into a massive
timeout cascade for the remaining queues? Would hoisting the
idpf_queue_set(SW_MARKER, ...) calls to a separate loop before the wait
loop prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013121224.84482-1-luckilystar08@gmail.com?part=3