[PATCH net] net: stmmac: rebuild VLAN priority steering when filters change

[PATCH net] net: stmmac: rebuild VLAN priority steering when filters change

[James Hilliard]

The RX queue priority callbacks preserve previously assigned priorities.
Passing zero from tc_del_vlan_flow() therefore leaves the deleted rule's
priority mapped to its RX queue. Clearing the whole queue instead would
also remove priorities belonging to other installed rules.

Save each VLAN rule's priority and rebuild the complete per-queue masks
after insertion and deletion, starting from the platform assignments.
Resolve overlapping platform masks with the same last-queue-wins ordering
used during initialization.

Make the GMAC4 and XGMAC callbacks replace the target queue's mask while
retaining their exclusion of its priorities from other queues. Program a
priority's new destination before clearing its previous assignment.

Keep VLAN rules in programming order. Flower installs a replacement under
a new cookie before deleting the old one, so the newer assignment must
remain effective while both are present, including when an earlier array
slot is reused. Removing a rejected replacement restores the older rule,
and removing the last override restores the platform assignment.

Reoffloading an existing cookie reuses its entry without incrementing the
rule count. Validate the selected RX queue before indexing the masks:
tc_classid_to_hwtc() checks the number of traffic classes, which can exceed
the number of RX queues.

Found by code inspection while working on STMMAC reset/recovery fixes.
Host-side tests exercising the driver's VLAN rule handlers and priority
callbacks with mocked MMIO reproduce the stale mapping after deletion
and pass with this fix. No hardware testing was performed.

Fixes: b3da86d432b7 ("net: stmmac: fix rx queue priority assignment")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c  |  2 +
 .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    |  2 +
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  4 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c    | 54 +++++++++++++++++++---
 4 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index 259e2ebed8c1..efbe2ec4c213 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -126,6 +126,7 @@ static void dwmac4_rx_queue_priority(struct mac_device_info *hw,
 	 * clear them from others queues
 	 */
 	if (queue < 4) {
+		ctrl2 &= ~GMAC_RXQCTRL_PSRQX_MASK(queue);
 		ctrl2 |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
 						GMAC_RXQCTRL_PSRQX_MASK(queue);
 
@@ -134,6 +135,7 @@ static void dwmac4_rx_queue_priority(struct mac_device_info *hw,
 	} else {
 		queue -= 4;
 
+		ctrl3 &= ~GMAC_RXQCTRL_PSRQX_MASK(queue);
 		ctrl3 |= (prio << GMAC_RXQCTRL_PSRQX_SHIFT(queue)) &
 						GMAC_RXQCTRL_PSRQX_MASK(queue);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index 1a88cbaed70c..7ad08651675e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -119,6 +119,7 @@ static void dwxgmac2_rx_queue_prio(struct mac_device_info *hw, u32 prio,
 	 * clear them from others queues
 	 */
 	if (queue < 4) {
+		ctrl2 &= ~XGMAC_PSRQ(queue);
 		ctrl2 |= (prio << XGMAC_PSRQ_SHIFT(queue)) &
 						XGMAC_PSRQ(queue);
 
@@ -127,6 +128,7 @@ static void dwxgmac2_rx_queue_prio(struct mac_device_info *hw, u32 prio,
 	} else {
 		queue -= 4;
 
+		ctrl3 &= ~XGMAC_PSRQ(queue);
 		ctrl3 |= (prio << XGMAC_PSRQ_SHIFT(queue)) &
 						XGMAC_PSRQ(queue);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index b76acfe0d958..fcc395321816 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -14,6 +14,7 @@
 #include <linux/clk.h>
 #include <linux/hrtimer.h>
 #include <linux/if_vlan.h>
+#include <linux/list.h>
 #include <linux/stmmac.h>
 #include <linux/phylink.h>
 #include <linux/pci.h>
@@ -210,6 +211,8 @@ enum stmmac_rfs_type {
 struct stmmac_rfs_entry {
 	unsigned long cookie;
 	u16 etype;
+	u8 vlan_prio;
+	struct list_head vlan_node;
 	int in_use;
 	int type;
 	int tc;
@@ -382,6 +385,7 @@ struct stmmac_priv {
 	unsigned int rfs_entries_cnt[STMMAC_RFS_T_MAX];
 	unsigned int rfs_entries_total;
 	struct stmmac_rfs_entry *rfs_entries;
+	struct list_head vlan_prio;
 
 	/* Pulse Per Second output */
 	struct stmmac_pps_cfg pps[STMMAC_PPS_MAX];
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 10b61fa2c27c..6707f7621f3b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -250,6 +250,8 @@ static int tc_rfs_init(struct stmmac_priv *priv)
 	if (!priv->rfs_entries)
 		return -ENOMEM;
 
+	INIT_LIST_HEAD(&priv->vlan_prio);
+
 	dev_info(priv->device, "Enabled RFS Flow TC (entries=%d)\n",
 		 priv->rfs_entries_total);
 
@@ -685,6 +687,41 @@ static struct stmmac_rfs_entry *tc_find_rfs(struct stmmac_priv *priv,
 
 #define VLAN_PRIO_FULL_MASK (0x07)
 
+static void tc_update_vlan_prio(struct stmmac_priv *priv, u8 prio)
+{
+	u8 prios[MTL_MAX_RX_QUEUES] = {};
+	struct stmmac_rfs_entry *entry;
+	unsigned int queue, i;
+
+	/* Restore platform assignments with the same precedence as setup. */
+	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++) {
+		if (!priv->plat->rx_queues_cfg[queue].use_prio)
+			continue;
+
+		prios[queue] = priv->plat->rx_queues_cfg[queue].prio;
+		for (i = 0; i < queue; i++)
+			prios[i] &= ~prios[queue];
+	}
+
+	/*
+	 * Flower installs a replacement before destroying the old cookie.
+	 * Preserve the latest assignment while both rules are present.
+	 */
+	list_for_each_entry(entry, &priv->vlan_prio, vlan_node) {
+		for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++)
+			prios[queue] &= ~entry->vlan_prio;
+		prios[entry->tc] |= entry->vlan_prio;
+	}
+
+	/* Install the new assignment before clearing the previous queue. */
+	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++)
+		if (prios[queue] & prio)
+			stmmac_rx_queue_prio(priv, priv->hw, prios[queue], queue);
+	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++)
+		if (!(prios[queue] & prio))
+			stmmac_rx_queue_prio(priv, priv->hw, prios[queue], queue);
+}
+
 static int tc_add_vlan_flow(struct stmmac_priv *priv,
 			    struct flow_cls_offload *cls)
 {
@@ -700,7 +737,7 @@ static int tc_add_vlan_flow(struct stmmac_priv *priv,
 			return -ENOENT;
 	}
 
-	if (priv->rfs_entries_cnt[STMMAC_RFS_T_VLAN] >=
+	if (!entry->in_use && priv->rfs_entries_cnt[STMMAC_RFS_T_VLAN] >=
 	    priv->rfs_entries_max[STMMAC_RFS_T_VLAN])
 		return -ENOENT;
 
@@ -708,7 +745,7 @@ static int tc_add_vlan_flow(struct stmmac_priv *priv,
 	if (!dissector_uses_key(dissector, FLOW_DISSECTOR_KEY_VLAN))
 		return -EINVAL;
 
-	if (tc < 0) {
+	if (tc < 0 || tc >= priv->plat->rx_queues_to_use) {
 		netdev_err(priv->dev, "Invalid traffic class\n");
 		return -EINVAL;
 	}
@@ -724,13 +761,18 @@ static int tc_add_vlan_flow(struct stmmac_priv *priv,
 		}
 
 		prio = BIT(match.key->vlan_priority);
-		stmmac_rx_queue_prio(priv, priv->hw, prio, tc);
+		if (entry->in_use)
+			list_del(&entry->vlan_node);
+		else
+			priv->rfs_entries_cnt[STMMAC_RFS_T_VLAN]++;
 
 		entry->in_use = true;
 		entry->cookie = cls->cookie;
 		entry->tc = tc;
 		entry->type = STMMAC_RFS_T_VLAN;
-		priv->rfs_entries_cnt[STMMAC_RFS_T_VLAN]++;
+		entry->vlan_prio = prio;
+		list_add_tail(&entry->vlan_node, &priv->vlan_prio);
+		tc_update_vlan_prio(priv, entry->vlan_prio);
 	}
 
 	return 0;
@@ -744,14 +786,14 @@ static int tc_del_vlan_flow(struct stmmac_priv *priv,
 	if (!entry || !entry->in_use || entry->type != STMMAC_RFS_T_VLAN)
 		return -ENOENT;
 
-	stmmac_rx_queue_prio(priv, priv->hw, 0, entry->tc);
-
+	list_del(&entry->vlan_node);
 	entry->in_use = false;
 	entry->cookie = 0;
 	entry->tc = 0;
 	entry->type = 0;
 
 	priv->rfs_entries_cnt[STMMAC_RFS_T_VLAN]--;
+	tc_update_vlan_prio(priv, entry->vlan_prio);
 
 	return 0;
 }

---
base-commit: c3e67bd82b9b9b1a5118f09a429ccefe2aab0741
change-id: 20261102-submit-stmmac-vlan-prio-v1-8e5f674f1201

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>

Re: [PATCH net] net: stmmac: rebuild VLAN priority steering when filters change

[netdev-bot+sashiko]

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

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

- [High] tc_add_vlan_flow()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c:764-765) now runs `if…
- [High] tc_add_ethtype_flow() (stmmac_tc.c:801-871) can take over an in-
  use VLAN entry found by cookie.
- [Medium] The new check `tc >= priv->plat->rx_queues_to_use` in
  tc_add_vlan_flow() (stmmac_tc.c:748) runs only when a rule is inserted.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but VLAN-
  priority steering programmed into RXQ_CTRL2/3 (PSRQn) for tc-flower…
- [Medium] This problem wasn't introduced by this patch, but
  tc_add_ethtype_flow() (stmmac_tc.c:820) still checks only `tc < 0`…

--

net: stmmac: rebuild VLAN priority steering when filters change

This patch puts each VLAN tc-flower rule's priority on a new per-device
list. After every insert and delete, it rebuilds the full RX queue
priority masks from the platform assignments plus that list. It also
makes the GMAC4/XGMAC rx_queue_prio callbacks replace the target queue's
mask, and checks hw_tc against rx_queues_to_use in tc_add_vlan_flow().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 10b61fa2c27c5..6707f7621f3b8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -685,6 +687,41 @@ static struct stmmac_rfs_entry *tc_find_rfs(struct stmmac_priv *priv,
>  
>  #define VLAN_PRIO_FULL_MASK (0x07)
>  
> +static void tc_update_vlan_prio(struct stmmac_priv *priv, u8 prio)
> +{
> +	u8 prios[MTL_MAX_RX_QUEUES] = {};
> +	struct stmmac_rfs_entry *entry;
> +	unsigned int queue, i;
> +

[ ... ]

> +	list_for_each_entry(entry, &priv->vlan_prio, vlan_node) {
> +		for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++)
> +			prios[queue] &= ~entry->vlan_prio;
> +		prios[entry->tc] |= entry->vlan_prio;
> +	}

[Severity: Medium]
The tc >= rx_queues_to_use check in tc_add_vlan_flow() only runs when a
rule is inserted. What happens to an installed rule after ethtool -L
reduces the RX queue count?

stmmac_reinit_queues() does:

	priv->plat->rx_queues_to_use = rx_cnt;

It does not revalidate or remove the installed VLAN rules. Take a rule
installed with hw_tc N, then run ethtool -L with rx <= N. On the next
unrelated VLAN rule add or delete, this loop clears that rule's
vlan_prio from every active queue. It then sets the bit in
prios[entry->tc], which neither programming loop below reaches, since
both stop at rx_queues_to_use.

Does that leave the PCP mapped to no RX queue at all? That would
override both the platform rx_queues_cfg[].prio assignment and any older
rule. There is no out-of-bounds access, because entry->tc is still below
the old rx_queues_to_use.

The commit message says "Validate the selected RX queue before indexing
the masks", but that only holds at insertion time.

> +
> +	/* Install the new assignment before clearing the previous queue. */
> +	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++)
> +		if (prios[queue] & prio)
> +			stmmac_rx_queue_prio(priv, priv->hw, prios[queue], queue);
> +	for (queue = 0; queue < priv->plat->rx_queues_to_use; queue++)
> +		if (!(prios[queue] & prio))
> +			stmmac_rx_queue_prio(priv, priv->hw, prios[queue], queue);
> +}

[Severity: Medium]
This isn't a bug introduced by this patch, but is the VLAN priority
steering programmed here lost every time stmmac_hw_setup() runs?

stmmac_hw_setup() runs on stmmac_open(), on resume, and on the ethtool
-L/-G reinit path. stmmac_init_dma_engine() calls stmmac_reset(), which
clears RXQ_CTRL2/3. After that, stmmac_mtl_configuration() calls
stmmac_mac_config_rx_queues_prio(), which only reprograms the platform
masks:

	for (queue = 0; queue < rx_queues_count; queue++) {
		if (!priv->plat->rx_queues_cfg[queue].use_prio)
			continue;

		prio = priv->plat->rx_queues_cfg[queue].prio;
		stmmac_rx_queue_prio(priv, priv->hw, prio, queue);
	}

priv->vlan_prio is never replayed there. Software still treats those
rules as installed, while the hardware steers by the platform defaults.

With this patch, the lost rules only come back as a side effect of the
next unrelated VLAN rule add or delete. A rule added while the interface
is down is wiped by the next open.

Could stmmac_mtl_configuration() use the new list to restore the rules
after a reset?

> +
>  static int tc_add_vlan_flow(struct stmmac_priv *priv,
>  			    struct flow_cls_offload *cls)
>  {

[ ... ]

> @@ -708,7 +745,7 @@ static int tc_add_vlan_flow(struct stmmac_priv *priv,
>  	if (!dissector_uses_key(dissector, FLOW_DISSECTOR_KEY_VLAN))
>  		return -EINVAL;
>  
> -	if (tc < 0) {
> +	if (tc < 0 || tc >= priv->plat->rx_queues_to_use) {
>  		netdev_err(priv->dev, "Invalid traffic class\n");
>  		return -EINVAL;
>  	}

[Severity: Medium]
This is a pre-existing issue, but tc_add_ethtype_flow() still only
checks tc < 0 before it uses tc as the RX queue:

	if (tc < 0) {
		netdev_err(priv->dev, "Invalid traffic class\n");
		return -EINVAL;
	}
	...
	stmmac_rx_queue_routing(priv, priv->hw,
				PACKET_DCBCPQ, tc);

The same point about tc_classid_to_hwtc() only bounding by
netdev_get_num_tc() applies there. Can an LLDP or PTP rule with
hw_tc >= rx_queues_to_use send those frames to an RX queue that isn't
enabled, so they are never received?

Should tc_add_ethtype_flow() get the same bound?

> @@ -724,13 +761,18 @@ static int tc_add_vlan_flow(struct stmmac_priv *priv,
>  		}
>  
>  		prio = BIT(match.key->vlan_priority);
> -		stmmac_rx_queue_prio(priv, priv->hw, prio, tc);
> +		if (entry->in_use)
> +			list_del(&entry->vlan_node);
> +		else
> +			priv->rfs_entries_cnt[STMMAC_RFS_T_VLAN]++;

[Severity: High]
Can this list_del() run on an entry that was never linked on
priv->vlan_prio?

tc_find_rfs() matches on cookie only:

	if (entry->cookie == cls->cookie)
		return entry;

So the entry returned here can be an in-use LLDP or 1588 entry.

tc_add_flow_cls() falls through to tc_add_vlan_flow() whenever
tc_add_ethtype_flow() fails. For an LLDP cookie that is already
installed, tc_add_ethtype_flow() returns -ENOENT because the entry
counts against itself:

	case ETH_P_LLDP:
		if (priv->rfs_entries_cnt[STMMAC_RFS_T_LLDP] >=
		    priv->rfs_entries_max[STMMAC_RFS_T_LLDP])
			return -ENOENT;

An LLDP/1588 entry's vlan_node is either still zeroed from
devm_kcalloc() in tc_rfs_init(), or holds LIST_POISON1/2 if
tc_del_vlan_flow() released the slot earlier. In both cases __list_del()
writes through a NULL or poison pointer here, with rtnl held.

One way to reach this with CAP_NET_ADMIN:

1) Bind a shared ingress block to eth0 and to a second device.

2) Add "protocol 802.1Q flower vlan_prio 3 vlan_ethtype 0x88cc hw_tc 1"
   to the block. tc_add_flow() fails, and tc_add_ethtype_flow() installs
   the rule as an LLDP entry with cookie C.

3) Run "ethtool -K eth0 hw-tc-offload off". "rxhash on" also works,
   since tc_setup_cls() then returns -EBUSY.

4) Run "tc qdisc del dev eth0 ingress". FLOW_CLS_DESTROY fails in
   stmmac_setup_tc_block_cb(), and tcf_block_playback_offloads()
   ignores errors on removal. The LLDP entry stays in use with
   cookie C.

5) Turn hw-tc-offload back on and re-bind the block. fl_reoffload()
   replays FLOW_CLS_REPLACE with cookie C. tc_add_ethtype_flow()
   returns -ENOENT, and tc_add_vlan_flow() reaches this list_del() with
   entry->in_use set.

tc_del_vlan_flow() checks entry->type != STMMAC_RFS_T_VLAN before its
list_del(), but the add path doesn't. Should the entry only be unlinked
and reused when entry->type == STMMAC_RFS_T_VLAN, with the request
rejected otherwise?

The commit message says "Reoffloading an existing cookie reuses its
entry without incrementing the rule count", but that only covers
reoffloading a VLAN cookie as VLAN. Before this patch the same type mixup
only threw off the counters.

>  
>  		entry->in_use = true;
>  		entry->cookie = cls->cookie;
>  		entry->tc = tc;
>  		entry->type = STMMAC_RFS_T_VLAN;
> -		priv->rfs_entries_cnt[STMMAC_RFS_T_VLAN]++;
> +		entry->vlan_prio = prio;
> +		list_add_tail(&entry->vlan_node, &priv->vlan_prio);
> +		tc_update_vlan_prio(priv, entry->vlan_prio);
>  	}

[Severity: High]
Can a VLAN entry stay linked on priv->vlan_prio after it has been
re-typed to LLDP or 1588?

tc_add_ethtype_flow() can also take over an in-use entry found by
cookie, including a VLAN rule that is linked here. It overwrites
entry->type and entry->tc but never calls list_del(&entry->vlan_node):

	entry->type = STMMAC_RFS_T_LLDP;
	priv->rfs_entries_cnt[STMMAC_RFS_T_LLDP]++;
	...
	entry->in_use = true;
	entry->cookie = cls->cookie;
	entry->tc = tc;

Later, tc_del_ethtype_flow() frees the slot (in_use = false,
cookie = 0) while vlan_node is still linked. It also never decrements
rfs_entries_cnt[STMMAC_RFS_T_VLAN].

The next VLAN rule that gets that slot from tc_find_rfs(priv, cls, true)
takes the !in_use branch. It then calls list_add_tail() on a node that
is already on the list.

Depending on the list layout, the node either ends up pointing to
itself, or other nodes drop out of the list and a later list_del()
leaves a self-loop. Would list_for_each_entry() in tc_update_vlan_prio()
then loop forever with rtnl held?

The same stale-cookie mechanism as above gets here:

1) Install LLDP rule L.

2) Install R = "protocol 802.1Q flower vlan_prio 3 vlan_ethtype 0x88cc
   hw_tc 2". The LLDP slot is full, so R becomes a VLAN entry.

3) Delete L.

4) Make R's entry stale with hw-tc-offload off plus unbind, then turn
   it back on and re-bind. fl_reoffload() replays R, and
   tc_add_ethtype_flow() re-types the linked VLAN entry.

5) Delete R.

6) Add VLAN rules until one of them reuses that slot.

While a re-typed entry is on the list, tc_update_vlan_prio() also does:

	prios[entry->tc] |= entry->vlan_prio;

prios[] is a u8 [MTL_MAX_RX_QUEUES] array, and tc_add_ethtype_flow()
only checks tc < 0. Suppose a new LLDP/1588 filter takes a stale VLAN
cookie (a freed filter address reused by slab) with hw_tc >= 8. That
needs num_tc > 8, for example taprio with overlapping TXQs. Could this
then write past the end of prios[]?

Should tc_add_ethtype_flow() unlink a VLAN entry before re-typing it,
or refuse to take over an entry of a different type?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102-submit-stmmac-vlan-prio-v1-v1-1-2b37767e4e08%40gmail.com