Skip to contentSkip to conversation

net: bcmgenet: restore the hardware filters on open

PATCH Applied Nicolai Buchwitz 5 participants 5 messages Sep 13 – Sep 15

Subsystems (3)
Networking Network drivers Broadcom genet Ethernet driver
lore
Share & tools
lore

Nicolai Buchwitz

author maintainer
#0 lore raw

PATCH 1/1 net: bcmgenet: restore the hardware filters on open

bcmgenet_hfb_init() runs INIT_LIST_HEAD() on priv->rxnfc_list, which drops every rule off the list, and bcmgenet_open() calls it on each ifup. Every rule the user configured is silently lost:

  # ethtool -N eth0 flow-type ether dst $MAC action 0
  Added rule with ID 0
  # ethtool -n eth0 | grep -c Filter:
  1
  # ip link set eth0 down && ip link set eth0 up
  # ethtool -n eth0 | grep -c Filter:
  0

Initialise the lists once at probe and restore the rules on open, as bcmgenet_resume() already does.

Fixes 3e370952287c ("net: bcmgenet: add support for ethtool rxnfc flows")
Signed-off-by Nicolai Buchwitz <[email protected]>
---
 .../net/ethernet/broadcom/genet/bcmgenet.c    | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +13 -6
index a2305e6428d1..b916080f4ff1 100644
@@ -749,8 +749,17 @@ static void bcmgenet_hfb_init(struct bcmgenet_priv *priv)
 		INIT_LIST_HEAD(&priv->rxnfc_rules[i].list);
 		priv->rxnfc_rules[i].state = BCMGENET_RXNFC_STATE_UNUSED;
 	}
+}
+
+static void bcmgenet_hfb_restore(struct bcmgenet_priv *priv)
+{
+	struct bcmgenet_rxnfc_rule *rule;
 
 	bcmgenet_hfb_clear(priv);
+
+	list_for_each_entry(rule, &priv->rxnfc_list, list)
+		if (rule->state != BCMGENET_RXNFC_STATE_UNUSED)
+			bcmgenet_hfb_create_rxnfc_filter(priv, rule);
 }
 
 static int bcmgenet_begin(struct net_device *dev)

@@ -3376,8 +3385,8 @@ static int bcmgenet_open(struct net_device *dev)
 
 	bcmgenet_set_hw_addr(priv, dev->dev_addr);
 
-	/* HFB init */
-	bcmgenet_hfb_init(priv);
+	/* Restore the filters, the MAC was reset above */
+	bcmgenet_hfb_restore(priv);
 
 	/* Reinitialize TDMA and RDMA and SW housekeeping */
 	ret = bcmgenet_init_dma(priv, true);

@@ -4075,6 +4084,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	/* Mii wait queue */
 	init_waitqueue_head(&priv->wq);
+	bcmgenet_hfb_init(priv);
 	INIT_WORK(&priv->bcmgenet_irq_work, bcmgenet_irq_task);
 
 	priv->clk_wol = devm_clk_get_optional(&priv->pdev->dev, "enet-wol");

@@ -4272,10 +4282,7 @@ static int bcmgenet_resume(struct device *d)
 	bcmgenet_set_hw_addr(priv, dev->dev_addr);
 
 	/* Restore hardware filters */
-	bcmgenet_hfb_clear(priv);
-	list_for_each_entry(rule, &priv->rxnfc_list, list)
-		if (rule->state != BCMGENET_RXNFC_STATE_UNUSED)
-			bcmgenet_hfb_create_rxnfc_filter(priv, rule);
+	bcmgenet_hfb_restore(priv);
 
 	/* Reinitialize TDMA and RDMA and SW housekeeping */
 	ret = bcmgenet_init_dma(priv, false);
Signature · 1 line
2.53.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The commit message of "net: bcmgenet: restore the hardware filters on open" describes only the loss of user-configured rxnfc…

--

net: bcmgenet: restore the hardware filters on open

The patch splits bcmgenet_hfb_init() into a list-initialisation part that now runs once from bcmgenet_probe(), and a new bcmgenet_hfb_restore() that clears the hardware filter block and re-programs the configured rxnfc rules. bcmgenet_open() and bcmgenet_resume() both call the restore helper.

Quoted 4 lines
drivers/net/ethernet/broadcom/genet/bcmgenet.c +0 -0
index a2305e6428d1f..b916080f4ff17 100644

[ ... ]

drivers/net/ethernet/broadcom/genet/bcmgenet.c static int bcmgenet_probe(struct platform_device *pdev) · line 4088

Quoted 6 lines
drivers/net/ethernet/broadcom/genet/bcmgenet.c +1 -0
@@ -4075,6 +4084,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	/* Mii wait queue */
 	init_waitqueue_head(&priv->wq);
+	bcmgenet_hfb_init(priv);
 	INIT_WORK(&priv->bcmgenet_irq_work, bcmgenet_irq_task);

[Severity: Medium]
Does this hunk also fix a NULL pointer dereference that the changelog does not mention?

Before this change, priv->rxnfc_list was only initialised from bcmgenet_open(), and alloc_etherdev_mqs() hands back zeroed private data, so between register_netdev() and the first ifup the list head is {NULL, NULL}.

The ethtool rxnfc getters have no netif_running() gate; bcmgenet_begin() only enables the clock:

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_begin() {
	struct bcmgenet_priv *priv = netdev_priv(dev);

	/* Turn on the clock */
	return clk_prepare_enable(priv->clk);
}

so ETHTOOL_GRXCLSRLCNT reaches bcmgenet_get_rxnfc() -> bcmgenet_get_num_flows():

drivers/net/ethernet/broadcom/genet/bcmgenet.c:bcmgenet_get_num_flows() {
	struct list_head *pos;
	int res = 0;

	list_for_each(pos, &priv->rxnfc_list)
		res++;
	...
}

with pos = head->next == NULL, the list_is_head() test does not match the head, and pos->next is then dereferenced.

ETHTOOL_GRXCLSRLCNT, ETHTOOL_GRXCLSRULE and ETHTOOL_GRXCLSRLALL are exempted from the ns_capable(CAP_NET_ADMIN) check in dev_ethtool():

net/ethtool/ioctl.c:dev_ethtool() {
	case ETHTOOL_GRXCLSRLCNT:
	case ETHTOOL_GRXCLSRULE:
	case ETHTOOL_GRXCLSRLALL:
		break;
	default:
		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
			return -EPERM;
	...
}

so an unprivileged local user running "ethtool -n eth0" on a bcmgenet interface that was never brought up could hit this.

The changelog only describes the lost rules:

Every rule the user configured is silently lost

Initialise the lists once at probe and restore the rules on open, as bcmgenet_resume() already does.

Could the changelog also mention this crash, since it affects how the patch is triaged for stable?

[ ... ]

Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913190052.939955-1-nb%40tipi-net.de
Reviewed-by on v1 1/1
#2 lore raw
In reply to Nicolai Buchwitz
Quoted 12 lines · Nicolai Buchwitz

bcmgenet_hfb_init() runs INIT_LIST_HEAD() on priv->rxnfc_list, which drops every rule off the list, and bcmgenet_open() calls it on each ifup. Every rule the user configured is silently lost:

   # ethtool -N eth0 flow-type ether dst $MAC action 0
   Added rule with ID 0
   # ethtool -n eth0 | grep -c Filter:
   1
   # ip link set eth0 down && ip link set eth0 up
   # ethtool -n eth0 | grep -c Filter:
   0
Quoted 5 lines

Initialise the lists once at probe and restore the rules on open, as bcmgenet_resume() already does.

Fixes 3e370952287c ("net: bcmgenet: add support for ethtool rxnfc flows")
Signed-off-by Nicolai Buchwitz <[email protected]>
Reviewed-by Justin Chen <[email protected]>

Thanks,
Justin

maintainer Reviewed-by on v1 1/1
#3 lore raw
In reply to Nicolai Buchwitz
Quoted 12 lines · Nicolai Buchwitz

bcmgenet_hfb_init() runs INIT_LIST_HEAD() on priv->rxnfc_list, which drops every rule off the list, and bcmgenet_open() calls it on each ifup. Every rule the user configured is silently lost:

   # ethtool -N eth0 flow-type ether dst $MAC action 0
   Added rule with ID 0
   # ethtool -n eth0 | grep -c Filter:
   1
   # ip link set eth0 down && ip link set eth0 up
   # ethtool -n eth0 | grep -c Filter:
   0
Quoted 5 lines

Initialise the lists once at probe and restore the rules on open, as bcmgenet_resume() already does.

Fixes 3e370952287c ("net: bcmgenet: add support for ethtool rxnfc flows")
Signed-off-by Nicolai Buchwitz <[email protected]>

With the sashiko feedback addressed:

Reviewed-by Florian Fainelli <[email protected]>
Signature · 1 line
Florian
CI Applied
#4 lore raw
In reply to Nicolai Buchwitz
This patch was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 12 lines · On Sun, 13 Sep 2026 21:00:52 +0200 you wrote:

bcmgenet_hfb_init() runs INIT_LIST_HEAD() on priv->rxnfc_list, which drops every rule off the list, and bcmgenet_open() calls it on each ifup. Every rule the user configured is silently lost:

  # ethtool -N eth0 flow-type ether dst $MAC action 0
  Added rule with ID 0
  # ethtool -n eth0 | grep -c Filter:
  1
  # ip link set eth0 down && ip link set eth0 up
  # ethtool -n eth0 | grep -c Filter:
  0
Quoted 1 line

[...]

Here is the summary with links:
- [net] net: bcmgenet: restore the hardware filters on open

    https://git.kernel.org/netdev/net/c/23ca4ddc4fce

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html