Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] idpf: Use-After-Free in idpf_handle_event_link() due to concurrent teardown --
commit b8311fab0c0442230fa268ded8ce6b4805cfd5c0 Author: Tian Xun Ng <[email protected]> idpf: keep the mailbox up while tearing down vports on shutdown This commit sets IDPF_REMOVE_IN_PROG during shutdown to keep the mailbox up while vports are destroyed, ensuring teardown messages reach the device. It also bounds transaction timeouts during teardown to prevent unresponsive control planes from delaying reboots. > diff --git a/drivers/net/ethernet/intel/idpf/idpf_main.c > b/drivers/net/ethernet/intel/idpf/idpf_main.c > index 129bccaa6baa8..2e018d02f7dfe 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 and Tx queue marker waits started > + * from here on, so that an unresponsive control plane costs a single > + * timeout. A transaction already in flight keeps its own timeout. > + */ > + WRITE_ONCE(adapter->vc_xn_timeout_ms, IDPF_VC_XN_SHUTDOWN_TIMEOUT_MSEC); > idpf_vc_core_deinit(adapter); [Severity: High] Does setting IDPF_REMOVE_IN_PROG here introduce a use-after-free regression during teardown? In idpf_vc_core_deinit(), when IDPF_REMOVE_IN_PROG is set, the mailbox task is explicitly requeued: drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:idpf_vc_core_deinit() { if (!remove_in_prog) { libie_ctlq_xn_shutdown(adapter->xnm); } else { /* ... */ cancel_delayed_work_sync(&adapter->mbx_task); queue_delayed_work(adapter->mbx_wq, &adapter->mbx_task, 0); } idpf_ptp_release(adapter); idpf_deinit_task(adapter); ... } This allows the mailbox task to run concurrently with idpf_deinit_task() which destroys the vports. If a hardware event (like a link state change) arrives during this window, the mailbox task can call idpf_handle_event_link() which looks up the vport locklessly: drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:idpf_handle_event_link() { np = netdev_priv(vport->netdev); np->link_speed_mbps = le32_to_cpu(v2e->link_speed); if (vport->link_up == v2e->link_status) return; vport->link_up = v2e->link_status; } Meanwhile, idpf_deinit_task() might have already freed the vport via idpf_vport_rel() without synchronizing against the mailbox task or clearing the lookup array first: drivers/net/ethernet/intel/idpf/idpf_lib.c:idpf_vport_rel() { kfree(vport); adapter->num_alloc_vports--; } Could this lead to memory corruption if an unsolicited hardware event arrives while idpf_vc_core_deinit() is tearing down the vports? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
