Re: [PATCH v18 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 1 potential issue(s) to consider:
- [High] net: octeontx2: non-atomic flags sync clobbers concurrent atomic bitops
--

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

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

This commit switches PF/VF and representor flags from non-atomic masks to
unsigned long bitmaps using atomic bit operations. It also relocates
representor VF initialization bits and introduces a helper to locklessly
sync representor flags onto the PF mailbox context.

> 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

[ ... ]

> @@ -593,6 +607,32 @@ 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_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]
Can this cause lost atomic updates? The patch converts flags to be modified
by atomic bitops like set_bit() and clear_bit(). However, this helper
performs a non-atomic read-modify-write (READ_ONCE followed by WRITE_ONCE)
over the entire dst->flags word.

If a concurrent path atomically sets a flag (such as OTX2_FLAG_INTF_DOWN or
OTX2_FLAG_PORT_UP) right between the READ_ONCE and WRITE_ONCE, the
WRITE_ONCE will overwrite the memory with the stale value, silently erasing
the concurrent atomic bit operation. This is reachable from the TC flower
offload setup path which calls rvu_rep_setup_tc_cb().

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

[ ... ]

> @@ -599,11 +599,11 @@ int otx2_napi_handler(struct napi_struct *napi, int budget)
>  
>  	if (workdone < budget && napi_complete_done(napi, workdone)) {
>  		/* If interface is going down, don't re-enable IRQ */
> -		if (pfvf->flags & OTX2_FLAG_INTF_DOWN)
> +		if (otx2_test_flag(pfvf, OTX2_FLAG_INTF_DOWN))
>  			return workdone;

[Severity: High]
Because the NAPI handler heavily relies on OTX2_FLAG_INTF_DOWN to halt
processing and prevent use-after-free scenarios during teardown, erasing
this flag via the race condition in otx2_sync_flags_from_rep() could result
in otx2_napi_handler() continuously polling freed resources.

> 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

[ ... ]

> @@ -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);

[Severity: High]
If the OTX2_FLAG_INTF_DOWN flag set here is immediately overwritten by the
non-atomic flag sync mentioned above, NAPI polling can continue executing
concurrently while rvu_rep_free_cq_rsrc() tears down the queues, leading to
a use-after-free crash on teardown.

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