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
