[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.