* [PATCH net v4] octeontx2-af: Fix rep link state sync and workqueue races
@ 2026-09-17 4:20 nshettyj
2026-09-21 4:33 ` netdev-bot+sashiko
0 siblings, 1 reply; 2+ messages in thread
From: nshettyj @ 2026-09-17 4:20 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: Geetha sowjanya, Nitin Shetty J, Sunil Goutham, Ratheesh Kannoth,
Subbaraya Sundeep, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Bharat Bhushan, Simon Horman,
Harman Kalra
From: Geetha sowjanya <gakula@marvell.com>
Move rep event workqueue init to rvu_mbox_handler_get_rep_cnt(),
fix use-after-free and race conditions in rep event handling,
add bounds checking, and serialize LBK link configuration.
Fixes: b8fea84a0468 ("octeontx2-pf: Add support to sync link state between representor and VFs")
Signed-off-by: Nitin Shetty J <nshettyj@marvell.com>
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
---
changes in v4:
- Addressed Sashiko review comments.
- Block get_rep_cnt() from allocating a new rep_evt_wq once RVU teardown starts.
- Flush the AF-VF mailbox workqueue too, in addition to AF-PF, before destroying rep_evt_wq.
- Drop queued representor events during teardown instead of blocking on mailbox timeouts.
- Publish/consume rep_evt_wq with smp_store_release()/smp_load_acquire() for correct ordering.
- Set rep_pcifunc only after the representor map and workqueue are successfully initialized.
- Reject GET_REP_CNT from VF callers; only a PF may register as the representor.
- Reject representor count exceeding RVU_MAX_REP instead of silently capping it.
- Disable the representor's own LBK link and reset MCAM bookkeeping on rule-install failure.
Link: https://lore.kernel.org/lkml/aqrInFMnvs4K48+3@kernel-ep2/
changes in v3:
- Introduce RVU_MAX_REP macro for the representor map array size.
- Fix use-after-free in rvu_remove() when rep_evt_wq is destroyed
while a mbox handler is still running.
- Fix a race in rvu_mbox_handler_rep_event_notify() where rep_evt_wq
could be freed between the NULL check and queue_work().
- Reject REP_EVENT_NOTIFY from non-owner PFs and invalid pcifunc values.
- Fix LBK link leak when MCAM rule installation fails midway.
- Fix a race in rvu_mbox_handler_get_rep_cnt() where concurrent mbox
handlers could both initialize rep2pfvf_map.
- Reject GET_REP_CNT from non-owner callers with -EPERM.
- Cap rep_cnt to RVU_MAX_REP to prevent out-of-bounds map writes.
- Fix inconsistent rep_cnt state when get_rep_cnt initialization fails.
- Fix a race in rvu_switch_enable_lbk_link() where nix_blkaddr could
change mid-call, and fix NULL dereference when nix_hw is not assigned.
changes in v2:
- Reject REP event notifications before workqueue setup and
validate PF/VF state event pcifuncs.
- Make REP map initialization atomic by using a temporary map,
handling zero-REP cases, and rolling back on workqueue allocation
failure.
- Use an unbound REP event workqueue.
- Serialize LBK link TL2 configuration with rsrc_lock.
---
.../net/ethernet/marvell/octeontx2/af/mbox.h | 4 +-
.../net/ethernet/marvell/octeontx2/af/rvu.c | 17 ++
.../net/ethernet/marvell/octeontx2/af/rvu.h | 1 +
.../ethernet/marvell/octeontx2/af/rvu_rep.c | 211 +++++++++++++-----
.../marvell/octeontx2/af/rvu_switch.c | 35 ++-
.../net/ethernet/marvell/octeontx2/nic/rep.c | 12 +
.../net/ethernet/marvell/octeontx2/nic/rep.h | 1 -
7 files changed, 220 insertions(+), 61 deletions(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
index cece197d1074..d114faeba1bb 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
@@ -1777,10 +1777,12 @@ struct ptp_get_cap_rsp {
u64 cap;
};
+#define RVU_MAX_REP 64
+
struct get_rep_cnt_rsp {
struct mbox_msghdr hdr;
u16 rep_cnt;
- u16 rep_pf_map[64];
+ u16 rep_pf_map[RVU_MAX_REP];
u64 rsvd;
};
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
index 937b085582b5..4a4e7e434d4b 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
@@ -3715,12 +3715,29 @@ static int rvu_probe(struct pci_dev *pdev, const struct pci_device_id *id)
static void rvu_remove(struct pci_dev *pdev)
{
+ struct workqueue_struct *rep_wq;
struct rvu *rvu = pci_get_drvdata(pdev);
rvu_dbg_exit(rvu);
rvu_unregister_dl(rvu);
+
+ /* Block get_rep_cnt() from allocating a new rep_evt_wq. */
+ mutex_lock(&rvu->rsrc_lock);
+ WRITE_ONCE(rvu->rep_evt_teardown, true);
+ rep_wq = rvu->rep_evt_wq;
+ WRITE_ONCE(rvu->rep_evt_wq, NULL);
+ mutex_unlock(&rvu->rsrc_lock);
+
rvu_unregister_interrupts(rvu);
rvu_flr_wq_destroy(rvu);
+
+ /* Flush both mbox workqueues before destroying rep_wq. */
+ flush_workqueue(rvu->afpf_wq_info.mbox_wq);
+ if (rvu->afvf_wq_info.mbox_wq)
+ flush_workqueue(rvu->afvf_wq_info.mbox_wq);
+ if (rep_wq)
+ destroy_workqueue(rep_wq);
+
rvu_cgx_exit(rvu);
rvu_fwdata_exit(rvu);
rvu_mcs_exit(rvu);
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
index 9afb7ac8969b..9da5fd29451b 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
@@ -675,6 +675,7 @@ struct rvu {
struct list_head rep_evtq_head;
/* Representor event lock */
spinlock_t rep_evtq_lock;
+ bool rep_evt_teardown;
struct ng_rvu *ng_rvu;
};
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
index a2781e0f504e..705821fbf011 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
@@ -44,6 +44,8 @@ static int rvu_rep_up_notify(struct rvu *rvu, struct rep_event *event)
if (event->event & RVU_EVENT_MAC_ADDR_CHANGE)
ether_addr_copy(pfvf->mac_addr, event->evt_data.mac);
+ if (event->event & RVU_EVENT_PFVF_STATE)
+ pf = rvu_get_pf(rvu->pdev, event->hdr.pcifunc);
mutex_lock(&rvu->mbox_lock);
msg = otx2_mbox_alloc_msg_rep_event_up_notify(rvu, pf);
if (!msg) {
@@ -53,6 +55,10 @@ static int rvu_rep_up_notify(struct rvu *rvu, struct rep_event *event)
msg->hdr.pcifunc = event->pcifunc;
msg->event = event->event;
+ msg->pcifunc = event->pcifunc;
+
+ if (event->event & RVU_EVENT_PFVF_STATE)
+ msg->hdr.pcifunc = event->hdr.pcifunc;
memcpy(&msg->evt_data, &event->evt_data, sizeof(struct rep_evt_data));
@@ -87,7 +93,12 @@ static void rvu_rep_wq_handler(struct work_struct *work)
event = &qentry->event;
- rvu_rep_up_notify(rvu, event);
+ /* Once teardown has started the AF-PF mbox interrupt may
+ * already be disabled, so sending would just block until
+ * otx2_mbox_wait_for_rsp() times out. Drop the event instead.
+ */
+ if (!READ_ONCE(rvu->rep_evt_teardown))
+ rvu_rep_up_notify(rvu, event);
kfree(qentry);
} while (1);
}
@@ -95,16 +106,28 @@ static void rvu_rep_wq_handler(struct work_struct *work)
int rvu_mbox_handler_rep_event_notify(struct rvu *rvu, struct rep_event *req,
struct msg_rsp *rsp)
{
+ struct workqueue_struct *wq;
struct rep_evtq_ent *qentry;
- /* The mailbox dispatcher normalises only the header pcifunc; the
- * nested struct rep_event::pcifunc body field is sender-controlled
- * and is later used by rvu_rep_up_notify() to index rvu->pf[] /
- * rvu->hwvf[]. Reject out-of-range body selectors before queueing.
- */
+ wq = smp_load_acquire(&rvu->rep_evt_wq);
+ if (!wq)
+ return -EINVAL;
+
+ /* Only the registered representor PF may send REP_EVENT_NOTIFY. */
+ if (req->hdr.pcifunc != rvu->rep_pcifunc)
+ return -EPERM;
+
if (!is_pf_func_valid(rvu, req->pcifunc))
return -EINVAL;
+ /* Only CGX-mapped PFs are present in the representor map. */
+ if (!is_pf_cgxmapped(rvu, rvu_get_pf(rvu->pdev, req->pcifunc)))
+ return -EINVAL;
+
+ if ((req->event & RVU_EVENT_PFVF_STATE) &&
+ rvu_get_pf(rvu->pdev, req->hdr.pcifunc) >= rvu->hw->total_pfs)
+ return -EINVAL;
+
qentry = kmalloc_obj(*qentry, GFP_ATOMIC);
if (!qentry)
return -ENOMEM;
@@ -113,37 +136,23 @@ int rvu_mbox_handler_rep_event_notify(struct rvu *rvu, struct rep_event *req,
spin_lock(&rvu->rep_evtq_lock);
list_add_tail(&qentry->node, &rvu->rep_evtq_head);
spin_unlock(&rvu->rep_evtq_lock);
- queue_work(rvu->rep_evt_wq, &rvu->rep_evt_work);
+ queue_work(wq, &rvu->rep_evt_work);
return 0;
}
int rvu_rep_notify_pfvf_state(struct rvu *rvu, u16 pcifunc, bool enable)
{
- struct rep_event *req;
- int pf;
+ struct rep_event req = { 0 };
+ struct msg_rsp rsp;
if (!is_pf_cgxmapped(rvu, rvu_get_pf(rvu->pdev, pcifunc)))
return 0;
- pf = rvu_get_pf(rvu->pdev, rvu->rep_pcifunc);
-
- mutex_lock(&rvu->mbox_lock);
- req = otx2_mbox_alloc_msg_rep_event_up_notify(rvu, pf);
- if (!req) {
- mutex_unlock(&rvu->mbox_lock);
- return -ENOMEM;
- }
-
- req->hdr.pcifunc = rvu->rep_pcifunc;
- req->event |= RVU_EVENT_PFVF_STATE;
- req->pcifunc = pcifunc;
- req->evt_data.vf_state = enable;
-
- otx2_mbox_wait_for_zero(&rvu->afpf_wq_info.mbox_up, pf);
- otx2_mbox_msg_send_up(&rvu->afpf_wq_info.mbox_up, pf);
-
- mutex_unlock(&rvu->mbox_lock);
- return 0;
+ req.hdr.pcifunc = rvu->rep_pcifunc;
+ req.event = RVU_EVENT_PFVF_STATE;
+ req.pcifunc = pcifunc;
+ req.evt_data.vf_state = enable;
+ return rvu_mbox_handler_rep_event_notify(rvu, &req, &rsp);
}
#define RVU_LF_RX_STATS(reg) \
@@ -325,6 +334,7 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
u16 start = rswitch->start_entry;
struct rvu_hwinfo *hw = rvu->hw;
u16 pcifunc, entry = 0;
+ struct rvu_pfvf *pfvf;
int pf, vf, numvfs;
int err, nixlf, i;
u8 rep;
@@ -334,19 +344,22 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
continue;
pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
+ pfvf = rvu_get_pfvf(rvu, pcifunc);
rvu_get_nix_blkaddr(rvu, pcifunc);
+ if (test_bit(NIXLF_INITIALIZED, &pfvf->flags))
+ rvu_switch_enable_lbk_link(rvu, pcifunc, true);
rep = true;
for (i = 0; i < 2; i++) {
err = rvu_rep_install_rx_rule(rvu, pcifunc,
start + entry, rep);
if (err)
- return err;
+ goto err_disable_lbk;
rswitch->entry2pcifunc[entry++] = pcifunc;
err = rvu_rep_install_tx_rule(rvu, pcifunc,
start + entry, rep);
if (err)
- return err;
+ goto err_disable_lbk;
rswitch->entry2pcifunc[entry++] = pcifunc;
rep = false;
}
@@ -354,9 +367,12 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
for (vf = 0; vf < numvfs; vf++) {
pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
+ pfvf = rvu_get_pfvf(rvu, pcifunc);
+ if (test_bit(NIXLF_INITIALIZED, &pfvf->flags))
+ rvu_switch_enable_lbk_link(rvu, pcifunc, true);
rvu_get_nix_blkaddr(rvu, pcifunc);
- /* Skip installimg rules if nixlf is not attached */
+ /* Skip installing rules if nixlf is not attached */
err = nix_get_nixlf(rvu, pcifunc, &nixlf, NULL);
if (err)
continue;
@@ -366,30 +382,37 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
start + entry,
rep);
if (err)
- return err;
+ goto err_disable_lbk;
rswitch->entry2pcifunc[entry++] = pcifunc;
err = rvu_rep_install_tx_rule(rvu, pcifunc,
start + entry,
rep);
if (err)
- return err;
+ goto err_disable_lbk;
rswitch->entry2pcifunc[entry++] = pcifunc;
rep = false;
}
}
}
+ return 0;
- /* Initialize the wq for handling REP events */
- spin_lock_init(&rvu->rep_evtq_lock);
- INIT_LIST_HEAD(&rvu->rep_evtq_head);
- INIT_WORK(&rvu->rep_evt_work, rvu_rep_wq_handler);
- rvu->rep_evt_wq = alloc_workqueue("rep_evt_wq", WQ_PERCPU, 0);
- if (!rvu->rep_evt_wq) {
- dev_err(rvu->dev, "REP workqueue allocation failed\n");
- return -ENOMEM;
+err_disable_lbk:
+ /* Undo any LBK links enabled above before the MCAM rule failure.
+ * Disabling a link that was never enabled is a safe no-op.
+ */
+ for (pf = 1; pf < hw->total_pfs; pf++) {
+ if (!is_pf_cgxmapped(rvu, pf))
+ continue;
+ pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
+ rvu_switch_enable_lbk_link(rvu, pcifunc, false);
+ rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
+ for (vf = 0; vf < numvfs; vf++) {
+ pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
+ rvu_switch_enable_lbk_link(rvu, pcifunc, false);
+ }
}
- return 0;
+ return err;
}
void rvu_rep_update_rules(struct rvu *rvu, u16 pcifunc, bool ena)
@@ -443,35 +466,111 @@ int rvu_mbox_handler_esw_cfg(struct rvu *rvu, struct esw_cfg_req *req,
return 0;
}
+static int rvu_rep_get_rep_map(struct rvu *rvu, struct msg_req *req,
+ struct get_rep_cnt_rsp *rsp)
+{
+ int rep;
+
+ if (req->hdr.pcifunc != rvu->rep_pcifunc)
+ return -EPERM;
+
+ rsp->rep_cnt = rvu->rep_cnt;
+ for (rep = 0; rep < rvu->rep_cnt; rep++)
+ rsp->rep_pf_map[rep] = rvu->rep2pfvf_map[rep];
+
+ return 0;
+}
+
int rvu_mbox_handler_get_rep_cnt(struct rvu *rvu, struct msg_req *req,
struct get_rep_cnt_rsp *rsp)
{
- int pf, vf, numvfs, hwvf, rep = 0;
+ int pf, vf, numvfs, hwvf, rep = 0, cnt;
+ struct workqueue_struct *wq;
+ int ret = 0;
u16 pcifunc;
+ u16 *map;
- rvu->rep_pcifunc = req->hdr.pcifunc;
- rsp->rep_cnt = rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs;
- rvu->rep_cnt = rsp->rep_cnt;
+ /* Serialize first-time initialization since mbox_wq is WQ_PERCPU. */
+ mutex_lock(&rvu->rsrc_lock);
- rvu->rep2pfvf_map = devm_kzalloc(rvu->dev, rvu->rep_cnt *
- sizeof(u16), GFP_KERNEL);
- if (!rvu->rep2pfvf_map)
- return -ENOMEM;
+ if (rvu->rep_evt_teardown) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+
+ if (rvu->rep2pfvf_map) {
+ ret = rvu_rep_get_rep_map(rvu, req, rsp);
+ goto unlock;
+ }
+
+ /* Only a PF can register as the representor, not a VF. */
+ if (req->hdr.pcifunc & RVU_PFVF_FUNC_MASK) {
+ ret = -EPERM;
+ goto unlock;
+ }
+
+ cnt = rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs;
+ if (cnt > RVU_MAX_REP) {
+ dev_err(rvu->dev, "Representor count %d exceeds max %d\n",
+ cnt, RVU_MAX_REP);
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ /* Allocate at least one element so the pointer is always non-NULL
+ * once published, keeping the fast-path check above reliable.
+ */
+ map = devm_kzalloc(rvu->dev, (cnt ?: 1) * sizeof(u16), GFP_KERNEL);
+ if (!map) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
for (pf = 0; pf < rvu->hw->total_pfs; pf++) {
if (!is_pf_cgxmapped(rvu, pf))
continue;
+ if (rep >= cnt)
+ break;
pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
- rvu->rep2pfvf_map[rep] = pcifunc;
+ map[rep] = pcifunc;
rsp->rep_pf_map[rep] = pcifunc;
rep++;
rvu_get_pf_numvfs(rvu, pf, &numvfs, &hwvf);
- for (vf = 0; vf < numvfs; vf++) {
- rvu->rep2pfvf_map[rep] = pcifunc |
- ((vf + 1) & RVU_PFVF_FUNC_MASK);
- rsp->rep_pf_map[rep] = rvu->rep2pfvf_map[rep];
+ for (vf = 0; vf < numvfs && rep < cnt; vf++) {
+ map[rep] = pcifunc | ((vf + 1) & RVU_PFVF_FUNC_MASK);
+ rsp->rep_pf_map[rep] = map[rep];
rep++;
}
}
- return 0;
+
+ if (!cnt) {
+ rvu->rep2pfvf_map = map;
+ rvu->rep_pcifunc = req->hdr.pcifunc;
+ goto unlock;
+ }
+
+ /* Initialize the wq for handling REP events */
+ spin_lock_init(&rvu->rep_evtq_lock);
+ INIT_LIST_HEAD(&rvu->rep_evtq_head);
+ INIT_WORK(&rvu->rep_evt_work, rvu_rep_wq_handler);
+ wq = alloc_workqueue("rep_evt_wq", WQ_UNBOUND, 0);
+ if (!wq) {
+ dev_err(rvu->dev, "REP workqueue allocation failed\n");
+ devm_kfree(rvu->dev, map);
+ ret = -ENOMEM;
+ goto unlock;
+ }
+
+ rvu->rep_cnt = cnt;
+ rsp->rep_cnt = cnt;
+ rvu->rep2pfvf_map = map;
+ rvu->rep_pcifunc = req->hdr.pcifunc;
+
+ /* Pairs with smp_load_acquire() in rvu_mbox_handler_rep_event_notify()
+ * to publish the above initialization before wq becomes visible.
+ */
+ smp_store_release(&rvu->rep_evt_wq, wq);
+unlock:
+ mutex_unlock(&rvu->rsrc_lock);
+ return ret;
}
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
index 49ce38685a7e..92719399a953 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
@@ -12,11 +12,18 @@ void rvu_switch_enable_lbk_link(struct rvu *rvu, u16 pcifunc, bool enable)
{
struct rvu_pfvf *pfvf = rvu_get_pfvf(rvu, pcifunc);
struct nix_hw *nix_hw;
+ int blkaddr;
- nix_hw = get_nix_hw(rvu->hw, pfvf->nix_blkaddr);
+ mutex_lock(&rvu->rsrc_lock);
+ blkaddr = pfvf->nix_blkaddr;
+ nix_hw = get_nix_hw(rvu->hw, blkaddr);
/* Enable LBK links with channel 63 for TX MCAM rule */
- rvu_nix_tx_tl2_cfg(rvu, pfvf->nix_blkaddr, pcifunc,
+ if (!nix_hw)
+ goto unlock;
+ rvu_nix_tx_tl2_cfg(rvu, blkaddr, pcifunc,
&nix_hw->txsch[NIX_TXSCH_LVL_TL2], enable);
+unlock:
+ mutex_unlock(&rvu->rsrc_lock);
}
static int rvu_switch_install_rx_rule(struct rvu *rvu, u16 pcifunc,
@@ -203,10 +210,16 @@ void rvu_switch_enable(struct rvu *rvu)
return;
uninstall_rules:
+ if (rvu->rep_mode && rvu->rep_pcifunc)
+ rvu_switch_enable_lbk_link(rvu, rvu->rep_pcifunc, false);
+
uninstall_req.start = rswitch->start_entry;
uninstall_req.end = rswitch->start_entry + rswitch->used_entries - 1;
rvu_mbox_handler_npc_delete_flow(rvu, &uninstall_req, &uninstall_rsp);
kfree(rswitch->entry2pcifunc);
+ rswitch->entry2pcifunc = NULL;
+ rswitch->used_entries = 0;
+ rswitch->start_entry = 0;
free_entries:
free_req.all = 1;
rvu_mbox_handler_npc_mcam_free_entry(rvu, &free_req, &rsp);
@@ -229,8 +242,23 @@ void rvu_switch_disable(struct rvu *rvu)
if (!rswitch->used_entries)
return;
- if (rvu->rep_mode)
+ if (rvu->rep_mode) {
+ if (rvu->rep_pcifunc)
+ rvu_switch_enable_lbk_link(rvu, rvu->rep_pcifunc, false);
+
+ for (pf = 1; pf < hw->total_pfs; pf++) {
+ if (!is_pf_cgxmapped(rvu, pf))
+ continue;
+ pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
+ rvu_switch_enable_lbk_link(rvu, pcifunc, false);
+ rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
+ for (vf = 0; vf < numvfs; vf++) {
+ pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
+ rvu_switch_enable_lbk_link(rvu, pcifunc, false);
+ }
+ }
goto free_ents;
+ }
for (pf = 1; pf < hw->total_pfs; pf++) {
if (!is_pf_cgxmapped(rvu, pf))
@@ -267,6 +295,7 @@ void rvu_switch_disable(struct rvu *rvu)
rvu_mbox_handler_npc_mcam_free_entry(rvu, &free_req, &rsp);
rswitch->used_entries = 0;
kfree(rswitch->entry2pcifunc);
+ rswitch->entry2pcifunc = NULL;
}
void rvu_switch_update_rules(struct rvu *rvu, u16 pcifunc, bool ena)
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
index 0f5d5642d3f7..ef47e7e21901 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
@@ -301,6 +301,12 @@ static void rvu_rep_state_evt_handler(struct otx2_nic *priv,
int rep_id;
rep_id = rvu_rep_get_repid(priv, info->pcifunc);
+ if (rep_id < 0) {
+ dev_warn_ratelimited(priv->dev,
+ "REP state event for unknown pcifunc 0x%x (err %d)\n",
+ info->pcifunc, rep_id);
+ return;
+ }
rep = priv->reps[rep_id];
if (info->evt_data.vf_state)
rep->flags |= RVU_REP_VF_INITIALIZED;
@@ -459,6 +465,9 @@ static int rvu_rep_open(struct net_device *dev)
netif_carrier_on(dev);
netif_tx_start_all_queues(dev);
+ if (rep->pcifunc & RVU_PFVF_FUNC_MASK)
+ return 0;
+
evt.event = RVU_EVENT_PORT_STATE;
evt.evt_data.port_state = 1;
evt.pcifunc = rep->pcifunc;
@@ -478,6 +487,9 @@ static int rvu_rep_stop(struct net_device *dev)
netif_carrier_off(dev);
netif_tx_disable(dev);
+ if (rep->pcifunc & RVU_PFVF_FUNC_MASK)
+ return 0;
+
evt.event = RVU_EVENT_PORT_STATE;
evt.pcifunc = rep->pcifunc;
rvu_rep_notify_pfvf(priv, RVU_EVENT_PORT_STATE, &evt);
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
index 5bc9e2c7d800..b98fe191e83a 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
@@ -16,7 +16,6 @@
#define PCI_DEVID_RVU_REP 0xA0E0
-#define RVU_MAX_REP OTX2_MAX_CQ_CNT
struct rep_stats {
u64 rx_bytes;
--
2.48.1
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [PATCH net v4] octeontx2-af: Fix rep link state sync and workqueue races
2026-09-17 4:20 [PATCH net v4] octeontx2-af: Fix rep link state sync and workqueue races nshettyj
@ 2026-09-21 4:33 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-21 4:33 UTC (permalink / raw)
To: nshettyj
Cc: netdev, linux-kernel, gakula, sgoutham, rkannoth, sbhatta,
andrew+netdev, davem, edumazet, kuba, pabeni, bbhushan2, horms,
hkalra
Thank you for your contribution! Sashiko AI review found 9 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 6 (1) · Low: 1 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] Converting `rvu_rep_notify_pfvf_state()` into an asynchronous
rep_evt_wq item lets a PFVF_STATE notification be delivered to the…
- [High] The `rvu->rswitch` bookkeeping (`used_entries`, `start_entry`,
`entry2pcifunc`) is mutated by the devlink-driven…
- [Medium] The commit message describes only four changes (move wq init,
fix UAF/races, add bounds checking, serialize LBK config) and carries…
- [Medium] The patch teaches `rvu_rep_install_mcam_rules()` to recognise
PF/VFs whose NIXLF is already initialized…
- [Medium] `rvu_rep_notify_pfvf_state()` no longer sends the PF/VF-state
up-message itself; it funnels the event through…
- [Medium] The representor workqueue allocation was moved out of
`rvu_rep_install_mcam_rules()` into the `GET_REP_CNT` mailbox handler,…
- [Medium] The new teardown guard in `rvu_rep_wq_handler()` (`if
(!READ_ONCE(rvu->rep_evt_teardown)) rvu_rep_up_notify(...)`) is a…
- [Medium] All the new access-control checks introduced by this patch
(-EPERM in `rvu_mbox_handler_get_rep_cnt()`/`rvu_rep_get_rep_map()`…
- [Low] In `rvu_rep_open()`/`rvu_rep_stop()` the patch adds `if
(rep->pcifunc & RVU_PFVF_FUNC_MASK) return 0;`, so administratively…
Pre-existing issues:
- [Medium] The new `err_disable_lbk` rollback in
`rvu_rep_install_mcam_rules()` only clears the TL2 LBK link…
- [Low] Producer/consumer disagreement on the rep_event pcifunc selector:
`rvu_rep_up_notify()` addresses the up-message with `pf =…
--
octeontx2-af: Fix rep link state sync and workqueue races
This moves the representor event workqueue allocation out of
rvu_rep_install_mcam_rules() and into the GET_REP_CNT mailbox handler,
adds ownership/bounds checks to the representor mailbox handlers, and
serializes LBK link configuration under rsrc_lock. It also converts
rvu_rep_notify_pfvf_state() from an inline up-mailbox send into a
queued representor event.
Fixes: b8fea84a0468 ("octeontx2-pf: Add support to sync link state between representor and VFs")
Link: https://lore.kernel.org/lkml/aqrInFMnvs4K48+3@kernel-ep2/
A few questions below, mostly about the pieces of the diff that are not
described in the changelog and about the lifetime of the queued events.
> octeontx2-af: Fix rep link state sync and workqueue races
>
> Move rep event workqueue init to rvu_mbox_handler_get_rep_cnt(),
> fix use-after-free and race conditions in rep event handling,
> add bounds checking, and serialize LBK link configuration.
[Severity: Medium]
Could this be split, and could the message describe the behaviour changes
it does not currently mention? The four items listed above do not seem to
cover everything in the diff:
- rvu_rep_notify_pfvf_state() changes from an inline synchronous up-mbox
send into a queued rep_evt_wq item that can now return -EINVAL, -EPERM
or -ENOMEM, or be dropped by the worker. Incidentally, the old code
did mutex_lock(&rvu->mbox_lock) while rvu_afpf_mbox_handler() already
holds that same non-recursive mutex around
__rvu_mbox_handler(mwork, TYPE_AFPF, true), so this conversion also
quietly removes a self-deadlock on the AF-PF path. Should that be
called out as its own fix?
- rvu_rep_up_notify() now sets msg->pcifunc = event->pcifunc and
re-targets PFVF_STATE at the representor PF.
- rvu_rep_install_mcam_rules() now enables LBK links for every PF/VF that
already has NIXLF_INITIALIZED set, and rvu_switch_disable() in rep mode
now walks all PFs/VFs disabling LBK links.
- rvu_switch_enable()'s error path now resets rswitch->entry2pcifunc,
used_entries and start_entry, which fixes a dangling pointer that came
from a different commit than b8fea84a0468.
With a single Fixes tag covering all of these, the get_rep_cnt response
overflow, the missing permission checks and the rswitch bookkeeping fix
cannot be backported separately. Would separate patches with their own
Fixes tags work better here?
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> index 937b085582b5f..4a4e7e434d4b9 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> @@ -3715,12 +3715,29 @@ static int rvu_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>
> static void rvu_remove(struct pci_dev *pdev)
> {
> + struct workqueue_struct *rep_wq;
> struct rvu *rvu = pci_get_drvdata(pdev);
>
> rvu_dbg_exit(rvu);
> rvu_unregister_dl(rvu);
> +
> + /* Block get_rep_cnt() from allocating a new rep_evt_wq. */
> + mutex_lock(&rvu->rsrc_lock);
> + WRITE_ONCE(rvu->rep_evt_teardown, true);
> + rep_wq = rvu->rep_evt_wq;
> + WRITE_ONCE(rvu->rep_evt_wq, NULL);
> + mutex_unlock(&rvu->rsrc_lock);
> +
> rvu_unregister_interrupts(rvu);
> rvu_flr_wq_destroy(rvu);
> +
> + /* Flush both mbox workqueues before destroying rep_wq. */
> + flush_workqueue(rvu->afpf_wq_info.mbox_wq);
> + if (rvu->afvf_wq_info.mbox_wq)
> + flush_workqueue(rvu->afvf_wq_info.mbox_wq);
> + if (rep_wq)
> + destroy_workqueue(rep_wq);
> +
> rvu_cgx_exit(rvu);
> rvu_fwdata_exit(rvu);
> rvu_mcs_exit(rvu);
[ ... ]
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> index a2781e0f504e3..705821fbf011b 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> @@ -44,6 +44,8 @@ static int rvu_rep_up_notify(struct rvu *rvu, struct rep_event *event)
> if (event->event & RVU_EVENT_MAC_ADDR_CHANGE)
> ether_addr_copy(pfvf->mac_addr, event->evt_data.mac);
>
> + if (event->event & RVU_EVENT_PFVF_STATE)
> + pf = rvu_get_pf(rvu->pdev, event->hdr.pcifunc);
> mutex_lock(&rvu->mbox_lock);
> msg = otx2_mbox_alloc_msg_rep_event_up_notify(rvu, pf);
> if (!msg) {
> @@ -53,6 +55,10 @@ static int rvu_rep_up_notify(struct rvu *rvu, struct rep_event *event)
>
> msg->hdr.pcifunc = event->pcifunc;
> msg->event = event->event;
> + msg->pcifunc = event->pcifunc;
[Severity: Low]
This isn't a bug introduced by this patch, but the producer and consumer
still seem to disagree about the pcifunc selector for the non-PFVF_STATE
events. For a VF representee, rvu_rep_up_notify() routes with
pf = rvu_get_pf(rvu->pdev, event->pcifunc) and stamps
msg->hdr.pcifunc = event->pcifunc, so the up-message carries VF function
bits. On the receiving side the parent PF drops it:
drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c:otx2_pfaf_mbox_up_handler() {
...
devid = msg->pcifunc & RVU_PFVF_FUNC_MASK;
/* Skip processing VF's messages */
if (!devid)
otx2_process_mbox_msg_up(pf, msg);
...
}
and the forwarded copy hits the default case of
otx2vf_process_mbox_msg_up(), which answers with otx2_reply_invalid_msg()
and -ENODEV. Since this patch adds a VF bail-out in rvu_rep_open() and
rvu_rep_stop() but leaves rvu_rep_change_mtu() and
rvu_rep_dl_port_fn_hw_addr_set() emitting MTU/MAC events with a VF
pcifunc, the emit sites are now inconsistent. Is changing the MTU on a
VF representor netdev expected to be propagated at all?
> +
> + if (event->event & RVU_EVENT_PFVF_STATE)
> + msg->hdr.pcifunc = event->hdr.pcifunc;
>
> memcpy(&msg->evt_data, &event->evt_data, sizeof(struct rep_evt_data));
>
> @@ -87,7 +93,12 @@ static void rvu_rep_wq_handler(struct work_struct *work)
>
> event = &qentry->event;
>
> - rvu_rep_up_notify(rvu, event);
> + /* Once teardown has started the AF-PF mbox interrupt may
> + * already be disabled, so sending would just block until
> + * otx2_mbox_wait_for_rsp() times out. Drop the event instead.
> + */
> + if (!READ_ONCE(rvu->rep_evt_teardown))
> + rvu_rep_up_notify(rvu, event);
[Severity: Medium]
Can the worker still hit the stall this comment describes? The check and
the send are not atomic with respect to rvu_remove():
CPU0 rvu_rep_wq_handler()
READ_ONCE(rvu->rep_evt_teardown) /* false */
CPU1 rvu_remove()
WRITE_ONCE(rvu->rep_evt_teardown, true);
WRITE_ONCE(rvu->rep_evt_wq, NULL);
rvu_unregister_interrupts(rvu); /* AF-PF mbox irq gone */
CPU0 then proceeds into rvu_rep_up_notify(), which does
otx2_mbox_wait_for_rsp(&rvu->afpf_wq_info.mbox_up, pf);
with rvu->mbox_lock held, and no response interrupt can be serviced, so
it waits the full MBOX_RSP_TIMEOUT. The following destroy_workqueue()
then waits on that worker, and rvu_afpf_mbox_handler() needs the same
rvu->mbox_lock. Would draining rep_evt_wq before
rvu_unregister_interrupts() avoid this?
> kfree(qentry);
> } while (1);
> }
> @@ -95,16 +106,28 @@ static void rvu_rep_wq_handler(struct work_struct *work)
> int rvu_mbox_handler_rep_event_notify(struct rvu *rvu, struct rep_event *req,
> struct msg_rsp *rsp)
> {
> + struct workqueue_struct *wq;
> struct rep_evtq_ent *qentry;
>
> - /* The mailbox dispatcher normalises only the header pcifunc; the
> - * nested struct rep_event::pcifunc body field is sender-controlled
> - * and is later used by rvu_rep_up_notify() to index rvu->pf[] /
> - * rvu->hwvf[]. Reject out-of-range body selectors before queueing.
> - */
> + wq = smp_load_acquire(&rvu->rep_evt_wq);
> + if (!wq)
> + return -EINVAL;
> +
> + /* Only the registered representor PF may send REP_EVENT_NOTIFY. */
> + if (req->hdr.pcifunc != rvu->rep_pcifunc)
> + return -EPERM;
[Severity: Medium]
What does this check authenticate? rvu->rep_pcifunc is set by whichever
PF sends GET_REP_CNT first, and the only identity helper is circular:
drivers/net/ethernet/marvell/octeontx2/af/rvu.h:is_rep_dev() {
if (rvu->rep_pcifunc && rvu->rep_pcifunc == pcifunc)
return true;
...
}
Nothing ties rep_pcifunc to the PCI_DEVID_RVU_REP function, so any PF
that wins the race becomes the representor. It then learns the whole
PF/VF map, can rewrite pfvf->mac_addr of any CGX-mapped PF/VF through
RVU_EVENT_MAC_ADDR_CHANGE in rvu_rep_up_notify(), inject MTU/port-state
up-messages into other functions' mailboxes, and is treated as the
representor by the from_rep_dev target selection in
rvu_mbox_handler_npc_install_flow(). Meanwhile the real representor
driver's probe fails, because rvu_rep_get_rep_map() returns -EPERM for
it. Should the AF validate the caller's device id before accepting the
registration?
> +
> if (!is_pf_func_valid(rvu, req->pcifunc))
> return -EINVAL;
>
> + /* Only CGX-mapped PFs are present in the representor map. */
> + if (!is_pf_cgxmapped(rvu, rvu_get_pf(rvu->pdev, req->pcifunc)))
> + return -EINVAL;
> +
> + if ((req->event & RVU_EVENT_PFVF_STATE) &&
> + rvu_get_pf(rvu->pdev, req->hdr.pcifunc) >= rvu->hw->total_pfs)
> + return -EINVAL;
> +
> qentry = kmalloc_obj(*qentry, GFP_ATOMIC);
> if (!qentry)
> return -ENOMEM;
> @@ -113,37 +136,23 @@ int rvu_mbox_handler_rep_event_notify(struct rvu *rvu, struct rep_event *req,
> spin_lock(&rvu->rep_evtq_lock);
> list_add_tail(&qentry->node, &rvu->rep_evtq_head);
> spin_unlock(&rvu->rep_evtq_lock);
> - queue_work(rvu->rep_evt_wq, &rvu->rep_evt_work);
> + queue_work(wq, &rvu->rep_evt_work);
> return 0;
> }
>
> int rvu_rep_notify_pfvf_state(struct rvu *rvu, u16 pcifunc, bool enable)
> {
> - struct rep_event *req;
> - int pf;
> + struct rep_event req = { 0 };
> + struct msg_rsp rsp;
>
> if (!is_pf_cgxmapped(rvu, rvu_get_pf(rvu->pdev, pcifunc)))
> return 0;
>
> - pf = rvu_get_pf(rvu->pdev, rvu->rep_pcifunc);
> -
> - mutex_lock(&rvu->mbox_lock);
> - req = otx2_mbox_alloc_msg_rep_event_up_notify(rvu, pf);
> - if (!req) {
> - mutex_unlock(&rvu->mbox_lock);
> - return -ENOMEM;
> - }
> -
> - req->hdr.pcifunc = rvu->rep_pcifunc;
> - req->event |= RVU_EVENT_PFVF_STATE;
> - req->pcifunc = pcifunc;
> - req->evt_data.vf_state = enable;
> -
> - otx2_mbox_wait_for_zero(&rvu->afpf_wq_info.mbox_up, pf);
> - otx2_mbox_msg_send_up(&rvu->afpf_wq_info.mbox_up, pf);
> -
> - mutex_unlock(&rvu->mbox_lock);
> - return 0;
> + req.hdr.pcifunc = rvu->rep_pcifunc;
> + req.event = RVU_EVENT_PFVF_STATE;
> + req.pcifunc = pcifunc;
> + req.evt_data.vf_state = enable;
> + return rvu_mbox_handler_rep_event_notify(rvu, &req, &rsp);
> }
[Severity: Medium]
Does routing this through the worker make PF/VF state changes wait for a
mailbox response that may never come? The old code only did
otx2_mbox_wait_for_zero() plus otx2_mbox_msg_send_up() and never waited,
while rvu_rep_up_notify() ends with
otx2_mbox_wait_for_rsp(&rvu->afpf_wq_info.mbox_up, pf);
mutex_unlock(&rvu->mbox_lock);
i.e. up to MBOX_RSP_TIMEOUT with rvu->mbox_lock held, and
rvu_afpf_mbox_handler() takes that same mutex for every AF-PF batch.
rvu->rep_mode and rvu->rep_pcifunc are only cleared by an explicit
ESW_CFG message, so if the representor PF disappears abnormally (FLR,
crash, forced unbind, guest reset of a passed-through PF), every
representee nix_lf_start_rx/stop_rx/teardown still enqueues an event, and
each one is retried for the full timeout. rvu_rep_wq_handler() only
skips the send when rvu->rep_evt_teardown is set, and that happens
exclusively in rvu_remove(). Should the AF stop treating a gone
representor as registered?
[Severity: High]
Can this asynchronous handoff outlive the representor netdevs? Delivery
now happens from the rep worker at an unbounded time after the producing
mailbox handler returned, and on the NIC side teardown is reachable at
runtime:
drivers/net/ethernet/marvell/octeontx2/nic/otx2_devlink.c:otx2_devlink_eswitch_mode_set() {
case DEVLINK_ESWITCH_MODE_LEGACY:
rvu_rep_destroy(pfvf);
...
}
drivers/net/ethernet/marvell/octeontx2/nic/rep.c:rvu_rep_destroy() {
...
free_netdev(rep->netdev);
kfree(rep->flow_cfg);
}
kfree(priv->reps);
}
priv->reps is not set to NULL and priv->rep_cnt / priv->rep_pf_map[] stay
intact, and nothing drains rvu->rep_evt_wq at this point:
rvu_mbox_handler_esw_cfg() only clears rvu->rep_mode, and the worker's
new guard tests rvu->rep_evt_teardown, which only rvu_remove() sets. A
still-queued event therefore reaches rvu_rep_state_evt_handler(), where
rvu_rep_get_repid() still returns a valid index and the handler reads the
freed priv->reps array and writes rep->flags through freed memory. Is
some flush of the AF representor event queue needed before
rvu_rep_destroy() frees the reps?
>
> #define RVU_LF_RX_STATS(reg) \
> @@ -325,6 +334,7 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
> u16 start = rswitch->start_entry;
> struct rvu_hwinfo *hw = rvu->hw;
> u16 pcifunc, entry = 0;
> + struct rvu_pfvf *pfvf;
> int pf, vf, numvfs;
> int err, nixlf, i;
> u8 rep;
> @@ -334,19 +344,22 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
> continue;
>
> pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
> + pfvf = rvu_get_pfvf(rvu, pcifunc);
> rvu_get_nix_blkaddr(rvu, pcifunc);
> + if (test_bit(NIXLF_INITIALIZED, &pfvf->flags))
> + rvu_switch_enable_lbk_link(rvu, pcifunc, true);
[Severity: Medium]
Should this path also publish RVU_EVENT_PFVF_STATE for the functions it
finds already initialized? The LBK link is enabled here, but
rvu_rep_notify_pfvf_state() has only three callers
(rvu_mbox_handler_nix_lf_start_rx(), nix_lf_stop_rx() and
rvu_nix_lf_teardown()), all gated on rvu->rep_mode already being set.
So for a PF/VF that was brought up before switchdev mode was enabled, no
state event is ever generated. rvu_rep_create() allocates rep_dev with
flags == 0, rvu_rep_state_evt_handler() is the only writer of
RVU_REP_VF_INITIALIZED, and rvu_rep_open() starts with
if (!(rep->flags & RVU_REP_VF_INITIALIZED))
return 0;
so the representor netdev never gets netif_carrier_on() or
netif_tx_start_all_queues() and its stats stay empty until the
representee interface is bounced. Does the initial-state half of the
link state sync still work in that ordering?
> rep = true;
> for (i = 0; i < 2; i++) {
> err = rvu_rep_install_rx_rule(rvu, pcifunc,
> start + entry, rep);
> if (err)
> - return err;
> + goto err_disable_lbk;
[ ... ]
> +err_disable_lbk:
> + /* Undo any LBK links enabled above before the MCAM rule failure.
> + * Disabling a link that was never enabled is a safe no-op.
> + */
> + for (pf = 1; pf < hw->total_pfs; pf++) {
> + if (!is_pf_cgxmapped(rvu, pf))
> + continue;
> + pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
> + rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> + rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
> + for (vf = 0; vf < numvfs; vf++) {
> + pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
> + rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> + }
> }
> - return 0;
> + return err;
> }
[Severity: Medium]
This is a pre-existing issue, but does this rollback also need to release
the TX VTAG entries? Every rvu_rep_install_tx_rule() that already
succeeded allocated and programmed a NIX TX VTAG definition through
rvu_rep_tx_vlan_cfg() -> nix_tx_vtag_cfg() -> nix_tx_vtag_alloc():
drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c:rvu_rep_tx_vlan_cfg() {
err = rvu_mbox_handler_nix_vtag_cfg(rvu, &req, &rsp);
...
*vidx = rsp.vtag0_idx;
}
Those entries are not freed by the new err_disable_lbk loop, not by
npc_delete_flow in rvu_switch_enable()'s uninstall path, and not by the
rep_mode branch of rvu_switch_disable(), which goes straight to
free_ents. With NIX_TX_VTAG_DEF_MAX at 0x400 and two entries per
representee per enable, repeated eswitch mode toggles (or repeated failed
enables) would eventually return NIX_AF_ERR_TX_VTAG_NOSPC until the
representee NIX LFs are freed. Would freeing them alongside the LBK
rollback be reasonable?
[ ... ]
> @@ -443,35 +466,111 @@ int rvu_mbox_handler_esw_cfg(struct rvu *rvu, struct esw_cfg_req *req,
[ ... ]
> + /* Only a PF can register as the representor, not a VF. */
> + if (req->hdr.pcifunc & RVU_PFVF_FUNC_MASK) {
> + ret = -EPERM;
> + goto unlock;
> + }
[ ... ]
> + /* Initialize the wq for handling REP events */
> + spin_lock_init(&rvu->rep_evtq_lock);
> + INIT_LIST_HEAD(&rvu->rep_evtq_head);
> + INIT_WORK(&rvu->rep_evt_work, rvu_rep_wq_handler);
> + wq = alloc_workqueue("rep_evt_wq", WQ_UNBOUND, 0);
> + if (!wq) {
> + dev_err(rvu->dev, "REP workqueue allocation failed\n");
> + devm_kfree(rvu->dev, map);
> + ret = -ENOMEM;
> + goto unlock;
> + }
[Severity: Medium]
What happens to this workqueue if rvu_probe() fails after
rvu_register_interrupts() has enabled the AF-PF mailbox interrupts? A PF
sending GET_REP_CNT in that window allocates and publishes rep_evt_wq
here, but the probe unwind
err_dl:
rvu_unregister_dl(rvu);
err_irq:
rvu_unregister_interrupts(rvu);
err_flr:
rvu_flr_wq_destroy(rvu);
err_mbox:
rvu_mbox_destroy(&rvu->afpf_wq_info);
...
devm_kfree(dev, rvu);
never drains or destroys it, so the workqueue is leaked. Worse,
rvu->rep_evt_work, rvu->rep_evtq_head and rvu->rep_evtq_lock all live
inside the rvu structure that devm_kfree() releases, so a REP_EVENT_NOTIFY
queued in the same window would run the worker against freed memory. The
teardown added by this patch only exists in rvu_remove(), which is not
called for a failed probe.
> +
> + rvu->rep_cnt = cnt;
> + rsp->rep_cnt = cnt;
> + rvu->rep2pfvf_map = map;
> + rvu->rep_pcifunc = req->hdr.pcifunc;
> +
> + /* Pairs with smp_load_acquire() in rvu_mbox_handler_rep_event_notify()
> + * to publish the above initialization before wq becomes visible.
> + */
> + smp_store_release(&rvu->rep_evt_wq, wq);
> +unlock:
> + mutex_unlock(&rvu->rsrc_lock);
> + return ret;
> }
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
> index 49ce38685a7e6..92719399a953f 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
> @@ -203,10 +210,16 @@ void rvu_switch_enable(struct rvu *rvu)
> return;
>
> uninstall_rules:
> + if (rvu->rep_mode && rvu->rep_pcifunc)
> + rvu_switch_enable_lbk_link(rvu, rvu->rep_pcifunc, false);
> +
> uninstall_req.start = rswitch->start_entry;
> uninstall_req.end = rswitch->start_entry + rswitch->used_entries - 1;
> rvu_mbox_handler_npc_delete_flow(rvu, &uninstall_req, &uninstall_rsp);
> kfree(rswitch->entry2pcifunc);
> + rswitch->entry2pcifunc = NULL;
> + rswitch->used_entries = 0;
> + rswitch->start_entry = 0;
[Severity: High]
Can this order of stores turn the existing unlocked access into a NULL
dereference? entry2pcifunc is cleared before used_entries, and the
readers run from the mailbox workqueues with no lock in common with the
devlink path:
drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c:rvu_rep_update_rules() {
u32 max = rswitch->used_entries;
...
if (!rswitch->used_entries)
return;
...
for (entry = 0; entry < max; entry++) {
if (rswitch->entry2pcifunc[entry] == pcifunc)
...
}
CPU0 rvu_devlink_eswitch_mode_set() -> rvu_switch_enable()
kfree(rswitch->entry2pcifunc);
rswitch->entry2pcifunc = NULL;
rswitch->used_entries = 0;
CPU1 rvu_mbox_handler_nix_lf_start_rx() -> rvu_switch_update_rules()
-> rvu_rep_update_rules() /* used_entries still non-zero */
rswitch->entry2pcifunc[entry] /* NULL deref */
rswitch->switch_lock is only taken inside
rvu_mbox_handler_npc_install_flow(), not by these readers or writers.
Ordering the stores the other way (as rvu_switch_disable() does) still
leaves a window, so would real serialization of the rswitch bookkeeping
be needed here?
> free_entries:
> free_req.all = 1;
> rvu_mbox_handler_npc_mcam_free_entry(rvu, &free_req, &rsp);
[ ... ]
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> index 0f5d5642d3f7a..ef47e7e219016 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> @@ -459,6 +465,9 @@ static int rvu_rep_open(struct net_device *dev)
> netif_carrier_on(dev);
> netif_tx_start_all_queues(dev);
>
> + if (rep->pcifunc & RVU_PFVF_FUNC_MASK)
> + return 0;
> +
[Severity: Low]
This bail-out (and the identical one in rvu_rep_stop()) stops VF
representors from emitting RVU_EVENT_PORT_STATE at all. As far as I can
tell nothing consumed those events before either: the parent PF skips
up-messages whose pcifunc has function bits set in
otx2_pfaf_mbox_up_handler(), and otx2vf_process_mbox_msg_up() only
implements MBOX_MSG_CGX_LINK_EVENT and replies with -ENODEV for
REP_EVENT_UP_NOTIFY. Is the intent to remove a send that never had a
receiver? If so, could the changelog say so, since the subject mentions
fixing rep link state sync?
> evt.event = RVU_EVENT_PORT_STATE;
> evt.evt_data.port_state = 1;
> evt.pcifunc = rep->pcifunc;
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917042057.1627523-1-nshettyj%40marvell.com
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-09-21 4:33 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-17 4:20 [PATCH net v4] octeontx2-af: Fix rep link state sync and workqueue races nshettyj
2026-09-21 4:33 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®