* Re: [PATCH] net: dsa: tag_rtl8_4: push VLAN tag into payload before DSA CPU tag
2026-09-08 4:00 [PATCH] net: dsa: tag_rtl8_4: push VLAN tag into payload before DSA CPU tag Amitesh Singh
@ 2026-09-08 11:50 ` Andrew Lunn
2026-09-08 14:26 ` Amitesh Singh
2026-09-10 22:01 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: Andrew Lunn @ 2026-09-08 11:50 UTC (permalink / raw)
To: Amitesh Singh; +Cc: netdev, linux-kernel, olteanv, kuba, davem
On Tue, Sep 08, 2026 at 09:30:41AM +0530, Amitesh Singh wrote:
> When a socket buffer has a hardware-accelerated VLAN tag (skb->vlan_tci
> set), the upstream NIC (e.g. imx-dwmac with tx-vlan-offload fixed:on)
> inserts the 802.1Q header after the DSA CPU tag, producing:
>
> [8100 VID][8899 CPU tag]
>
> on the wire instead of the correct ordering:
>
> [8899 CPU tag][8100 VID]
>
> The switch reads 0x8100 as the EtherType, does not recognise a valid
> CPU tag, and fails to strip it on egress. The raw 0x8899 tag then leaks
> to the peer port, breaking any protocol (e.g. batman-adv over a VLAN
> subinterface) that relies on seeing clean 802.1Q frames.
>
> Fix this by calling __vlan_hwaccel_push_inside() to move the VLAN tag
> into the skb payload before prepending the RTL8_4 DSA CPU tag in
> rtl8_4_tag_xmit(). The helper frees the skb internally on allocation
> failure, so returning NULL directly is correct and consistent with how
> tag_sja1105.c handles the same pattern.
Please take a step back. Rather than have two drivers do this, then a
third, a fourth, ... should the core be doing this?
Andrew
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] net: dsa: tag_rtl8_4: push VLAN tag into payload before DSA CPU tag
2026-09-08 4:00 [PATCH] net: dsa: tag_rtl8_4: push VLAN tag into payload before DSA CPU tag Amitesh Singh
2026-09-08 11:50 ` Andrew Lunn
@ 2026-09-10 22:01 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-10 22:01 UTC (permalink / raw)
To: singh.amitesh; +Cc: netdev, linux-kernel, andrew, olteanv, kuba, davem
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 1 · Low: 2
- [High] Headroom accounting mismatch in net/dsa/tag_rtl8_4.c:
rtl8_4_tag_xmit() now consumes VLAN_HLEN (4) bytes of headroom via…
- [Medium] Incomplete fix: the defect described in the commit message is
not specific to tag_rtl8_4.
- [Low] The patch fixes a user-visible data-path bug (VLAN-tagged traffic
through an RTL8365MB leaks raw 0x8899 CPU tags to the peer port,…
- [Low] The new comment in rtl8_4_tag_xmit()
(net/dsa/tag_rtl8_4.c:133-136) states the conduit NIC "will insert the…
--
net: dsa: tag_rtl8_4: push VLAN tag into payload before DSA CPU tag
This adds a __vlan_hwaccel_push_inside() call to rtl8_4_tag_xmit() so that
a hardware-accelerated VLAN tag is moved into the skb payload before the
8-byte RTL8_4 CPU tag is prepended. The stated goal is to keep the conduit
NIC from inserting the 802.1Q header at the CPU tag offset.
A few questions below.
> net: dsa: tag_rtl8_4: push VLAN tag into payload before DSA CPU tag
[Severity: Low]
Should this carry a Fixes: tag and name the target tree? It changes
data-path behaviour for VLAN traffic through an RTL8365MB, and git blame
attributes rtl8_4_tag_xmit() and the rest of the tagger to:
1521d5adfc2b ("net: dsa: tag_rtl8_4: add realtek 8 byte protocol 4 tag")
so that looks like the anchor stable tooling would want.
[Severity: Medium]
Is the problem really specific to tag_rtl8_4? The user port inherits the
conduit's VLAN features unconditionally in dsa_user_setup_tagger():
user->features = conduit->vlan_features | NETIF_F_HW_TC;
and nothing in net/dsa clears NETIF_F_HW_VLAN_CTAG_TX, so
validate_xmit_vlan() leaves the tag in hwaccel form:
if (skb_vlan_tag_present(skb) &&
!vlan_hw_offload_capable(features, skb->vlan_proto))
skb = __vlan_hwaccel_push_inside(skb);
The conduit then inserts the 802.1Q header at offset 12, which is exactly
where every dsa_alloc_etype_header() based CPU tag lives.
That includes the sibling Realtek tagger, rtl4a_tag_xmit(), which still
does:
skb_push(skb, RTL4_A_HDR_LEN);
dsa_alloc_etype_header(skb, RTL4_A_HDR_LEN);
with no push-inside, plus tag_brcm.c, tag_dsa.c, tag_lan9303.c,
tag_mt7628.c, tag_mtk.c, tag_qca.c, tag_netc.c and friends.
Was a core-side fix considered instead, for example not inheriting
NETIF_F_HW_VLAN_CTAG_TX for etype-position tag protocols, or doing the
push once in dsa_user_xmit()? Could the changelog say why the per-tagger
variant was chosen?
> diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
> index 4da3beebef75e..bc11ddc4e1784 100644
> --- a/net/dsa/tag_rtl8_4.c
> +++ b/net/dsa/tag_rtl8_4.c
> @@ -128,6 +128,19 @@ static void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev,
> static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
> struct net_device *dev)
> {
> + /* If the skb has a hardware-accelerated VLAN tag (skb->vlan_tci set),
> + * push it into the payload before prepending the DSA CPU tag.
> + * Otherwise the upstream NIC (e.g. imx-dwmac with tx-vlan-offload
> + * fixed:on) will insert the 802.1Q header *after* the CPU tag,
> + * producing [8100 VID][8899 CPU tag] on the wire instead of the
> + * correct [8899 CPU tag][8100 VID].
> + */
[Severity: Low]
The prose and the diagram in this comment disagree. It says the NIC
inserts the 802.1Q header "*after* the CPU tag", but the next line shows
the result as [8100 VID][8899 CPU tag], i.e. the 802.1Q header ends up
before the tag. The file header documents the same thing:
* +--------+--------+------------+------+-----
* | MAC DA | MAC SA | 8 byte tag | Type | ...
* +--------+--------+------------+------+-----
Should this read "before the CPU tag"?
> + if (skb_vlan_tag_present(skb)) {
> + skb = __vlan_hwaccel_push_inside(skb);
> + if (!skb)
> + return NULL;
> + }
> +
> skb_push(skb, RTL8_4_TAG_LEN);
[Severity: High]
Does this need .needed_headroom bumped by VLAN_HLEN as well? The tagger
still declares only:
net/dsa/tag_rtl8_4.c:rtl8_4_netdev_ops = {
...
.needed_headroom = RTL8_4_TAG_LEN,
};
The only headroom guarantee before the tagger runs comes from
dsa_user_xmit() -> skb_ensure_writable_head_tail(), which tops up to
exactly dev->needed_headroom and no more:
net/core/skbuff.c:skb_ensure_writable_head_tail() {
...
needed_headroom = max_t(int, needed_headroom - skb_headroom(skb), 0);
...
return pskb_expand_head(skb, needed_headroom, needed_tailroom, GFP_ATOMIC);
}
For a plain NIC conduit that is 8 bytes. __vlan_hwaccel_push_inside()
then goes through __vlan_insert_inner_tag(), which only asks for an
absolute VLAN_HLEN of headroom before consuming it:
include/linux/if_vlan.h:__vlan_insert_inner_tag() {
if (skb_cow_head(skb, meta_len + VLAN_HLEN) < 0)
return -ENOMEM;
skb_push(skb, VLAN_HLEN);
...
}
With 8 bytes of headroom present, skb_cow_head(skb, 4) does not expand
anything, so 4 bytes are left when control reaches
skb_push(skb, RTL8_4_TAG_LEN). Can that drive skb->data below skb->head
and hit skb_under_panic() -> BUG()?
The sja1105 tagger doing the same push declares:
net/dsa/tag_sja1105.c:sja1110_netdev_ops = {
...
.needed_headroom = SJA1110_HEADER_LEN + VLAN_HLEN,
};
Would .needed_headroom = RTL8_4_TAG_LEN + VLAN_HLEN, or an explicit
skb_cow_head(skb, RTL8_4_TAG_LEN) before the CPU-tag push, be the right
thing here?
There is a second effect: dsa_tag_protocol_overhead() is derived from
needed_headroom + needed_tailroom, so the conduit MTU is sized 4 bytes
short for full-size VLAN frames, and when the cow does fire it adds a
pskb_expand_head() per VLAN packet in the transmit path plus a newly
reachable allocation-failure drop.
>
> dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
>
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908040041.124640-1-singh.amitesh%40gmail.com
^ permalink raw reply [flat|nested] 4+ messages in thread