Re: [v9,net-next,4/8] octeontx2-af: switch: Representor for switch port

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 2 (2) · Medium: 1 · Low: 1

- [High] esw_cfg_req_has_switch_id()
  (drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c:442-455) derives…
- [High] Lockless access to rvu->rep2pfvf_map in rvu_sw_port_id() races
  with GET_REP_CNT mailbox handler updates, leading to a NULL pointer…
- [Medium] struct esw_cfg_req — an existing AF<->PF mailbox wire-format
  message carried in the shared mailbox window — grows a 32-byte…
- [Low] The patch adds rvu_sw_port_id(), RVU_SWITCH_FLAG_FW_READY and the
  rvu_switch.flags/pcifunc fields with no producer or consumer at this…

Pre-existing issues:
- [High] rvu_mbox_handler_get_rep_cnt() (af/rvu_rep.c:474-507, unchanged
  at the end of the series) re-allocates rvu->rep2pfvf_map with…
- [High] The new switch_id ingestion and rvu_sw_port_id() build on
  rvu->rep_pcifunc, rvu->rep_cnt and rvu->rep2pfvf_map, all of which any…

--

octeontx2-af: switch: Representor for switch port

This adds a devlink-derived switch id to the esw_cfg mailbox message, copies
it into rvu->rswitch on the AF side, and adds rvu_sw_port_id() plus an export
of rvu_rep_get_vlan_id().  A few questions below about the new mailbox field
and the state the new helper depends on.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> index e45e6e93ed087..8e3850f33751d 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> @@ -1841,6 +1841,7 @@ struct esw_cfg_req {
>  	struct mbox_msghdr hdr;
>  	u8 ena;
>  	u64 rsvd;
> +	unsigned char switch_id[MAX_PHYS_ITEM_ID_LEN];
>  };

[Severity: Medium]

This changes the layout of an existing AF<->PF mailbox message, growing
struct esw_cfg_req from 32 to 64 bytes, but OTX2_MBOX_VERSION in the same
header stays at 0x000a.

The existing precedent in this driver for a message layout extension is to
gate on the header version, for example in af/rvu.c:

	if (attach->hdr.ver < RVU_MULTI_BLK_VER)

and rvu_process_mbox_msg() only validates req->sig, never req->ver.  With
the version unchanged, an old PF and a new AF both advertise ver = 0x000a
for two incompatible layouts.  Should OTX2_MBOX_VERSION be bumped and
checked here instead?

Two related points on the field itself:

The on-the-wire size of this message is now derived from
MAX_PHYS_ITEM_ID_LEN, a core netdevice constant outside this driver's
control.  Is it intentional that the mailbox layout follows that constant?

No length accompanies the value.  rvu_rep_devlink_set_switch_id() in
nic/rep.c sets ppid->id_len = sizeof(u64), and rvu_eswitch_config() copies
only that many bytes:

	memcpy(req->switch_id, attrs.switch_id.id, attrs.switch_id.id_len);

while the AF stores and would compare all MAX_PHYS_ITEM_ID_LEN bytes.  How
does the AF recover the meaningful prefix, i.e. should an id_len companion
be carried in the message and in struct rvu_switch?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> index 2876c76ae61b6..9174b879850a2 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> @@ -576,6 +576,10 @@ struct rvu_switch {
>  	u16 *entry2pcifunc;
>  	u16 mode;
>  	u16 start_entry;
> +	unsigned char switch_id[MAX_PHYS_ITEM_ID_LEN];
> +#define RVU_SWITCH_FLAG_FW_READY BIT_ULL(0)
> +	u64 flags;
> +	u16 pcifunc;
>  };

[Severity: Low]

This isn't a bug, but rvu_sw_port_id(), RVU_SWITCH_FLAG_FW_READY and
rvu_switch.pcifunc have no producer or consumer at this commit; they do gain
users in the later patches of the series, which reads like normal series
decomposition.

The one field that stays unused through the end of the series is
rvu_switch.switch_id: it is written by rvu_mbox_handler_esw_cfg() and never
read anywhere.  Would it be clearer to add that store together with the code
that consumes it?

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> index a2781e0f504e3..672d54847c7b1 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c

[ ... ]

> @@ -189,7 +190,7 @@ int rvu_mbox_handler_nix_lf_stats(struct rvu *rvu,
>  	return 0;
>  }
>  
> -static u16 rvu_rep_get_vlan_id(struct rvu *rvu, u16 pcifunc)
> +u16 rvu_rep_get_vlan_id(struct rvu *rvu, u16 pcifunc)
>  {
>  	int id;
>  
> @@ -429,6 +430,30 @@ int rvu_rep_pf_init(struct rvu *rvu)
>  	return 0;
>  }
>  
> +/* ESW_CFG is always the sole message in a mailbox transaction.
> + *
> + * The otx2 mailbox API does not batch multiple messages per sync: the
> + * representor driver allocates only ESW_CFG before calling
> + * otx2_sync_mbox_msg() (see rvu_eswitch_config()), and the AF processes
> + * one message per dispatch. next_msgoff is therefore the end offset of this
> + * message, not a cumulative offset across batched messages, so the length
> + * check below is safe. Batching is not supported; do not flag this path.
> + */
> +static bool esw_cfg_req_has_switch_id(const struct esw_cfg_req *req)
> +{
> +	u16 hdr_len = ALIGN(sizeof(struct mbox_hdr), MBOX_MSG_ALIGN);
> +	u16 next_off = req->hdr.next_msgoff;
> +	u16 msg_len;
> +
> +	if (next_off < hdr_len)
> +		return false;
> +
> +	msg_len = next_off - hdr_len;
> +
> +	return msg_len >= offsetof(struct esw_cfg_req, switch_id) +
> +			  MAX_PHYS_ITEM_ID_LEN;
> +}
> +
>  int rvu_mbox_handler_esw_cfg(struct rvu *rvu, struct esw_cfg_req *req,
>  			     struct msg_rsp *rsp)
>  {
> @@ -436,6 +461,9 @@ int rvu_mbox_handler_esw_cfg(struct rvu *rvu, struct esw_cfg_req *req,
>  		return 0;
>  
>  	rvu->rep_mode = req->ena;
> +	if (esw_cfg_req_has_switch_id(req))
> +		memcpy(rvu->rswitch.switch_id, req->switch_id,
> +		       MAX_PHYS_ITEM_ID_LEN);

[Severity: High]

Is the premise of the comment above esw_cfg_req_has_switch_id() correct?
otx2_mbox_alloc_msg_rsp() in af/mbox.c derives next_msgoff from the running
total of the whole mailbox region:

	mdev->msg_size += size;
	mdev->rsp_size += size_rsp;
	msghdr->next_msgoff = mdev->msg_size + msgs_offset;

and __rvu_mbox_handler() in af/rvu.c walks a batch of messages per doorbell
using it:

	for (id = 0; id < mw->mbox_wrk[devid].num_msgs; id++) {
		msg = mdev->mbase + offset;
		...
		offset = mbox->rx_start + msg->next_msgoff;
	}

So if ESW_CFG is not the first message in a transaction, wouldn't
msg_len = next_msgoff - hdr_len exceed this message's real size and let the
check pass for an old 32-byte esw_cfg_req?

Beyond batching, next_msgoff lives in the shared mailbox window and is
written by the peer.  rvu_process_mbox_msg() checks only req->sig, and
otx2_pfvf_mbox_handler() in nic/otx2_pf.c forwards a VF's headers after
rewriting just pcifunc:

	if (msg->sig != OTX2_MBOX_REQ_SIG)
		goto inval_msg;
	msg->pcifunc &= ~RVU_PFVF_FUNC_MASK;
	msg->pcifunc |= (vf_idx + 1) & RVU_PFVF_FUNC_MASK;
	offset = msg->next_msgoff;

with otx2_forward_vf_mbox_msgs() handing the VF's own mailbox memory to the
AF verbatim (dst_mdev->mbase = src_mdev->mbase).

Can a VF that writes next_msgoff >= 80 and sends a 32-byte ESW_CFG make the
memcpy above read 32 bytes past the end of its message, picking up the
following message's header and payload, or bytes past the last message and
potentially past the per-device mailbox window, and persist them as the AF's
switch identity?

Since hdr.ver / OTX2_MBOX_VERSION exists to signal exactly this kind of
layout skew, would a version check be the safer bound here rather than a
peer-supplied offset?

One more note on the comment: the closing sentence "Batching is not
supported; do not flag this path" instructs readers not to inspect this
code.  Could that be dropped?

>  
>  	if (!rvu->rep_mode)
>  		rvu_npc_free_mcam_entries(rvu, req->hdr.pcifunc, -1);
> @@ -449,6 +477,9 @@ int rvu_mbox_handler_get_rep_cnt(struct rvu *rvu, struct msg_req *req,
>  	int pf, vf, numvfs, hwvf, rep = 0;
>  	u16 pcifunc;
>  
> +	/* Called once from representor driver probe during devlink eswitch
> +	 * SWITCHDEV bring-up; not re-run during switch device operation.
> +	 */
>  	rvu->rep_pcifunc = req->hdr.pcifunc;
>  	rsp->rep_cnt = rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs;
>  	rvu->rep_cnt = rsp->rep_cnt;

[Severity: High]

This isn't a bug introduced by this patch, but the new comment states this
handler is called once, and the new rvu_sw_port_id() is documented as relying
on that.  Is the invariant enforced anywhere?

rvu_mbox_handler_get_rep_cnt() re-runs on every MBOX_MSG_GET_REP_CNT:

	rvu->rep_cnt = rsp->rep_cnt;

	rvu->rep2pfvf_map = devm_kzalloc(rvu->dev, rvu->rep_cnt *
					 sizeof(u16), GFP_KERNEL);
	if (!rvu->rep2pfvf_map)
		return -ENOMEM;

Each repetition drops the previous pointer without devm_kfree(), and the old
buffer is devres-managed against the AF device, so it is only released when
the AF itself is removed.  Does a representor PF unbind/rebind or module
reload, or a VF repeating the message, grow the devres list without bound?

On the -ENOMEM path, rvu->rep_cnt is already published while
rvu->rep2pfvf_map is NULL, and rvu_rep_get_vlan_id() dereferences the map
with no NULL check:

	for (id = 0; id < rvu->rep_cnt; id++)
		if (rvu->rep2pfvf_map[id] == pcifunc)
			return id;

Can that NULL-dereference in the pre-existing AF callers of
rvu_rep_get_vlan_id() after a failed re-initialization?  Note that
rvu_sw_port_id() checks rvu->rep2pfvf_map itself, but its callee does not.

[Severity: High]

This is a pre-existing issue, but the new switch_id ingestion and
rvu_sw_port_id() build on rvu->rep_pcifunc, rvu->rep_cnt and
rvu->rep2pfvf_map, all of which this handler sets with no capability or
identity check:

	rvu->rep_pcifunc = req->hdr.pcifunc;

rvu_process_mbox_msg() applies no per-message privilege filter, and
otx2_pfvf_mbox_handler() forwards raw VF message IDs after checking only
sig.  Can a guest-owned VF send MBOX_MSG_GET_REP_CNT to become
rvu->rep_pcifunc, and then pass the gate in rvu_mbox_handler_esw_cfg():

	if (req->hdr.pcifunc != rvu->rep_pcifunc)
		return 0;

	rvu->rep_mode = req->ena;

thereby setting the AF-global rvu->rep_mode, driving
rvu_npc_free_mcam_entries(), and reaching the new switch_id memcpy?

Also, rvu_afvf_mbox_handler() does not take rvu->mbox_lock while the AFPF
worker does.  Can the AFVF path replace rvu->rep2pfvf_map concurrently with
the new lockless readers?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/switch/rvu_sw.c b/drivers/net/ethernet/marvell/octeontx2/af/switch/rvu_sw.c
> index fe143ad3f9449..2451eb57ec4c8 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/switch/rvu_sw.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/switch/rvu_sw.c
> @@ -5,7 +5,33 @@
>   *
>   */
>  
> +#include <linux/bitfield.h>
> +
>  #include "rvu.h"
> +#include "rvu_sw.h"
> +
> +/*
> + * rep_cnt and rep2pfvf_map are populated once when the representor driver
> + * probes via GET_REP_CNT (see rvu_get_rep_cnt() in rep.c), as part of
> + * devlink eswitch SWITCHDEV bring-up. They are not updated during switch
> + * device mailbox handling, so this lockless lookup cannot race with a
> + * concurrent rep2pfvf_map resize.
> + */
> +u32 rvu_sw_port_id(struct rvu *rvu, u16 pcifunc)
> +{
> +	u16 rep_id;
> +
> +	if (!rvu->rep2pfvf_map || !rvu->rep_cnt)
> +		return RVU_SW_INVALID_PORT_ID;
> +
> +	rep_id = rvu_rep_get_vlan_id(rvu, pcifunc);
> +	if (rep_id >= rvu->rep_cnt ||
> +	    rvu->rep2pfvf_map[rep_id] != pcifunc)
> +		return RVU_SW_INVALID_PORT_ID;
> +
> +	return FIELD_PREP(GENMASK_ULL(31, 16), rep_id) |
> +	       FIELD_PREP(GENMASK_ULL(15, 0), pcifunc);
> +}

[Severity: High]
The comment above this helper asserts that rep_cnt and rep2pfvf_map
cannot change under a reader, but rvu_mbox_handler_get_rep_cnt() runs
for every MBOX_MSG_GET_REP_CNT, and it publishes the new count before
the matching array exists:

	rvu->rep_cnt = rsp->rep_cnt;

	rvu->rep2pfvf_map = devm_kzalloc(rvu->dev, rvu->rep_cnt *
					 sizeof(u16), GFP_KERNEL);
	if (!rvu->rep2pfvf_map)
		return -ENOMEM;

So there is a window in which rvu->rep_cnt describes the new topology
while rvu->rep2pfvf_map still points at the previous, smaller buffer.
A reader that passes the entry test here then walks id < rvu->rep_cnt
in rvu_rep_get_vlan_id() and indexes rvu->rep2pfvf_map[rep_id] past the
end of the old allocation.  The rep_id >= rvu->rep_cnt bound added in
this helper does not help, since rep_cnt is exactly the value that has
already been advanced.

The allocation failure path looks worse: rvu->rep2pfvf_map is
overwritten with NULL while rvu->rep_cnt stays non-zero, and nothing
re-checks the pointer after this function's own test - rvu_sw_port_id()
validates rvu->rep2pfvf_map on entry, but rvu_rep_get_vlan_id()
dereferences it unconditionally, so a store that lands between the two
turns the lookup into a NULL dereference.

Neither side takes a lock: rvu_afvf_mbox_handler() does not hold
rvu->mbox_lock, so a VF-originated GET_REP_CNT can execute the
reallocation in parallel with these reads on another CPU.

What is the intended serialization here?  If GET_REP_CNT really can only
arrive once, could that be enforced (reject the message when
rvu->rep2pfvf_map is already set) rather than documented in a comment?
Otherwise the map and its count need to be published together under a
lock or as an RCU-swapped object, and rvu->rep_cnt should not be updated
before the allocation succeeds.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831131944.2649362-1-rkannoth%40marvell.com