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

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

[Hongyan Xu]

nfp_flower_xmit_pre_tun_flow() keeps using the entry the lookup returned.

nfp_tunnel_lookup_offloaded_macs() wraps rhashtable_lookup_fast(), which
takes an RCU read lock and drops it again before returning the pointer
(include/linux/rhashtable.h:708-719), as its kerneldoc (:703-704) warns,
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.

Take rcu_read_lock() before the lookup and rcu_read_unlock() after the
last use of mac_entry, so only the lookup and the read-modify-write of
mac_entry->index are inside the section, with the index copied into a
local variable and nfp_flower_xmit_tun_conf() (GFP_KERNEL) called after
the unlock: the firmware payload is unchanged and kfree_rcu() is kept.

The other two callers run on the notifier chain under rtnl, unchanged.

Fixes: 14690995c141 ("nfp: flower: avoid rmmod nfp crash issues")
Cc: stable@vger.kernel.org
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
 .../netronome/nfp/flower/tunnel_conf.c        | 22 +++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
index 0cef0e2b85d0..e1cd75f0af3c 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
@@ -188,6 +188,7 @@ enum nfp_flower_mac_offload_cmd {
  * @ref_count:		Number of devs using this MAC address
  * @repr_list:		List of reprs sharing this MAC address
  * @bridge_count:	Number of bridge/internal devs with MAC
+ * @rcu:		RCU head used when freeing the entry
  */
 struct nfp_tun_offloaded_mac {
 	struct rhash_head ht_node;
@@ -196,6 +197,7 @@ struct nfp_tun_offloaded_mac {
 	int ref_count;
 	struct list_head repr_list;
 	int bridge_count;
+	struct rcu_head rcu;
 };
 
 static const struct rhashtable_params offloaded_macs_params = {
@@ -1152,7 +1154,7 @@ nfp_tunnel_add_shared_mac(struct nfp_app *app, struct net_device *netdev,
 	rhashtable_remove_fast(&priv->tun.offloaded_macs, &entry->ht_node,
 			       offloaded_macs_params);
 err_free_entry:
-	kfree(entry);
+	kfree_rcu(entry, rcu);
 err_free_ida:
 	if (ida_idx != -1)
 		ida_free(&priv->tun.mac_off_ids, ida_idx);
@@ -1241,7 +1243,7 @@ nfp_tunnel_del_shared_mac(struct nfp_app *app, struct net_device *netdev,
 		ida_free(&priv->tun.mac_off_ids, ida_idx);
 	}
 
-	kfree(entry);
+	kfree_rcu(entry, rcu);
 
 	return __nfp_tunnel_offload_mac(app, mac, 0, true);
 }
@@ -1433,6 +1435,7 @@ int nfp_flower_xmit_pre_tun_flow(struct nfp_app *app,
 	struct nfp_flower_meta_tci *key_meta;
 	struct nfp_tun_pre_tun_rule payload;
 	struct net_device *internal_dev;
+	u16 mac_idx;
 	int err;
 
 	if (app_priv->pre_tun_rule_cnt == NFP_TUN_PRE_TUN_RULE_LIMIT)
@@ -1447,11 +1450,19 @@ int nfp_flower_xmit_pre_tun_flow(struct nfp_app *app,
 	/* Lookup MAC index for the pre-tunnel rule egress device.
 	 * Note that because the device is always an internal port, it will
 	 * have a constant global index so does not need to be tracked.
+	 *
+	 * rhashtable_lookup_fast() releases its own RCU read lock before it
+	 * returns, so the entry has to be kept alive by a critical section of
+	 * our own. nfp_flower_xmit_tun_conf() allocates an skb with GFP_KERNEL
+	 * and must stay outside of it.
 	 */
+	rcu_read_lock();
 	mac_entry = nfp_tunnel_lookup_offloaded_macs(app,
 						     internal_dev->dev_addr);
-	if (!mac_entry)
+	if (!mac_entry) {
+		rcu_read_unlock();
 		return -ENOENT;
+	}
 
 	/* Set/clear IPV6 bit. cpu_to_be16() swap will lead to MSB being
 	 * set/clear for port_idx.
@@ -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;
-- 
2.50.1.windows.1

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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