Thanks! Reviewed-by: Naman Gulati <[email protected]>
On Fri, Oct 9, 2026 at 2:43 PM Emil Tantilov <[email protected]> wrote: > > idpf_ptp_update_cached_phctime() runs from the PTP auxiliary worker and > walks the Rx queues of every vport to refresh rxq->cached_phc_time. There > is a small window on down that can result in use-after-free, detected by > KASAN: > > BUG: KASAN: slab-use-after-free in > idpf_ptp_update_cached_phctime+0x367/0x4a0 [idpf] > Write of size 8 at addr ff11002146c2a4a0 by task ptp3/8084 > ... > idpf_ptp_update_cached_phctime+0x367/0x4a0 [idpf] > idpf_ptp_do_aux_work+0x37/0x50 [idpf] > ptp_aux_kworker+0x64/0xd0 > kthread_worker_fn+0x289/0xa20 > ... > Allocated by task 8615 on cpu 5 at 812.655509s: > idpf_vport_queues_alloc+0x9e5/0x2580 [idpf] > idpf_vport_open.constprop.0+0x12c/0x10f0 [idpf] > idpf_open+0x1bb/0x230 [idpf] > __dev_open+0x2f5/0x7c0 > ... > Freed by task 8628 on cpu 10 at 814.925757s: > idpf_rxq_group_rel.part.0+0xef/0x5a0 [idpf] > idpf_vport_queues_rel+0xaf/0x140 [idpf] > idpf_vport_stop+0x49e/0x850 [idpf] > idpf_stop+0x6d/0xc0 [idpf] > __dev_close_many+0x24c/0x650 > > The existing rxq_grps test does not help. It is evaluated before the > pointer is read again, so the worker can equally run past the kfree() and > dereference a NULL rxq_grps with a stale num_rxq_grp. > > Add a per vport rxq_grps_lock and use it to serialize every transition of > rsrc->rxq_grps against the worker: > > - Teardown: idpf_rxq_group_rel() sets rsrc->rxq_grps to NULL under the > lock before anything reachable through it is freed. The worker > re-reads the pointer under the same lock, so it either sees a live > array while the release path is still blocked on the lock, or sees > NULL and skips that vport. The frees stay outside the lock. > > - Allocation: Build the rxq_grps array privately and publish it to > rsrc->rxq_grps under the lock only once every group is complete. The > freeing of the group contents moves to idpf_rxq_grps_free(), which > takes the array explicitly so the allocation error path can unwind > safely. > > - Soft reset: idpf_vport_stop() freed rxq_grps and cleared the live > vport's copy under rxq_grps_lock. Drop the stale pointer in the clone > so the memcpy() below does not republish the freed array to > the PTP aux worker before idpf_vport_open() reallocates it. The lock > sits past the offsetof(struct idpf_vport, link_up) boundary that > idpf_initiate_soft_reset() stops its memcpy() at, next to the wait > queues and mutexes that are already kept out of the copy, so a soft > reset never byte-copies it. The data it guards, dflt_qv_rsrc, is > inside that copied region, which is why the clone has to be cleared > rather than relying on the lock alone. > > Also, fix the NULL check in idpf_ptp_update_phctime_rxq_grp(). The split > queue case took the address of a member before testing it, which can never > be NULL, so test the rxq_sets[] entry itself. > > Fixes: 1a49cf814fe1 ("idpf: add Tx timestamp flows") > Reviewed-by: Madhu Chittim <[email protected]> > Signed-off-by: Emil Tantilov <[email protected]> > --- > drivers/net/ethernet/intel/idpf/idpf.h | 5 ++ > drivers/net/ethernet/intel/idpf/idpf_lib.c | 6 ++ > drivers/net/ethernet/intel/idpf/idpf_ptp.c | 16 ++++- > drivers/net/ethernet/intel/idpf/idpf_txrx.c | 65 +++++++++++++++------ > 4 files changed, 71 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/intel/idpf/idpf.h > b/drivers/net/ethernet/intel/idpf/idpf.h > index 470bc23c844c..9d63feda1bc6 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf.h > +++ b/drivers/net/ethernet/intel/idpf/idpf.h > @@ -373,6 +373,8 @@ struct idpf_q_vec_rsrc { > * @default_vport: Use this vport if one isn't specified > * @crc_enable: Enable CRC insertion offload > * @link_up: True if link is up > + * @rxq_grps_lock: Serializes every transition of the @dflt_qv_rsrc.rxq_grps > + * pointer of a vport against the PTP auxiliary worker > * @tx_tstamp_caps: Capabilities negotiated for Tx timestamping > * @tstamp_config: The Tx tstamp config > * @tstamp_task: Tx timestamping task > @@ -406,6 +408,9 @@ struct idpf_vport { > bool crc_enable; > bool link_up; > > + /* Must be past the point idpf_initiate_soft_reset() memcpy()s */ > + spinlock_t rxq_grps_lock; > + > struct idpf_ptp_vport_tx_tstamp_caps *tx_tstamp_caps; > struct kernel_hwtstamp_config tstamp_config; > struct work_struct tstamp_task; > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c > b/drivers/net/ethernet/intel/idpf/idpf_lib.c > index 827c795afcb6..a1c739cdd540 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c > @@ -1276,6 +1276,7 @@ static struct idpf_vport *idpf_vport_alloc(struct > idpf_adapter *adapter, > vport->compln_clean_budget = IDPF_TX_COMPLQ_CLEAN_BUDGET; > vport->default_vport = adapter->num_alloc_vports < > idpf_get_default_vports(adapter); > + spin_lock_init(&vport->rxq_grps_lock); > > rsrc = &vport->dflt_qv_rsrc; > rsrc->dev = &adapter->pdev->dev; > @@ -2075,6 +2076,11 @@ int idpf_initiate_soft_reset(struct idpf_vport *vport, > } else { > set_bit(IDPF_VPORT_DEL_QUEUES, vport->flags); > idpf_vport_stop(vport, false); > + > + /* Drop the stale pointer in the clone, as rxq_grps was freed > + * by idpf_vport_stop(). > + */ > + new_rsrc->rxq_grps = NULL; > } > > err = idpf_send_add_queues_msg(adapter, vport_config, new_rsrc, > diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c > b/drivers/net/ethernet/intel/idpf/idpf_ptp.c > index 71fe8b2a8b4e..9b15973d2f7e 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c > @@ -348,9 +348,11 @@ idpf_ptp_update_phctime_rxq_grp(const struct > idpf_rxq_group *grp, bool split, > } > } else { > for (i = 0; i < grp->splitq.num_rxq_sets; i++) { > + if (!grp->splitq.rxq_sets[i]) > + continue; > + > rxq = &grp->splitq.rxq_sets[i]->rxq; > - if (rxq) > - WRITE_ONCE(rxq->cached_phc_time, systime); > + WRITE_ONCE(rxq->cached_phc_time, systime); > } > } > } > @@ -387,10 +389,17 @@ static int idpf_ptp_update_cached_phctime(struct > idpf_adapter *adapter) > struct idpf_q_vec_rsrc *rsrc; > bool split; > > - if (!vport || !vport->dflt_qv_rsrc.rxq_grps) > + if (!vport) > continue; > > rsrc = &vport->dflt_qv_rsrc; > + > + spin_lock(&vport->rxq_grps_lock); > + if (!rsrc->rxq_grps) { > + spin_unlock(&vport->rxq_grps_lock); > + continue; > + } > + > split = idpf_is_queue_model_split(rsrc->rxq_model); > > for (u16 i = 0; i < rsrc->num_rxq_grp; i++) { > @@ -398,6 +407,7 @@ static int idpf_ptp_update_cached_phctime(struct > idpf_adapter *adapter) > > idpf_ptp_update_phctime_rxq_grp(grp, split, systime); > } > + spin_unlock(&vport->rxq_grps_lock); > } > > return 0; > diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c > b/drivers/net/ethernet/intel/idpf/idpf_txrx.c > index af3213eae393..96f79554a4d0 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c > @@ -1358,16 +1358,15 @@ static void idpf_rxq_sw_queue_rel(struct > idpf_rxq_group *rx_qgrp) > } > > /** > - * idpf_rxq_group_rel - Release all resources for rxq groups > + * idpf_rxq_grps_free - Free an Rx queue group array and everything in it > * @rsrc: pointer to queue and vector resources > + * @rxq_grps: group array to free, not reachable through @rsrc any more > */ > -static void idpf_rxq_group_rel(struct idpf_q_vec_rsrc *rsrc) > +static void idpf_rxq_grps_free(struct idpf_q_vec_rsrc *rsrc, > + struct idpf_rxq_group *rxq_grps) > { > - if (!rsrc->rxq_grps) > - return; > - > for (unsigned int i = 0; i < rsrc->num_rxq_grp; i++) { > - struct idpf_rxq_group *rx_qgrp = &rsrc->rxq_grps[i]; > + struct idpf_rxq_group *rx_qgrp = &rxq_grps[i]; > u16 num_rxq; > > if (idpf_is_queue_model_split(rsrc->rxq_model)) { > @@ -1388,19 +1387,40 @@ static void idpf_rxq_group_rel(struct idpf_q_vec_rsrc > *rsrc) > } > } > } > - kfree(rsrc->rxq_grps); > + kfree(rxq_grps); > +} > + > +/** > + * idpf_rxq_group_rel - Release all resources for rxq groups > + * @vport: vport the queue groups belong to > + * @rsrc: pointer to queue and vector resources > + */ > +static void idpf_rxq_group_rel(struct idpf_vport *vport, > + struct idpf_q_vec_rsrc *rsrc) > +{ > + struct idpf_rxq_group *rxq_grps; > + > + if (!rsrc->rxq_grps) > + return; > + > + spin_lock(&vport->rxq_grps_lock); > + rxq_grps = rsrc->rxq_grps; > rsrc->rxq_grps = NULL; > + spin_unlock(&vport->rxq_grps_lock); > + > + idpf_rxq_grps_free(rsrc, rxq_grps); > } > > /** > * idpf_vport_queue_grp_rel_all - Release all queue groups > - * @vport: vport to release queue groups for > + * @vport: vport the queue groups belong to > * @rsrc: pointer to queue and vector resources > */ > -static void idpf_vport_queue_grp_rel_all(struct idpf_q_vec_rsrc *rsrc) > +static void idpf_vport_queue_grp_rel_all(struct idpf_vport *vport, > + struct idpf_q_vec_rsrc *rsrc) > { > idpf_txq_group_rel(rsrc); > - idpf_rxq_group_rel(rsrc); > + idpf_rxq_group_rel(vport, rsrc); > } > > /** > @@ -1419,7 +1439,7 @@ void idpf_vport_queues_rel(struct idpf_vport *vport, > idpf_rx_desc_rel_all(rsrc); > > idpf_xdpsqs_put(vport); > - idpf_vport_queue_grp_rel_all(rsrc); > + idpf_vport_queue_grp_rel_all(vport, rsrc); > > kfree(vport->txqs); > vport->txqs = NULL; > @@ -1803,18 +1823,19 @@ static int idpf_rxq_group_alloc(struct idpf_vport > *vport, > u16 num_rxq) > { > struct idpf_adapter *adapter = vport->adapter; > + struct idpf_rxq_group *rxq_grps; > bool hs, rsc; > - int err = 0; > + int err; > > - rsrc->rxq_grps = kzalloc_objs(struct idpf_rxq_group, > rsrc->num_rxq_grp); > - if (!rsrc->rxq_grps) > + rxq_grps = kzalloc_objs(struct idpf_rxq_group, rsrc->num_rxq_grp); > + if (!rxq_grps) > return -ENOMEM; > > hs = idpf_vport_get_hsplit(vport) == ETHTOOL_TCP_DATA_SPLIT_ENABLED; > rsc = idpf_is_feature_ena(vport, NETIF_F_GRO_HW); > > for (unsigned int i = 0; i < rsrc->num_rxq_grp; i++) { > - struct idpf_rxq_group *rx_qgrp = &rsrc->rxq_grps[i]; > + struct idpf_rxq_group *rx_qgrp = &rxq_grps[i]; > > rx_qgrp->vport = vport; > if (!idpf_is_queue_model_split(rsrc->rxq_model)) { > @@ -1915,9 +1936,17 @@ static int idpf_rxq_group_alloc(struct idpf_vport > *vport, > } > } > > + /* Publish only once every group is built, the PTP aux worker reads > + * the array as soon as it becomes visible. > + */ > + spin_lock(&vport->rxq_grps_lock); > + rsrc->rxq_grps = rxq_grps; > + spin_unlock(&vport->rxq_grps_lock); > + > + return 0; > + > err_alloc: > - if (err) > - idpf_rxq_group_rel(rsrc); > + idpf_rxq_grps_free(rsrc, rxq_grps); > > return err; > } > @@ -1948,7 +1977,7 @@ static int idpf_vport_queue_grp_alloc_all(struct > idpf_vport *vport, > return 0; > > err_out: > - idpf_vport_queue_grp_rel_all(rsrc); > + idpf_vport_queue_grp_rel_all(vport, rsrc); > > return err; > } > -- > 2.37.3 >
