Re: [v9,net-next,1/8] octeontx2-af: switch: Add AF to switch mbox and skeleton files

[netdev-bot+sashiko]

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

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

- [High] The four new messages are registered in the generic AF ID range
  (0x010-0x013) and are dispatched by rvu_process_mbox_msg(), which…
- [Medium] rvu_mbox_handler_fl_notify() and
  rvu_mbox_handler_fl_get_stats() (af/switch/rvu_sw_fl.c:9-21) register…

Pre-existing issues:
- [Medium] rvu_process_mbox_msg() casts the received message to `struct
  _req_type *` purely on the basis of the sender-supplied req->id, and…

--

octeontx2-af: switch: Add AF to switch mbox and skeleton files

This patch adds four new AF mailbox messages (FDB_NOTIFY, FIB_NOTIFY,
FL_NOTIFY, FL_GET_STATS) with their request/response structures, plus a new
switch/ subdirectory holding skeleton handler functions that currently just
return 0.

A few questions about the new message IDs and the stub handlers below.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> index cece197d1074b..854696d2a35f1 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> @@ -164,6 +164,14 @@ M(PTP_GET_CAP,		0x00c, ptp_get_cap, msg_req, ptp_get_cap_rsp)	\
>  M(GET_REP_CNT,		0x00d, get_rep_cnt, msg_req, get_rep_cnt_rsp)	\
>  M(ESW_CFG,		0x00e, esw_cfg, esw_cfg_req, msg_rsp)	\
>  M(REP_EVENT_NOTIFY,     0x00f, rep_event_notify, rep_event, msg_rsp) \
> +M(FDB_NOTIFY,		0x010,  fdb_notify,				\
> +				fdb_notify_req, msg_rsp)		\
> +M(FIB_NOTIFY,		0x011,  fib_notify,				\
> +				fib_notify_req, msg_rsp)		\
> +M(FL_NOTIFY,		0x012,  fl_notify,				\
> +				fl_notify_req, msg_rsp)		\
> +M(FL_GET_STATS,		0x013,  fl_get_stats,				\
> +				fl_get_stats_req, fl_get_stats_rsp)	\

[Severity: High]
Should these four IDs be restricted to a privileged sender?

They are added to the generic AF ID range, and rvu_process_mbox_msg() in
af/rvu.c dispatches on req->id after only checking req->sig, with no
per-ID or per-sender filtering. __rvu_mbox_handler() feeds both TYPE_AFPF
and TYPE_AFVF messages into that same dispatcher, and the only
sanitisation it does for a VF is to stamp the sender's own identity:

	case TYPE_AFVF:
		msg->pcifunc &=
			~(RVU_PFVF_FUNC_MASK << RVU_PFVF_FUNC_SHIFT);
		msg->pcifunc |= (devid << RVU_PFVF_FUNC_SHIFT) + 1;
		break;

	err = rvu_process_mbox_msg(mbox, devid, msg);

So any PF and any VF, including a VF assigned to an untrusted guest via
SR-IOV, can issue these switch control-plane messages. None of the new
handlers looks at who sent the message.

The stubs make this harmless today, but as the series makes the handlers
functional, rvu_mbox_handler_fdb_notify() ends up gating only on
RVU_SWITCH_FLAG_FW_READY plus

	port_id = rvu_sw_port_id(rvu, req->hdr.pcifunc);
	if (port_id == RVU_SW_INVALID_PORT_ID)
		return -EINVAL;
	...
	ether_addr_copy(l2_entry->mac, req->mac);
	l2_entry->flags = req->flags;

which every representor-mapped PF and VF satisfies, after which the
caller-supplied MAC and OTX2_FDB_ADD/OTX2_FDB_DEL are programmed toward
the switch for that port. rvu_mbox_handler_fib_notify() similarly accepts
caller-supplied route entries from any PF/VF.

Compare with the sibling handler added in the same series, which does
perform the check:

	if (!rvu_is_switch_pcifunc(rvu, sender))
		return -EPERM;

and with rvu_mbox_handler_esw_cfg() in af/rvu_rep.c, which ignores
requests whose hdr.pcifunc differs from rvu->rep_pcifunc. Would an
equivalent sender check on the new handlers be appropriate?

[ ... ]

> @@ -1812,6 +1820,116 @@ struct rep_event {
>  	struct rep_evt_data evt_data;
>  };
>  
[ ... ]
> +struct fib_notify_req {
> +	struct  mbox_msghdr hdr;
> +	u16 cnt;
> +	u16 rsvd[3]; /* explicit padding for entry[] 8-byte alignment */
> +	struct fib_entry entry[16];
> +};

[Severity: Medium]
This isn't a problem introduced by this patch, but it does enlarge the
worst case for it.

rvu_process_mbox_msg() casts the received message to struct _req_type *
based purely on the sender-supplied req->id:

	err = rvu_mbox_handler_ ## _fn_name(rvu,			\
					    (struct _req_type *)req, \
					    rsp);

and __rvu_mbox_handler() walks the message list using the sender-supplied
offset without bounding it against the region:

		offset = mbox->rx_start + msg->next_msgoff;

struct fib_notify_req is 1176 bytes here, which becomes by far the largest
AF request payload, so it widens how far past a minimally sized message a
handler can be made to read once it dereferences req->entry[]. At the end
of the series rvu_mbox_handler_fib_notify() memcpy()s req->cnt *
sizeof(struct fib_entry) out of the shared mbox region and forwards it to
the switch OS.

The actionable fix here belongs in the mbox core (validating next_msgoff
and the available message size against the declared request type) rather
than in these handlers, so this is noted for information only.

[ ... ]

> +struct fl_get_stats_req {
> +	struct  mbox_msghdr hdr;
> +	u64 cookie;
> +};
> +
> +struct fl_get_stats_rsp {
> +	struct  mbox_msghdr hdr;
> +	u64 pkts_diff;
> +};
> +

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/switch/rvu_sw_fl.c b/drivers/net/ethernet/marvell/octeontx2/af/switch/rvu_sw_fl.c
> new file mode 100644
> index 0000000000000..1f8b82a84a5dd
> --- /dev/null
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/switch/rvu_sw_fl.c
> @@ -0,0 +1,21 @@
[ ... ]
> +int rvu_mbox_handler_fl_get_stats(struct rvu *rvu,
> +				  struct fl_get_stats_req *req,
> +				  struct fl_get_stats_rsp *rsp)
> +{
> +	return 0;
> +}
> +
> +int rvu_mbox_handler_fl_notify(struct rvu *rvu,
> +			       struct fl_notify_req *req,
> +			       struct msg_rsp *rsp)
> +{
> +	return 0;
> +}

[Severity: Medium]
Should these two stubs report success?

Once FL_NOTIFY and FL_GET_STATS are registered in MBOX_MESSAGES, the
dispatcher no longer answers with otx2_reply_invalid_msg(); instead it
presets a success code and only overrides it if the handler fails:

	if (rsp) {
		...
		rsp->hdr.rc = 0;
	}
	err = rvu_mbox_handler_ ## _fn_name(rvu, (struct _req_type *)req, rsp);
	if (rsp && err)
		rsp->hdr.rc = err;

Since otx2_mbox_alloc_msg_rsp() zeroes the whole response region:

	/* Clear the whole msg region */
	memset(msghdr, 0, size);

FL_GET_STATS answers every query with pkts_diff = 0 and rc = 0, so a
requesting peer cannot tell a genuine zero delta from a discarded request
and may age the flow out. Likewise, rvu_mbox_handler_fl_notify() ignores
cookie, flags, features and tuple entirely, yet acknowledges the flow
install or delete as done, so a peer that stops software forwarding after
a successful FL_NOTIFY would blackhole that flow.

Both handlers are still identical at the end of the series, so this is not
a transient state within the series. Would returning -EOPNOTSUPP, or
holding back the ID registration until the handlers are implemented, be
preferable?

[ ... ]

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