Re: [PATCH v21 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 to consider:
- [High] net: octeontx2: non-atomic RMW in otx2_sync_flags_from_rep()
  clobbers flags
--

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

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

This commit converts the PF/VF and representor flags from u64 bitmasks to
unsigned long bitmaps manipulated with atomic bitops, and adds
otx2_sync_flags_from_rep() to copy representor flags onto the PF context.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> index 8417af94a02a0..f3158e293d864 100644
> --- 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_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 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 the non-atomic read-modify-write in otx2_sync_flags_from_rep() clobber
concurrent atomic updates to dst->flags?

All representor netdevs share a single struct otx2_nic *priv (rep->mdev).
rvu_rep_setup_tc_cb() calls otx2_sync_flags_from_rep(priv, &rep->flags)
without holding the devlink lock, while otx2_devlink_eswitch_mode_set() ->
rvu_rep_destroy() runs under the devlink lock without rtnl_lock and sets
OTX2_FLAG_INTF_DOWN before unregistering the representor netdevs:

CPU0                                CPU1
rvu_rep_setup_tc_cb()               otx2_devlink_eswitch_mode_set()
  otx2_sync_flags_from_rep()          rvu_rep_destroy()
    READ_ONCE(dst->flags)
    /* OTX2_FLAG_INTF_DOWN is 0 */
                                        otx2_set_flag(priv,
                                          OTX2_FLAG_INTF_DOWN)
    WRITE_ONCE(dst->flags, new_flags)
    /* clears OTX2_FLAG_INTF_DOWN */
                                        ...
                                    rvu_rep_remove()
                                      if (!otx2_test_flag(priv,
                                            OTX2_FLAG_INTF_DOWN))
                                        rvu_rep_destroy(priv)
                                          /* double teardown / free */

Because OTX2_FLAG_INTF_DOWN is overwritten back to 0, otx2_napi_handler()
can re-enable CQ interrupts during teardown, and rvu_rep_remove() will call
rvu_rep_destroy() a second time, causing a NULL pointer dereference on
qset->napi and a double free of priv->reps and the representor netdevs.

Similarly, because cls_fl_ops sets TCF_PROTO_OPS_DOIT_UNLOCKED, concurrent
TC flower operations on sibling representors can lose updates to
OTX2_FLAG_TC_MARK_ENABLED on priv->flags.

Using a try_cmpxchg() loop (or atomic_long_andnot() / atomic_long_or()) to
update the masked bits in dst->flags would preserve concurrent bit updates.

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