Re: [PATCH net] nfp: flower: hold an RCU read lock while using an offloaded MAC entry

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] The commit message gets the pre-patch state wrong and leaves
  out half of the fix.
- [Medium] nfp: flower: RMW race on mac_entry->index in
  nfp_flower_xmit_pre_tun_flow()

--

nfp: flower: hold an RCU read lock while using an offloaded MAC entry

This commit takes rcu_read_lock() around the offloaded MAC lookup in
nfp_flower_xmit_pre_tun_flow() and around the update of mac_entry->index.
The index is copied into a local before nfp_flower_xmit_tun_conf() is
called. The patch also adds an rcu_head to struct nfp_tun_offloaded_mac
and converts the kfree(entry) calls in nfp_tunnel_add_shared_mac() and
nfp_tunnel_del_shared_mac() to kfree_rcu().

> so kfree_rcu() alone is not enough: a grace period only protects readers
> still inside an RCU read-side critical section. The writer is the netdev
> notifier chain under rtnl, freeing in nfp_tunnel_del_shared_mac() with
> rhashtable_remove_fast() + kfree_rcu(); the reader is the tc flower
> offload path, which holds no rtnl, only priv->nfp_fl_lock.

[ ... ]

> the unlock: the firmware payload is unchanged and kfree_rcu() is kept.

[Severity: Medium]
Is this an accurate description of the code before the patch?

Before this patch, struct nfp_tun_offloaded_mac had no rcu_head, and
nfp_tunnel_del_shared_mac() freed the entry with a plain kfree(). This
patch adds both the field and the conversion:

    struct nfp_tun_offloaded_mac {
    ...
    	int bridge_count;
    +	struct rcu_head rcu;
    };

and in nfp_tunnel_del_shared_mac():

    -	kfree(entry);
    +	kfree_rcu(entry, rcu);

So kfree_rcu() is not "kept". It is introduced here.

The patch also converts the err_free_entry path in
nfp_tunnel_add_shared_mac(). That path is reached from err_remove_hash
after the entry has already been inserted into offloaded_macs, but the
commit message does not mention it:

    err_remove_hash:
    	rhashtable_remove_fast(&priv->tun.offloaded_macs, &entry->ht_node,
    			       offloaded_macs_params);
    err_free_entry:
    -	kfree(entry);
    +	kfree_rcu(entry, rcu);

The patch carries Cc: stable. Could the commit message, and maybe the
subject, say that the original bug was an immediate kfree() with no grace
period? It would also help to say that the fix adds the rcu_head and
converts both kfree() calls, not just the reader-side lock. Otherwise a
backport could take only the rcu_read_lock()/rcu_read_unlock() part and
keep the immediate kfree().

[ ... ]

> diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> --- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> @@ -1462,7 +1473,10 @@ int nfp_flower_xmit_pre_tun_flow(struct nfp_app *app,
>  	else
>  		mac_entry->index &= ~NFP_TUN_PRE_TUN_IPV6_BIT;
>  
> -	payload.port_idx = cpu_to_be16(mac_entry->index);
> +	mac_idx = mac_entry->index;
> +	rcu_read_unlock();
> +
> +	payload.port_idx = cpu_to_be16(mac_idx);
>  
>  	/* Copy mac id and vlan to flow - dev may not exist at delete time. */
>  	flow->pre_tun_rule.vlan_tci = payload.vlan_tci;

[Severity: Medium]
The commit message says the read-modify-write of mac_entry->index is
now inside the RCU read-side section, but rcu_read_lock() only keeps
the entry from being freed. It does not serialize setting or clearing
NFP_TUN_PRE_TUN_IPV6_BIT here against the writers in
nfp_tunnel_add_shared_mac() and nfp_tunnel_del_shared_mac(). Those
assign entry->index from the netdev notifier under rtnl, while this
path holds only priv->nfp_fl_lock. Either side's update to
entry->index can be lost.

Writing the IPv6 bit back into the shared entry also seems to corrupt
the index those writers rely on. NFP_TUN_PRE_TUN_IPV6_BIT is BIT(7),
and nfp_tunnel_is_mac_idx_global() compares the whole low byte of the
index against NFP_FLOWER_CMSG_PORT_TYPE_OTHER_PORT. After an IPv6
pre-tunnel rule has been offloaded, the low byte of entry->index has
0x80 set.

Later, nfp_tunnel_del_shared_mac() drops the last reference. It only
masks off NFP_TUN_PRE_TUN_IDX_BIT before calling
nfp_tunnel_is_mac_idx_global(), so the check fails. The ida_free() on
priv->tun.mac_off_ids is then skipped, which leaks the ID. The stale
bit is also carried into the index that the bridge_count revert path
sends through __nfp_tunnel_offload_mac().

This behaviour predates the patch. Since mac_idx is now a local copy
anyway, would it make sense to read mac_entry->index once with
READ_ONCE() under rcu_read_lock() and apply NFP_TUN_PRE_TUN_IPV6_BIT
only to mac_idx? flow->pre_tun_rule.port_idx is taken from
payload.port_idx, so the delete path should still see the IPv6 bit.
Or is there a reason the bit has to be stored in the shared entry?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108105605.1668-1-getshell%40seu.edu.cn