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 <[email protected]> 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/[email protected]?part=3
