Re: [PATCH net v6 2/2] octeontx2-af: mcs: Fix SC resource cleanup loop

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeontx2-af: mcs: Fix SC resource cleanup loop

This changes the SC loops in mcs_clear_all_stats() and mcs_free_all_rsrc()
to use map->sc.max instead of map->secy.max as the loop bound.  Since
mcs_alloc_struct_mem() sets secy.max = hw->secy_entries - MCS_RSRC_RSVD_CNT
while sc.max = hw->sc_entries, the old bound was one short.

> octeontx2-af: mcs: Fix SC resource cleanup loop
>
>     The SC resource cleanup loop was incorrectly iterating over secy.max
>     instead of sc.max. Use sc.max as the loop bound.

[Severity: Medium]
The message describes a single "SC resource cleanup loop", but the diff
changes two loops in two different functions: the SC statistics loop in
mcs_clear_all_stats() and the SC free loop in mcs_free_all_rsrc().  Could
the message mention both call sites, since one of them is statistics
clearing rather than resource cleanup?

Could the message also state the observable consequence?  The numbers make
this look like an off-by-one that skipped exactly the highest SC id:

mcs_alloc_struct_mem():
	res->secy.max = hw->secy_entries - MCS_RSRC_RSVD_CNT;
	...
	res->sc.max = hw->sc_entries;

with MCS_RSRC_RSVD_CNT == 1, and both HW variants setting
secy_entries == sc_entries (128 for cn10kb, 64 for cnf10kb).  So SC id 127
(63 on cnf10kb) could be handed out by rvu_alloc_rsrc() but was never
freed on teardown or FLR, its sc2pf_map[] entry stayed owned by a dead
pcifunc, its RX SC CAM entry stayed enabled because
mcs_ena_dis_sc_cam_entry() was skipped, and its stats were never cleared
for the next owner.

That information seems useful for anyone deciding whether to backport this.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
> index 211c10aa5880f..30c672d526a0c 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c

[ ... ]

> @@ -738,7 +738,7 @@ int mcs_free_all_rsrc(struct mcs *mcs, int dir, u16 pcifunc)
>  	}
>  
>  	/* free sc entries */
> -	for (id = 0; id < map->secy.max; id++) {
> +	for (id = 0; id < map->sc.max; id++) {
>  		if (map->sc2pf_map[id] != pcifunc)
>  			continue;
>  		mcs_free_rsrc(&map->sc, map->sc2pf_map, id, pcifunc);

[Severity: Low]
This isn't a bug introduced by this patch, but the new bound now passes
sc_id == 127 into mcs_ena_dis_sc_cam_entry() a few lines below, in the
dir == MCS_RX case.  That helper picks the second register but shifts by
the unmasked index:

mcs_ena_dis_sc_cam_entry() {
	reg = MCSX_CPM_RX_SLAVE_SC_CAM_ENA(0);
	if (sc_id > 63)
		reg = MCSX_CPM_RX_SLAVE_SC_CAM_ENA(1);

	if (ena)
		val = mcs_reg_read(mcs, reg) | BIT_ULL(sc_id);
	else
		val = mcs_reg_read(mcs, reg) & ~BIT_ULL(sc_id);
}

For sc_id in 64..127 (cn10kb sets hw->sc_entries = 128) the shift count is
at least 64, which is undefined in C, so this can produce a UBSAN "shift
exponent too large" report.  On arm64 the variable shift is masked to six
bits and 127 % 64 == 63, so the bit that gets touched happens to be the
intended one, and the enable side via mcs_rx_sc_cam_write() uses the same
idiom, so the two stay consistent.  Would BIT_ULL(sc_id % 64) be a
worthwhile follow-up cleanup here and in mcs_ena_dis_flowid_entry()?

[Severity: High]
This is a pre-existing issue, but this patch widens the region it applies
to from map->secy.max to map->sc.max entries, so it may be worth a look.
Is mcs_free_all_rsrc() safe to call without rvu->rsrc_lock?

The mailbox paths serialize the same bitmaps and ownership arrays:

rvu_mbox_handler_mcs_alloc_resources()
   mutex_lock(&rvu->rsrc_lock);
   ... mcs_alloc_rsrc() -> rvu_alloc_rsrc() -> __set_bit()

rvu_mbox_handler_mcs_free_resources()
   mutex_lock(&rvu->rsrc_lock);
   ... mcs_free_rsrc() / mcs_free_all_rsrc()

The FLR path does not:

__rvu_flr_handler()
   mutex_lock(&rvu->flr_lock);
   ...
   rvu_mcs_flr_handler()
      mcs_free_all_rsrc(mcs, MCS_RX, pcifunc);
      mcs_free_all_rsrc(mcs, MCS_TX, pcifunc);

rvu_alloc_rsrc() and rvu_free_rsrc() use the non-atomic __set_bit() /
__clear_bit(), so the read-modify-write of a bitmap word in the FLR work
item can race with another function's mailbox thread, and both sides also
write the same pf_map[] array.

Can a lost update here hand the same SC/SA/SECY/flowid id to two different
pcifuncs, or leak an id permanently?

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but
mcs_free_rsrc() is the callee whose use this loop widens, so mentioning it
here.  Is req->rsrc_id range checked anywhere before it reaches this
function?

rvu_mbox_handler_mcs_free_resources() validates only req->mcs_id and then
passes the caller-supplied u8 rsrc_id (0-255, struct mcs_free_rsrc_req)
straight through:

	switch (req->rsrc_type) {
	case MCS_RSRC_TYPE_FLOWID:
		rc = mcs_free_rsrc(&map->flow_ids, map->flowid2pf_map, req->rsrc_id, pcifunc);
		mcs_ena_dis_flowid_entry(mcs, req->rsrc_id, req->dir, false);

mcs_free_rsrc() then dereferences it with no bound against rsrc->max:

	/* Check if the rsrc_id is mapped to PF/VF */
	if (pf_map[rsrc_id] != pcifunc)
		return -EINVAL;

	rvu_free_rsrc(rsrc, rsrc_id);
	pf_map[rsrc_id] = 0;

The pf_map arrays are sized from hardware capability in
mcs_alloc_struct_mem(): flowid2pf_map, secy2pf_map and sc2pf_map are 128
u16 entries on cn10kb (64 on cnf10kb), sa2pf_map is 256 (128 on cnf10kb).
So a rsrc_id above the corresponding max reads up to 254 bytes past the
devm_kcalloc() allocation.

If that out-of-bounds slot happens to contain the caller's pcifunc, does
this then also write out of bounds?  rvu_free_rsrc() does
__clear_bit(rsrc_id, rsrc->bmap) on a bitmap allocated as
BITS_TO_LONGS(max) longs by rvu_alloc_bitmap(), which is 16 bytes for
max 127/128, and pf_map[rsrc_id] = 0 stores past the array.  Adjacent slab
allocations make this plausible: flowid2pf_map and secy2pf_map are both
256 bytes and allocated back-to-back, so flowid2pf_map[128 + i] can read
secy2pf_map[i], which legitimately holds the caller's pcifunc after an
"alloc all" request.

Separately, mcs_ena_dis_flowid_entry(), mcs_clear_secy_plcy() and
mcs_ena_dis_sc_cam_entry() are called with the same unvalidated index
regardless of the mcs_free_rsrc() return code, so the index also reaches
the MMIO offset arithmetic.  Should the handler reject rsrc_id >= max for
the requested type before the switch?

[Severity: Medium]
This is a pre-existing gap rather than something this patch introduces,
but since mcs_free_all_rsrc() is the "free everything owned by pcifunc"
helper being touched: should it also release ctrlpktrule resources?

It frees flow_ids, secy, sc and sa, but there is no map->ctrlpktrule /
ctrlpktrule2pf_map loop, and rvu_mcs_flr_handler() calls only
mcs_free_all_rsrc() for RX and TX.  The only release path is the req->all
branch of mcs_free_ctrlpktrule(), which needs an explicit mailbox request
from the function that just went through FLR.

So a rule allocated via rvu_mbox_handler_mcs_alloc_ctrl_pkt_rule() stays
marked allocated in map->ctrlpktrule and stays enabled in hardware, since
MCSX_PEX_{RX,TX}_SLAVE_RULE_ENABLE is only cleared by
mcs_free_ctrlpktrule().  Recovery appears to require a driver reload.

No in-tree consumer allocates these rules today (nothing under
octeontx2/nic/ references ctrl_pkt_rule, and cn10k_macsec.c only uses
FLOWID/SECY/SC/SA), so this needs an out-of-tree or other user of the AF
mailbox to trigger.