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

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-11-12 at 03:02:28, netdev-bot+sashiko@kernel.org (netdev-bot+sashiko@kernel.org) wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
>
> Critical: 0 (1) · High: 1 (4) · Medium: 2 (1) · Low: 1

v21 is RESEND of v20

v20 sashiko comments reply : https://lore.kernel.org/netdev/atV60YxlUUmKoOD7@rkannoth-OptiPlex-7090/

>
> 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()
>   (drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h) is the only…
> - [Medium] The commit message hides that this patch fixes a real bug, and
>   it has no Fixes: tag.
> - [Medium] Before this patch, `priv->flags = rep->flags` always cleared
>   OTX2_FLAG_INTF_DOWN on the eswitch PF context before calling into the…
> - [Low] The commit message says the flags now use atomic bitops and that
>   otx2_sync_flags_from_rep() publishes representor flags "without…
>
> Pre-existing issues:
> - [Critical] rvu_rep_state_evt_handler()
>   (drivers/net/ethernet/marvell/octeontx2/nic/rep.c) does `rep_id =…
> - [High] rep->stats_wrk is set up with INIT_DELAYED_WORK in
>   rvu_rep_create().
> - [High] In rvu_rep_destroy()
>   (drivers/net/ethernet/marvell/octeontx2/nic/rep.c),…
> - [High] rvu_rep_mcam_flow_init()
>   (drivers/net/ethernet/marvell/octeontx2/nic/rep.c) builds and sends…
> - [High] Any PF or VF, including a VF owned by a guest since the PF
>   forwards all VF mbox messages to AF, can send REP_EVENT_NOTIFY with…
> - [Medium] rvu_rep_setup_tc_cb() calls rvu_rep_mcam_flow_init(rep) on
>   every tc callback while OTX2_FLAG_TC_FLOWER_SUPPORT is clear in…
>
> --
>
> 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 through set_bit()/clear_bit()/test_bit() helpers.
> In rvu_rep_setup_tc_cb(), the full-word assignment is replaced by
> otx2_sync_flags_from_rep(), which copies a mask of representor-owned bits
> onto the eswitch PF context. The representor VF-initialized flag moves
> from BIT_ULL(0) in rep.h to OTX2_FLAG_REP_VF_INITIALIZED (bit 21).
>
> > Switch PF/VF and representor flags from non-atomic u64 masks to unsigned
> > long bitmaps with set_bit/clear_bit/test_bit.  Add helpers and
> > otx2_sync_flags_from_rep() to publish representor-owned flags onto the PF
> > mailbox context with a masked WRITE_ONCE, giving lockless NAPI readers a
> > consistent word without clearing PF-owned bits.
>
> [Severity: Low]
> Is "without clearing PF-owned bits" accurate? otx2_sync_flags_from_rep()
> does a plain READ_ONCE()/mask/WRITE_ONCE() read-modify-write of
> dst->flags. Every other writer of that word now uses set_bit() or
> clear_bit().
>
> If an atomic update to a PF-owned bit lands between the read and the
> write, it gets overwritten. So the guarantee only holds when nothing else
> is writing at the same time. There are more details on this below, at
> otx2_sync_flags_from_rep().
>
> Could the commit message also say which concurrent update problem the
> atomic conversion is meant to fix?
>
> > Relocate representor VF initialization to OTX2_FLAG_REP_VF_INITIALIZED
> > (bit 21).
>
> [Severity: Medium]
> Is this only a relocation? Before this patch, rvu_rep_setup_tc_cb() did:
>
>     priv->flags = rep->flags;
>
> That overwrote the whole flags word of the eswitch PF otx2_nic, including
> OTX2_FLAG_REP_MODE_ENABLED, which rvu_rep_probe() sets.
>
> The removed RVU_REP_VF_INITIALIZED was also BIT_ULL(0), the same bit as
> OTX2_FLAG_RX_TSTAMP_ENABLED. The callback only gets past its first check
> when that bit is set, so the bit was copied onto priv every time.
>
> Once REP_MODE_ENABLED was cleared, otx2_tx_napi_handler() stopped taking
> these branches:
>
>     if (otx2_test_flag(pfvf, OTX2_FLAG_REP_MODE_ENABLED))
>         ndev = pfvf->reps[qidx]->netdev;
>     ...
>     if (otx2_test_flag(pfvf, OTX2_FLAG_REP_MODE_ENABLED))
>         qidx = 0;
>     txq = netdev_get_tx_queue(ndev, qidx);
>
> Instead it fell back to pfvf->netdev, which rvu_rep_setup_tc_cb() had
> pointed at rep->netdev. That netdev comes from alloc_etherdev() and has a
> single TX queue, and qidx was no longer clamped. Doesn't that index past
> dev->_tx[] and then write DQL state beyond it?
>
> If so, this patch includes a fix. Would a Fixes: tag and a description of
> the old behaviour help with stable backports?
>
> The mask also changes one more behaviour: OTX2_FLAG_TC_MARK_ENABLED now
> stays set in priv instead of being cleared on every representor tc
> callback. Is that intended, and is it worth mentioning in the commit
> message?
>
> > 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_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 lose concurrent updates to PF-owned bits in priv->flags?
>
> The only caller is rvu_rep_setup_tc_cb(), which runs from the tc block
> callback under rtnl. Other writers of the same word don't hold rtnl. One
> example is the devlink eswitch path:
>
> otx2_devlink_eswitch_mode_set()
>     case DEVLINK_ESWITCH_MODE_LEGACY:
>         rvu_rep_destroy(pfvf);
>             otx2_set_flag(priv, OTX2_FLAG_INTF_DOWN);
>
> This runs under the devlink lock only; unregister_netdev() takes rtnl
> later. rvu_rep_napi_init() clears INTF_DOWN on the same devlink path, and
> the mbox up-handler work sets and clears OTX2_FLAG_PORT_UP.
>
> If one of those set_bit()/clear_bit() calls lands between the READ_ONCE()
> and the WRITE_ONCE() here, the stale word gets written back.
>
> If the INTF_DOWN set is lost, the eswitch PF still looks up after
> teardown. otx2_napi_handler() can then re-arm the CINT, and
> rvu_rep_remove() runs destroy a second time:
>
>     if (!otx2_test_flag(priv, OTX2_FLAG_INTF_DOWN))
>         rvu_rep_destroy(priv);
>
> That second destroy calls unregister_netdev()/free_netdev() on netdevs
> that are already freed, and calls kfree(priv->reps) a second time.
>
> If the INTF_DOWN clear in rvu_rep_napi_init() is lost instead, the CQ
> interrupts are never re-enabled, because otx2_napi_handler() returns
> early:
>
>     if (otx2_test_flag(pfvf, OTX2_FLAG_INTF_DOWN))
>         return workdone;
>
> Would updating only the masked bits with assign_bit(), or using a cmpxchg
> loop, avoid this?
>
> [ ... ]
>
> > 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
>
> [ ... ]
>
> > @@ -907,11 +907,11 @@ static int otx2_mbox_up_handler_rep_event_up_notify(struct otx2_nic *pf,
> >
> >  	if (info->event == RVU_EVENT_PORT_STATE) {
> >  		if (info->evt_data.port_state) {
> > -			pf->flags |= OTX2_FLAG_PORT_UP;
> > +			otx2_set_flag(pf, OTX2_FLAG_PORT_UP);
> >  			netif_carrier_on(netdev);
> >  			netif_tx_start_all_queues(netdev);
> >  		} else {
> > -			pf->flags &= ~OTX2_FLAG_PORT_UP;
> > +			otx2_clear_flag(pf, OTX2_FLAG_PORT_UP);
> >  			netif_tx_stop_all_queues(netdev);
> >  			netif_carrier_off(netdev);
> >  		}
>
> [Severity: High]
> This isn't a bug introduced by this patch, but can any PF or VF forge
> this event for a PF it doesn't own?
>
> On the AF side, rvu_mbox_handler_rep_event_notify() only range-checks the
> target:
>
>     if (!is_pf_func_valid(rvu, req->pcifunc))
>         return -EINVAL;
>
> It doesn't check that the sender is rvu->rep_pcifunc.
> rvu_rep_up_notify() then forwards the event and evt_data to
> rvu_get_pf(event->pcifunc). The PF forwards VF mailbox messages to AF
> through otx2_forward_vf_mbox_msgs(), so a VF owned by a guest can send
> this as well.
>
> On the victim PF, port_state=0 stops TX and drops carrier. port_state=1
> sets OTX2_FLAG_PORT_UP, and from then on otx2_handle_link_event() ignores
> real CGX link events until the next otx2_open():
>
>     if (otx2_test_flag(pf, OTX2_FLAG_PORT_UP))
>         return;
>
> Should AF reject REP_EVENT_NOTIFY from anything other than the
> representor PF?
>
> [ ... ]
>
> > 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 is a pre-existing issue, but where does rvu_rep_mcam_flow_init()
> take priv->mbox.lock? The loop allocates and sends NPC_MCAM_ALLOC_ENTRY
> messages on priv->mbox without the lock, and the exit path then does:
>
> exit:
>     ...
>     mutex_unlock(&priv->mbox.lock);
>
> Neither this function nor rvu_rep_setup_tc_cb() calls mutex_lock().
>
> Suppose another context holds the mutex at that point, such as the
> rvu_rep_get_stats() worker. Doesn't this release that holder's lock and
> let mailbox messages from both contexts interleave? If nobody holds it,
> this is an unbalanced unlock, which lockdep would report.
>
> > @@ -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: Medium]
> This isn't a bug introduced by this patch, but does this leak
> rep->flow_cfg and rep->flow_cfg->flow_ent?
>
> rvu_rep_mcam_flow_init() always assigns new kzalloc()/kcalloc() results
> to rep->flow_cfg and rep->flow_cfg->flow_ent. It only sets
> OTX2_FLAG_TC_FLOWER_SUPPORT when allocated > 0.
>
> So if AF returns no entries, a mailbox call fails, or the flow_ent
> allocation fails, the next tc callback calls it again and the previous
> allocations are lost. The return value is also ignored here.
>
> Separately, rvu_rep_destroy() only does kfree(rep->flow_cfg), so flow_ent
> is never freed on teardown.
>
> >
> >  	priv->netdev = rep->netdev;
> > -	priv->flags = rep->flags;
> > +	otx2_sync_flags_from_rep(priv, &rep->flags);
>
> [Severity: Medium]
> Does this change how otx2_tc_del_flow() handles representor rules during
> teardown?
>
> The old full-word assignment always cleared OTX2_FLAG_INTF_DOWN on priv,
> because rep->flags never carried that bit. With the mask, priv keeps its
> real INTF_DOWN value.
>
> rvu_rep_destroy() sets INTF_DOWN before unregister_netdev(rep->netdev).
> Unregistering sends FLOW_CLS_DESTROY through this callback, and for
> police rules otx2_tc_del_flow() then skips the policer and mcast group
> cleanup:
>
>     if (otx2_test_flag(nic, OTX2_FLAG_INTF_DOWN))
>         goto free_mcam_flow;
>
> otx2_free_hw_resources() skips cn10k_free_all_ipolicers() for
> otx2_rep_dev() devices, and AF's nix_lf_free handler doesn't free
> bandwidth profiles. Can each switch from switchdev to legacy mode leak
> ingress policer leaf profiles?
>
> There is a similar change in rvu_rep_create(). Between register_netdev()
> and rvu_rep_napi_init(), otx2_tc_add_flow() now rejects representor
> offload with "Interface not initialized". Is that intended?
>
> [ ... ]
>
> > @@ -303,9 +303,9 @@ static void rvu_rep_state_evt_handler(struct otx2_nic *priv,
> >  	rep_id = rvu_rep_get_repid(priv, info->pcifunc);
> >  	rep = priv->reps[rep_id];
> >  	if (info->evt_data.vf_state)
> > -		rep->flags |= RVU_REP_VF_INITIALIZED;
> > +		set_bit(OTX2_FLAG_REP_VF_INITIALIZED, &rep->flags);
> >  	else
> > -		rep->flags &= ~RVU_REP_VF_INITIALIZED;
> > +		clear_bit(OTX2_FLAG_REP_VF_INITIALIZED, &rep->flags);
> >  }
>
> [Severity: Critical]
> This is a pre-existing issue, but what happens here when
> rvu_rep_get_repid() doesn't find info->pcifunc? It returns -EINVAL:
>
>     for (rep_id = 0; rep_id < priv->rep_cnt; rep_id++)
>         if (priv->rep_pf_map[rep_id] == pcifunc)
>             return rep_id;
>     return -EINVAL;
>
> In that case priv->reps[-22] is read from memory before the array, and
> set_bit() or clear_bit() writes through that pointer. priv->reps can also
> be NULL before switchdev mode is enabled. It can also be freed memory,
> because rvu_rep_destroy() calls kfree() on it without setting it to NULL.
>
> Can a VF reach this? AF's rvu_mbox_handler_rep_event_notify() only checks
> is_pf_func_valid(). rvu_rep_up_notify() forwards the sender's event bits
> and only sets the header pcifunc:
>
>     msg->hdr.pcifunc = event->pcifunc;
>     msg->event = event->event;
>
> So the body pcifunc arrives as 0. A REP_EVENT_NOTIFY with
> event=RVU_EVENT_PFVF_STATE aimed at the representor PF would reach this
> handler through
> otx2_mbox_up_handler_rep_event_up_notify()->rvu_event_up_notify(), and
> the lookup would miss.
>
> Should rep_id be checked here, and should AF validate the sender?
>
> [ ... ]
>
> > @@ -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: High]
> This isn't a bug introduced by this patch, but is rep->stats_wrk ever
> cancelled? rvu_rep_get_stats64() ends with:
>
>     schedule_delayed_work(&rep->stats_wrk, msecs_to_jiffies(100));
>
> The only cancel_delayed_work_sync() in rep.c is for priv->refill_wrk.
>
> rvu_rep_destroy() and the rvu_rep_create() error unwind both call
> free_netdev(rep->netdev). That frees the rep_dev (netdev_priv) while the
> delayed work embedded in it may still be pending. unregister_netdev()
> itself sends link notifications that call ndo_get_stats64, so the work is
> likely armed during teardown.
>
> Wouldn't rvu_rep_get_stats() then run about 100ms later on freed memory?
>
>     rep = container_of(del_work, struct rep_dev, stats_wrk);
>     priv = rep->mdev;
>
> [ ... ]
>
> > @@ -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 the rest of this loop does:
>
>         unregister_netdev(rep->netdev);
>         rvu_rep_devlink_port_unregister(rep);
>         free_netdev(rep->netdev);
>         kfree(rep->flow_cfg);
>
> rep comes from netdev_priv() of a netdev that rvu_rep_create() allocated
> with alloc_etherdev(sizeof(*rep)). Isn't rep->flow_cfg read after
> free_netdev() has already freed that memory?
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109033154.2161803-1-rkannoth%40marvell.com