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

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-10-18 at 08:19:26, netdev-bot+sashiko@kernel.org (netdev-bot+sashiko@kernel.org) wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 (1) · High: 1 (3) · Medium: 1 (5) · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] otx2_sync_flags_from_rep() (otx2_common.h:625-634) updates
>   priv->flags with a non-atomic read-modify-write: `new =…
This is not a regression from this patch. The race already existed, and the patch narrows it.

> - [Medium] The commit message presents this patch as a cleanup that
>   switches flag updates to atomic bitops.
the fix to the representor path is a side effect of this patch, not its purpose

>
> Pre-existing issues:
> - [Critical] rvu_rep_get_stats64() calls
>   schedule_delayed_work(&rep->stats_wrk, 100ms) whenever…
> - [High] rvu_rep_setup_tc_cb() ignores the return value of
>   rvu_rep_mcam_flow_init() and then publishes rep->flow_cfg as…
> - [High] rvu_rep_destroy() reads rep->flow_cfg after freeing the memory
>   that holds rep.
> - [High] rvu_rep_mcam_flow_init() builds and sends AF mailbox messages on
>   priv->mbox (otx2_mbox_alloc_msg_npc_mcam_alloc_entry() /…
> - [Medium] The new comment on OTX2_REP_SYNC_FLAGS_MASK says only
>   MCAM_ENTRIES_ALLOC, NTUPLE_SUPPORT, TC_FLOWER_SUPPORT and…
> - [Medium] In otx2_tc_del_flow(), deleting any marked flower rule clears
>   OTX2_FLAG_TC_MARK_ENABLED even if other marked rules are still…
> - [Medium] rvu_rep_mcam_flow_init() zero-allocates rep->flow_cfg with
>   kzalloc_objs() and never sets mark_flows to 1.
> - [Medium] In otx2_config_hwtstamp_set() (otx2_pf.c), a direct switch
>   from HWTSTAMP_TX_ONESTEP_SYNC to HWTSTAMP_TX_ON leaves…
> - [Medium] The representor flow_cfg lifecycle leaks memory in two ways.
>
> --
>
> octeontx2: use atomic bitops for PF/VF and rep flags
>
> This converts the otx2_nic and rep_dev flags from u64 masks to unsigned
> long bitmaps updated with set_bit()/clear_bit()/test_bit(). It also adds
> otx2_sync_flags_from_rep(), which copies a masked set of representor-owned
> bits onto the shared PF context. The representor VF-initialized flag moves
> to OTX2_FLAG_REP_VF_INITIALIZED (bit 21).
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > index 3c7275d545bba..0dc4c44e521d9 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > @@ -482,6 +482,41 @@ struct pf_irq_data {
> >  	int mdevs;
> >  };
> >
> > +enum otx2_flag_bits {
> > +	OTX2_FLAG_RX_TSTAMP_ENABLED,
>
> [ ... ]
>
> > +	OTX2_FLAG_TC_MARK_ENABLED,
> > +	OTX2_FLAG_REP_MODE_ENABLED,
> > +	OTX2_FLAG_PORT_UP,
> > +	OTX2_FLAG_IPSEC_OFFLOAD_ENABLED,
> > +	OTX2_FLAG_REP_VF_INITIALIZED,
> > +	OTX2_FLAG_MAX,
> > +};
> > +
> > +/* Representor-owned flags copied onto the PF mailbox context in
> > + * rvu_rep_setup_tc_cb().  All other bits are owned by the PF/VF netdev.
> > + */
> > +#define OTX2_REP_SYNC_FLAGS_MASK				\
> > +	(BIT(OTX2_FLAG_MCAM_ENTRIES_ALLOC) |			\
> > +	 BIT(OTX2_FLAG_NTUPLE_SUPPORT) |			\
> > +	 BIT(OTX2_FLAG_TC_FLOWER_SUPPORT) |			\
> > +	 BIT(OTX2_FLAG_REP_VF_INITIALIZED))
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but does this comment get the
> owner of OTX2_FLAG_TC_MARK_ENABLED right?
>
> On the representor path, rvu_rep_setup_tc_cb() points priv->flow_cfg at
> rep->flow_cfg. otx2_tc_parse_actions() then does:
>
>     otx2_set_flag(nic, OTX2_FLAG_TC_MARK_ENABLED);
>     refcount_inc(&nic->flow_cfg->mark_flows);
>
> So the bit lives on the shared priv, but the counter is per-rep.
> otx2_tc_del_flow() then clears the shared bit based on one rep's counter.
>
> Say rep A and rep B both have skbedit mark rules and the rule on rep B is
> deleted. Wouldn't otx2_rcv_pkt_handler() then stop setting skb->mark for
> rep A's traffic?
>
>     if (otx2_test_flag(pfvf, OTX2_FLAG_TC_MARK_ENABLED))
>         skb->mark = parse->match_id;
>
> Before this patch, priv->flags = rep->flags wiped the bit on every
> callback, so representor marking was already broken. This patch changes
> how it fails, but the new comment now describes TC_MARK_ENABLED as owned
> by the PF/VF netdev.
>
> [ ... ]
>
> > @@ -593,6 +607,32 @@ struct otx2_nic {
> >  	unsigned long		*af_xdp_zc_qidx;
> >  };
> >
>
> [ ... ]
>
> > +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 read-modify-write lose a concurrent set_bit() or clear_bit() on
> priv->flags?
>
> Every other writer of this word now uses atomic bitops. Any update that
> lands between the READ_ONCE() and the WRITE_ONCE() here is overwritten
> with the stale value.
>
> rvu_rep_setup_tc_cb() runs from the flower block callback under rtnl
> only. rvu_rep_destroy() and rvu_rep_create()->rvu_rep_napi_init() are
> called from otx2_devlink_eswitch_mode_set() holding only the devlink
> instance lock. During that time the rep netdevs and their TC blocks are
> still registered:
>
> CPU1 (tc flower on a rep)
> rvu_rep_setup_tc_cb()
>   otx2_sync_flags_from_rep()
>     new_flags = (READ_ONCE(dst->flags) & ~MASK) | ...
>
> CPU2 (devlink eswitch mode legacy)
> rvu_rep_destroy()
>   otx2_set_flag(priv, OTX2_FLAG_INTF_DOWN);
>
> CPU1
>     WRITE_ONCE(dst->flags, new_flags);   <- INTF_DOWN lost
>
> If INTF_DOWN is lost, otx2_napi_handler() would re-arm the CQ interrupt
> through NIX_LF_CINTX_ENA_W1S during teardown. rvu_rep_remove() would
> later see INTF_DOWN clear and call rvu_rep_destroy() a second time on the
> already freed priv->reps.
>
> The opposite direction looks possible too. The clear of INTF_DOWN in
> rvu_rep_napi_init() could be undone, and the representor datapath would
> never get its interrupts re-enabled.
>
> Would set_mask_bits(&dst->flags, OTX2_REP_SYNC_FLAGS_MASK,
> src & OTX2_REP_SYNC_FLAGS_MASK), or a try_cmpxchg() loop, match what the
> commit message says about atomic bitops and "without clearing PF-owned
> bits"?
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > index 7dc0ce669d0b7..777e7156badb7 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
>
> [ ... ]
>
> > @@ -2538,8 +2538,8 @@ int otx2_config_hwtstamp_set(struct net_device *netdev,
> >
> >  	switch (config->tx_type) {
> >  	case HWTSTAMP_TX_OFF:
> > -		if (pfvf->flags & OTX2_FLAG_PTP_ONESTEP_SYNC)
> > -			pfvf->flags &= ~OTX2_FLAG_PTP_ONESTEP_SYNC;
> > +		if (otx2_test_flag(pfvf, OTX2_FLAG_PTP_ONESTEP_SYNC))
> > +			otx2_clear_flag(pfvf, OTX2_FLAG_PTP_ONESTEP_SYNC);
> >
> >  		cancel_delayed_work(&pfvf->ptp->synctstamp_work);
> >  		otx2_config_hw_tx_tstamp(pfvf, false);
> > @@ -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;
>
> [Severity: Medium]
> This is a pre-existing issue, but what clears OTX2_FLAG_PTP_ONESTEP_SYNC
> when the mode switches straight from HWTSTAMP_TX_ONESTEP_SYNC to
> HWTSTAMP_TX_ON?
>
> Only the HWTSTAMP_TX_OFF case clears it. The HWTSTAMP_TX_ON case only
> does:
>
>     case HWTSTAMP_TX_ON:
>         otx2_config_hw_tx_tstamp(pfvf, true);
>         break;
>
> otx2_set_txtstamp() would keep taking the one-step branch for PTP Sync
> packets. It rewrites the origin timestamp and never sets
> SKBTX_IN_PROGRESS, so the requested two-step mode isn't applied until
> there is an OFF request in between.
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> > index dee38d5c7f65b..8877af348a090 100644
> > --- 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]
> This isn't a bug introduced by this patch, but for representors, is
> mark_flows ever initialized before this refcount_inc()?
>
> rvu_rep_mcam_flow_init() allocates rep->flow_cfg with kzalloc_objs() and
> never sets mark_flows. The only refcount_set(&flow_cfg->mark_flows, 1) is
> in otx2_mcam_entry_init(), which representors don't use.
>
> A rule such as "tc filter add dev <rep> ... action skbedit mark N" would
> then go through:
>
> rvu_rep_setup_tc_cb()->otx2_setup_tc_cls_flower()->otx2_tc_add_flow()
>   ->otx2_tc_prepare_flow()->otx2_tc_parse_actions()
>     refcount_inc(&nic->flow_cfg->mark_flows);   /* from 0 */
>
> That fires the "addition on 0; use-after-free" warning in
> refcount_warn_saturate() and saturates the counter. After that,
> refcount_dec_and_test() in otx2_tc_del_flow() always returns false.
>
> otx2_sync_flags_from_rep() now keeps TC_MARK_ENABLED on priv across
> callbacks, so this broken counter is the only thing that decides when the
> flag gets cleared.
>
> [ ... ]
>
> > @@ -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: Medium]
> This is a pre-existing issue, but is this condition inverted?
>
> otx2_mcam_entry_init() sets mark_flows to 1, and each FLOW_ACTION_MARK
> adds one. With two marked rules the count is 3. Deleting one brings it to
> 2, refcount_dec_and_test() returns false, and the flag is cleared.
>
> Because of the base reference, deleting mark rules never brings the
> counter to 0. Wouldn't every delete of a marked rule clear
> OTX2_FLAG_TC_MARK_ENABLED, so that otx2_rcv_pkt_handler() stops setting
> skb->mark for the rules still installed?
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > index 0f5d5642d3f7a..7df82c22cc12b 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
> rvu_rep_mcam_flow_init() ever take priv->mbox.lock?
>
> It allocates and sends AF messages without the lock:
>
>     while (allocated < count) {
>         req = otx2_mbox_alloc_msg_npc_mcam_alloc_entry(&priv->mbox);
>         ...
>         if (otx2_sync_mbox_msg(&priv->mbox))
>             goto exit;
>
> and then unlocks unconditionally at the exit label:
>
> exit:
>     ...
>     mutex_unlock(&priv->mbox.lock);
>
> Its only caller, rvu_rep_setup_tc_cb(), holds rtnl but not the mbox
> mutex. rvu_rep_get_stats(), rvu_eswitch_config() and the otx2_tc_*
> helpers all serialize on priv->mbox.lock.
>
> Can these MCAM allocation messages interleave with the stats work's
> mailbox traffic and corrupt the shared mbox region?
>
> And if another task holds mbox.lock at that point, doesn't this
> mutex_unlock() release that task's critical section?
>
> [Severity: Medium]
> This is also a pre-existing issue, but do rep->flow_cfg and its flow_ent
> leak here?
>
> rvu_rep_mcam_flow_init() allocates a new flow_cfg and flow_ent on every
> call and never frees the previous ones. It also doesn't free flow_cfg
> when the flow_ent kcalloc() fails:
>
>     rep->flow_cfg = kzalloc_objs(struct otx2_flow_config, 1);
>     ...
>     rep->flow_cfg->flow_ent = kcalloc(count, sizeof(u16), GFP_KERNEL);
>     if (!rep->flow_cfg->flow_ent)
>         return -ENOMEM;
>
> rvu_rep_setup_tc_cb() calls it again on every TC callback while
> TC_FLOWER_SUPPORT stays clear, including FLOW_CLS_STATS from dumps.
> TC_FLOWER_SUPPORT stays clear after a flow_ent allocation failure, a
> mailbox failure, or an MCAM allocation that returns zero entries.
>
> Separately, rvu_rep_destroy() only does kfree(rep->flow_cfg).
> flow_cfg->flow_ent is never freed, and the MCAM entries allocated for the
> rep's pcifunc are never released to the AF.
>
> > @@ -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_FLAG_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);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but what happens when
> rvu_rep_mcam_flow_init() returns -ENOMEM here?
>
> The return value is dropped, and rep->flow_cfg is published below in one
> of two broken states:
>
> - NULL, if kzalloc_objs() failed.
> - With zeroed list heads, if the flow_ent kcalloc() failed, because that
>   path skips INIT_LIST_HEAD().
>
> FLOW_CLS_REPLACE bails out early because TC_FLOWER_SUPPORT is clear.
> FLOW_CLS_DESTROY, however, goes straight to:
>
> otx2_setup_tc_cls_flower()->otx2_tc_del_flow()
>   ->otx2_tc_get_entry_by_cookie()
>       list_for_each_entry(tmp, &flow_cfg->flow_list_tc, list)
>
> Wouldn't that dereference a NULL flow_cfg, or follow a NULL ->next from a
> zeroed list head? tc core issues the destroy when a skip_sw replace fails,
> and when a non-skip_hw filter is deleted.
>
> >
> >  	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: Medium]
> This change looks like it fixes a real bug, but the commit message only
> hints at it:
>
>   ... giving lockless NAPI readers a consistent word without clearing
>   PF-owned bits.
>
>   Relocate representor VF initialization to OTX2_FLAG_REP_VF_INITIALIZED
>   (bit 21).
>
> Before this patch, priv->flags = rep->flags overwrote every PF-owned bit
> on the shared rep PF otx2_nic. rvu_rep_probe() sets
> OTX2_FLAG_REP_MODE_ENABLED on priv, but rep->flags never has it, so any
> flower callback on a representor cleared it.
>
> The old RVU_REP_VF_INITIALIZED was BIT_ULL(0), the same bit as
> OTX2_FLAG_RX_TSTAMP_ENABLED. The callback only proceeds when that bit is
> set, so the copy also set RX_TSTAMP_ENABLED on priv.
>
> With REP_MODE_ENABLED cleared, otx2_tx_napi_handler() uses pfvf->netdev,
> which the callback has just set to the last rep's netdev. It also stops
> forcing qidx to 0:
>
>     if (otx2_test_flag(pfvf, OTX2_FLAG_REP_MODE_ENABLED))
>         qidx = 0;
>     txq = netdev_get_tx_queue(ndev, qidx);
>
> That means netdev_tx_completed_queue() could update a netdev_queue past
> the single TX queue that alloc_etherdev() allocates for the rep.
>
> Should the commit message describe this representor TX/RX breakage and
> carry a Fixes: tag so it gets picked up for stable? Something like:
>
> Fixes: 6c40ca957fe5 ("octeontx2-pf: Adds TC offload support")
>
> [ ... ]
>
> > @@ -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_FLAG_REP_VF_INITIALIZED, &rep->flags))
> >  		return;
> >
> >  	stats->rx_packets = rep->stats.rx_frames;
>
> [Severity: Critical]
> This is a pre-existing issue, but is stats_wrk ever cancelled before the
> rep netdev is freed?
>
> Further down, rvu_rep_get_stats64() does:
>
>     schedule_delayed_work(&rep->stats_wrk, msecs_to_jiffies(100));
>
> stats_wrk is embedded in struct rep_dev, which is netdev_priv(rep->netdev).
> Neither rvu_rep_destroy() nor the rvu_rep_create() error path calls
> cancel_delayed_work_sync() before free_netdev().
>
> unregister_netdev() itself re-arms the work through the RTM_DELLINK
> message:
>
> unregister_netdev()->...->rtnl_fill_ifinfo()->rtnl_fill_stats()
>   ->dev_get_stats()->rvu_rep_get_stats64()
>     schedule_delayed_work(&rep->stats_wrk, ...)
>
> Any stats read does the same, including unprivileged ones such as
> /proc/net/dev or ip -s link.
>
> About 100ms after free_netdev(), wouldn't the timer fire from freed
> memory? rvu_rep_get_stats() would then do container_of() on the freed rep,
> dereference rep->mdev and take priv->mbox.lock.
>
> There is also an ordering question in rvu_rep_create().
> INIT_DELAYED_WORK(&rep->stats_wrk, rvu_rep_get_stats) runs only after
> register_netdev(). Can the RTM_NEWLINK stats fill schedule the work before
> it is initialized?
>
> [ ... ]
>
> > @@ -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 isn't a bug introduced by this patch, but doesn't the rest of this
> loop read rep after it has been freed?
>
>         unregister_netdev(rep->netdev);
>         rvu_rep_devlink_port_unregister(rep);
>         free_netdev(rep->netdev);
>         kfree(rep->flow_cfg);
>
> In rvu_rep_create(), rep comes from netdev_priv(ndev), so it lives inside
> the netdev allocation. Once unregister_netdev() has completed,
> free_netdev() normally drops the last device reference and frees that
> memory.
>
> kfree(rep->flow_cfg) then loads the pointer from freed memory. With slab
> poisoning that means kfree() of a poison value. With init_on_free,
> flow_cfg leaks. KASAN would report the access on every
> rvu_rep_destroy().
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014023431.1775238-1-rkannoth%40marvell.com