Re: [PATCH v15 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:

Pre-existing issues:
- [High] TC offload setup on representors corrupts the shared physical function (PF) device state by locklessly overwriting its netdev and clobbering its flags via a non-atomic bitwise sync loop.
- [High] Inverted logic in `otx2_tc_del_flow()` prematurely disables TC MARK hardware offload when any single mark rule is deleted.
- [High] Missing cancellation of `stats_wrk` leads to a Use-After-Free during representor teardown.
- [High] `rvu_rep_mcam_flow_init()` interacts with the mailbox without acquiring its lock and executes an unbalanced `mutex_unlock()`.
--

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

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

This commit replaces non-atomic u64 flag read-modify-write operations
with unsigned long bitmaps across the octeontx2 driver. It introduces
helpers for atomic bitwise flag updates and representor state sync.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> index eecee612b7b2c..7e09c1444a6d5 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> @@ -594,6 +595,34 @@ struct otx2_nic {
>  	unsigned long		*af_xdp_zc_qidx;
>  };
>  
> +static inline void otx2_set_flag(struct otx2_nic *nic, unsigned int flag)
> +{
> +	set_bit(flag, &nic->flags);
> +}
> +
> +static inline void otx2_clear_flag(struct otx2_nic *nic, unsigned int flag)
> +{
> +	clear_bit(flag, &nic->flags);
> +}
> +
> +static inline bool otx2_test_flag(struct otx2_nic *nic, unsigned int flag)
> +{
> +	return test_bit(flag, &nic->flags);
> +}
> +
> +static inline void otx2_sync_flags_from_rep(struct otx2_nic *dst,
> +					    unsigned long *src_flags)
> +{
> +	unsigned int flag;
> +
> +	for (flag = 0; flag <= OTX2_REP_VF_INITIALIZED; flag++) {
> +		if (test_bit(flag, src_flags))
> +			set_bit(flag, &dst->flags);
> +		else
> +			clear_bit(flag, &dst->flags);
> +	}
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does this loop corrupt
the physical function (PF) state by blindly overwriting its flags?

If a TC rule is configured on a representor, rvu_rep_setup_tc_cb() calls
this sync loop. It iteratively clears all global PF-specific flags (like
OTX2_FLAG_REP_MODE_ENABLED and OTX2_FLAG_INTF_DOWN) that aren't set
on the local representor, breaking datapath routing.

Additionally, is this 22-step bitwise copy susceptible to race conditions
with concurrent interrupt state changes since the updates are not atomic as
a single block?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> index 039fd47ebf522..ddb46b580c3b6 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> @@ -1195,12 +1195,12 @@ static int otx2_tc_del_flow(struct otx2_nic *nic,
>  	/* Disable TC MARK flag if they are no rules with skbedit mark action */
>  	if (flow_node->req.match_id)
>  		if (!refcount_dec_and_test(&flow_cfg->mark_flows))
> -			nic->flags &= ~OTX2_FLAG_TC_MARK_ENABLED;
> +			otx2_clear_flag(nic, OTX2_FLAG_TC_MARK_ENABLED);

[Severity: High]
This is a pre-existing issue, but does this inverted logic prematurely
disable TC MARK hardware offload when any single mark rule is deleted?

Because the base refcount is 1, deleting a rule drops the count to > 0,
causing refcount_dec_and_test() to return false. The logical NOT operator
inverts this to true, triggering the clear flag while other flows are
still active.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> index 0f5d5642d3f7a..8a8c0088fd204 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> @@ -93,9 +93,9 @@ static int rvu_rep_mcam_flow_init(struct rep_dev *rep)
>  	rep->flow_cfg->max_flows = allocated;
>  
>  	if (allocated) {
> -		rep->flags |= OTX2_FLAG_MCAM_ENTRIES_ALLOC;
> -		rep->flags |= OTX2_FLAG_NTUPLE_SUPPORT;
> -		rep->flags |= OTX2_FLAG_TC_FLOWER_SUPPORT;
> +		set_bit(OTX2_FLAG_MCAM_ENTRIES_ALLOC, &rep->flags);
> +		set_bit(OTX2_FLAG_NTUPLE_SUPPORT, &rep->flags);
> +		set_bit(OTX2_FLAG_TC_FLOWER_SUPPORT, &rep->flags);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does this function interact
with the mailbox without acquiring its lock and execute an unbalanced
mutex_unlock?

When called by rvu_rep_setup_tc_cb(), the function enters without acquiring
priv->mbox.lock, issues mailbox allocation commands directly, and
unconditionally unlocks the mutex right before this block on the exit path.
This could trigger a lockdep splat or panic, and risks ring corruption.

> @@ -109,14 +109,14 @@ static int rvu_rep_setup_tc_cb(enum tc_setup_type type,
>  	struct rep_dev *rep = cb_priv;
>  	struct otx2_nic *priv = rep->mdev;
>  
> -	if (!(rep->flags & RVU_REP_VF_INITIALIZED))
> +	if (!test_bit(OTX2_REP_VF_INITIALIZED, &rep->flags))
>  		return -EINVAL;
>  
> -	if (!(rep->flags & OTX2_FLAG_TC_FLOWER_SUPPORT))
> +	if (!test_bit(OTX2_FLAG_TC_FLOWER_SUPPORT, &rep->flags))
>  		rvu_rep_mcam_flow_init(rep);
>  
>  	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: High]
This is a pre-existing issue, but does this locklessly overwrite the shared
PF's netdev with the representor's netdev?

By reassigning priv->netdev to rep->netdev, subsequent TX completions in
otx2_tx_napi_handler() will incorrectly route packets to this single
representor's netdev.

> @@ -382,7 +382,7 @@ static void rvu_rep_get_stats64(struct net_device *dev,
>  {
>  	struct rep_dev *rep = netdev_priv(dev);
>  
> -	if (!(rep->flags & RVU_REP_VF_INITIALIZED))
> +	if (!test_bit(OTX2_REP_VF_INITIALIZED, &rep->flags))
>  		return;
>  
>  	stats->rx_packets = rep->stats.rx_frames;

[ ... ]

> @@ -632,7 +632,7 @@ void rvu_rep_destroy(struct otx2_nic *priv)
>  	int rep_id;
>  
>  	rvu_eswitch_config(priv, false);
> -	priv->flags |= OTX2_FLAG_INTF_DOWN;
> +	otx2_set_flag(priv, OTX2_FLAG_INTF_DOWN);
>  	rvu_rep_free_cq_rsrc(priv);
>  	for (rep_id = 0; rep_id < priv->rep_cnt; rep_id++) {
>  		rep = priv->reps[rep_id];

[Severity: High]
This is a pre-existing issue, but can this lead to a Use-After-Free during
representor teardown?

If a user queries interface statistics (e.g., via ip -s link),
rvu_rep_get_stats64() schedules the delayed work stats_wrk. If the
representor is destroyed via rvu_rep_destroy() before the timer expires,
the struct is unregistered and freed without canceling the pending work.
When the timer fires, the workqueue executes and accesses the freed memory.

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