> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > > Pre-existing issues: > - [High] Use-After-Free of CQ in EQ event processing due to lack of > synchronization during CQ destruction > -- > > commit 3d0d6fcf405f01d902d109c4441be576c316986f > Author: Long Li <[email protected]> > net: mana: release EQs left idle by a channel-count reduction > > The shared EQ pool only grows, so it sits at the high-water mark of every > channel count the port has ever used. This commit releases the EQs above > the live queue count once a retiring set has been torn down. It also ensures > the dentry is properly recorded for debugfs. > > > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c > > b/drivers/net/ethernet/microsoft/mana/mana_en.c > > index 69e34fab8fe8e..2295769da0306 100644 > > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c > [ ... ] > > @@ -4745,12 +4785,19 @@ void mana_free_qset(struct > mana_port_context > > *scratch, struct mana_qset *qset) > > > > memset(qset, 0, sizeof(*qset)); > > > > + /* This set is gone, so any EQ above the live queue count is now > > + * unreferenced. Release those vectors instead of holding them at > the > > + * high-water mark. Safe here and only here: the retiring set's CQs > > + * have just been destroyed. > > + */ > > [Severity: High] > This is a pre-existing issue, but since this teardown path is being modified, > can the destruction of the retiring set's CQs race with the EQ interrupt > handler? > > An admin reconfiguring the network interface (e.g., changing MTU, ring sizes, > or channels) will tear down the old queue set. The hardware queues are > fenced, generating a final CQE in the CQ. > > If the EQ interrupt fires, CPU A could process the EQE in > mana_gd_process_eqe(). It reads the CQ pointer but might be delayed (e.g., > by an NMI) before invoking the callback: > > drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_process_eqe() > { > ... > cq = gc->cq_table[cq_id]; > if (WARN_ON_ONCE(!cq || cq->type != GDMA_CQ || cq->id != cq_id)) > break; > > if (cq->cq.callback) > cq->cq.callback(cq->cq.context, cq); > ... > } > > Concurrently, NAPI on CPU C could process the fence CQE and signal the > completion event. CPU B, waiting in mana_fence_rqs(), wakes up and tears > down the old CQ via mana_destroy_rxq() -> mana_gd_destroy_queue(). > > It clears gc->cq_table[cq_id] = NULL in mana_gd_destroy_cq() and frees the > CQ memory immediately in mana_gd_destroy_queue(): > > drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_destroy_queu > e() { > ... > mana_gd_destroy_cq(gc, queue); > ... > kfree(queue); > } > > When CPU A resumes and executes cq->cq.callback(cq->cq.context, cq), this > dereferences the freed cq and mana_rxq structures to call napi_schedule(), > causing a UAF write to napi.state. > > Is there any synchronization, such as RCU or IRQ synchronization, to prevent > this use-after-free during CQ destruction? > > > + mana_shrink_eqs(apc, apc->num_queues); > > + > > /* Queues built through a scratch context carry no debugfs nodes, > > * because both sets are alive during the swap and would collide on > > * the same names. The retiring set's nodes are gone now, so the > > * published queues can finally take those names. > > */ > > - mana_qset_debugfs_publish(netdev_priv(scratch->ndev)); > > + mana_qset_debugfs_publish(apc); > > } > > > > /* --- end of pre-allocate + swap reconfiguration path > > ---------------------- */ > > --
This is an existing issue with the original code; this patch set doesn't touch it and will leave it as is. If needed, I will submit another net patch for fixing the existing issue. Long
