Re: [PATCH v20 net-next 1/2] octeontx2: use atomic bitops for PF/VF and rep flags

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] octeontx2: panic and cross-interface resource corruption from rvu_rep_setup_tc_cb
- [High] net: octeontx2: non-atomic RMW in otx2_sync_flags_from_rep() clobbers concurrent flags updates
- [Medium] net/octeontx2: leak of TC_MARK_ENABLED flag and mark_flows refcount on error
- [Medium] net/octeontx2: missing return value check in otx2_config_hwtstamp_set
--

commit add59cc125681c120e908ba8a2b3ddcdb3d109fd
Author: Ratheesh Kannoth <rkannoth@marvell.com>

octeontx2: use atomic bitops for PF/VF and rep flags

This commit converts PF/VF and representor flags from non-atomic u64 masks
to unsigned long bitmaps using atomic set_bit/clear_bit operations. It also
introduces helpers to synchronize representor-owned flags onto the PF
mailbox context safely.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h

[ ... ]

> +static inline void otx2_sync_flags_from_rep(struct otx2_nic *dst,
> +					    unsigned long *src_flags)
> +{
> +	unsigned long src = READ_ONCE(*src_flags);
> +	unsigned long new_flags;
> +
> +	new_flags = (READ_ONCE(dst->flags) & ~OTX2_REP_SYNC_FLAGS_MASK) |
> +		    (src & OTX2_REP_SYNC_FLAGS_MASK);
> +	WRITE_ONCE(dst->flags, new_flags);
> +}

[Severity: High]
Does this non-atomic read-modify-write sequence clobber concurrent updates?

While this patch transitions to atomic bitops like set_bit and clear_bit, this
helper uses a non-atomic READ_ONCE, bitwise mask, and WRITE_ONCE sequence. If
an asynchronous handler (like otx2_mbox_up_handler_rep_event_up_notify)
modifies a flag such as OTX2_FLAG_PORT_UP concurrently using otx2_set_flag,
the WRITE_ONCE here will blindly overwrite the memory word.

This can silently discard the concurrent atomic update and lose critical state
transitions.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c

[ ... ]

>  	priv->netdev = rep->netdev;
> -	priv->flags = rep->flags;
> +	otx2_sync_flags_from_rep(priv, &rep->flags);
>  	priv->pcifunc = rep->pcifunc;
>  	priv->flow_cfg = rep->flow_cfg;

[Severity: Critical]
Does this assignment permanently corrupt the master device's hardware and
software states?

By overwriting priv->netdev, priv->pcifunc, and priv->flow_cfg with the
representor's values to trick the TC offload routines, this creates a major
data race and cross-interface resource corruption.

If an RX or TX error occurs before any TC rules are configured, routines like
otx2_check_rcv_errors will dereference a NULL pfvf->netdev, causing an
immediate kernel panic. Furthermore, during hardware teardown,
otx2_free_hw_resources will send MBOX commands using the corrupted pcifunc,
freeing the representor's NIX LF instead of its own and leaking resources.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c

[ ... ]

> @@ -505,7 +505,7 @@ static int otx2_tc_parse_actions(struct otx2_nic *nic,
>  			mark = act->mark;
>  			req->match_id = mark & OTX2_RX_MATCH_ID_MASK;
>  			req->op = NIX_RX_ACTION_DEFAULT;
> -			nic->flags |= OTX2_FLAG_TC_MARK_ENABLED;
> +			otx2_set_flag(nic, OTX2_FLAG_TC_MARK_ENABLED);
>  			refcount_inc(&nic->flow_cfg->mark_flows);
>  			break;

[Severity: Medium]
If the flow parsing or addition fails later in the function, does this leak
the TC_MARK_ENABLED flag and the mark_flows refcount?

The flag and refcount are updated immediately during parsing. If a subsequent
action validation fails, or if otx2_tc_add_flow fails later on, the error
cleanup paths (like the free_leaf label) do not appear to revert the mark
flag or decrement the refcount.

This leaves the fast path unconditionally executing skb->mark = parse->match_id
for all packets even if the rule was never successfully installed.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c

[ ... ]

> @@ -2550,7 +2550,7 @@ int otx2_config_hwtstamp_set(struct net_device *netdev,
>  					   "One-step time stamping is not supported");
>  			return -ERANGE;
>  		}
> -		pfvf->flags |= OTX2_FLAG_PTP_ONESTEP_SYNC;
> +		otx2_set_flag(pfvf, OTX2_FLAG_PTP_ONESTEP_SYNC);
>  		schedule_delayed_work(&pfvf->ptp->synctstamp_work,
>  				      msecs_to_jiffies(500));
>  		fallthrough;
>  	case HWTSTAMP_TX_ON:
>  		otx2_config_hw_tx_tstamp(pfvf, true);
>  		break;

[Severity: Medium]
Should the return values of otx2_config_hw_tx_tstamp and
otx2_config_hw_rx_tstamp be checked here?

These hardware configuration functions can fail due to memory allocation issues
or mailbox timeouts. By ignoring their return values, the function can return
success to userspace and update the software state (pfvf->tstamp), leaving
userspace silently failing to receive hardware timestamps because the software
and hardware states are mismatched.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014023431.1775238-1-rkannoth@marvell.com?part=1