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

[netdev-bot+sashiko@kernel.org]

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

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