Bug 270736 - if_epair(4): Unexpected double tagged ICMP requests
Summary: if_epair(4): Unexpected double tagged ICMP requests
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 13.2-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Kristof Provost
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-04-10 09:51 UTC by Zhenlei Huang
Modified: 2023-09-07 01:31 UTC (History)
2 users (show)

See Also:
zlei: mfc-stable14+
zlei: mfc-stable13+
zlei: mfc-stable12+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Zhenlei Huang freebsd_committer freebsd_triage 2023-04-10 09:51:53 UTC
Steps to repeat:

```
# ifconfig epair create
epair0a
# jail -ic vnet persist
1
# ifconfig epair0a 192.0.2.1/24 pcp 3
# ifconfig epair0b vnet 1
# jexec 1 ifconfig epair0b inet 192.0.2.2/24
# ping -C5 -c1 192.0.2.2
PING 192.0.2.2 (192.0.2.2): 56 data bytes

--- 192.0.2.2 ping statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss
```

tcpdump on epair0b shows the ICMP request packet is double tagged.
```
17:48:15.742959 02:56:4d:eb:12:0a > 02:56:4d:eb:12:0b, ethertype 802.1Q (0x8100), length 106: vlan 0, p 5, ethertype 802.1Q, vlan 0, p 5, ethertype IPv4, (tos 0x0, ttl 64, id 6424, offset 0, flags [none], proto ICMP (1), length 84)
    192.0.2.1 > 192.0.2.2: ICMP echo request, id 24072, seq 0, length 64
```

Expected behavior: The ICMP request packets should have only one PCP 5 tag.
Comment 1 Kristof Provost freebsd_committer freebsd_triage 2023-04-10 12:39:00 UTC
That's a fun one.

It looks like the problem is actually on the receive side, in that we don't clear the M_VLANTAG from m->m_flags and end up thinking there's an extra vlan on the RX side.

if_epair already has some code to remove tags and such when handing a packet over to the receive path, but it didn't clear the vlan information. (Arguably we could also clear it on the network stack side, but this is pretty specific to if_epair.)

I'm testing this now:

diff --git a/sys/net/if_epair.c b/sys/net/if_epair.c
index aeed993249f5..21c0c598e8d4 100644
--- a/sys/net/if_epair.c
+++ b/sys/net/if_epair.c
@@ -140,6 +140,11 @@ epair_clear_mbuf(struct mbuf *m)
                m->m_pkthdr.csum_flags &= ~CSUM_SND_TAG;
        }

+       /* Clear vlan information. */
+       m->m_flags &= ~M_VLANTAG;
+       if (m->m_flags & M_PKTHDR)
+               m->m_pkthdr.ether_vtag = 0;
+
        m_tag_delete_nonpersistent(m);
 }
Comment 3 commit-hook freebsd_committer freebsd_triage 2023-04-10 15:56:55 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=c69ae8419734829404bdb47d694d105c85f9835e

commit c69ae8419734829404bdb47d694d105c85f9835e
Author:     Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2023-04-10 11:02:55 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2023-04-10 13:55:35 +0000

    if_epair: also remove vlan metadata from mbufs

    We already remove mbuf tags from packets transitting an if_epair, but we
    didn't remove vlan metadata.
    In certain configurations this could lead to unexpected vlan tags
    turning up on the rx side.

    PR:             270736
    Reviewed by:    markj
    MFC after:      1 week
    Sponsored by:   Rubicon Communications, LLC ("Netgate")
    Differential Revision:  https://reviews.freebsd.org/D39482

 sys/net/if_epair.c | 6 ++++++
 1 file changed, 6 insertions(+)
Comment 4 commit-hook freebsd_committer freebsd_triage 2023-04-10 15:56:56 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d116b8430b90212e308fe9945038c7bd98edf1bc

commit d116b8430b90212e308fe9945038c7bd98edf1bc
Author:     Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2023-04-10 10:55:10 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2023-04-10 13:55:53 +0000

    epair tests: test PCP tagged packets

    PR:             270736
    MFC after:      1 week
    Sponsored by:   Rubicon Communications, LLC ("Netgate")
    Differential Revision:  https://reviews.freebsd.org/D39483

 tests/sys/net/Makefile               |  1 +
 tests/sys/net/if_epair_test.sh (new) | 65 ++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)
Comment 5 Zhenlei Huang freebsd_committer freebsd_triage 2023-04-11 10:28:23 UTC
Test with cxgbe(4), it is also affected.

See proposal patch: https://reviews.freebsd.org/D39499
Comment 6 commit-hook freebsd_committer freebsd_triage 2023-04-18 07:23:52 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=c79831b38a63c5e2b6c20602104f1ccda7075182

commit c79831b38a63c5e2b6c20602104f1ccda7075182
Author:     Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2023-04-10 10:55:10 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2023-04-18 07:23:16 +0000

    epair tests: test PCP tagged packets

    PR:             270736
    MFC after:      1 week
    Sponsored by:   Rubicon Communications, LLC ("Netgate")
    Differential Revision:  https://reviews.freebsd.org/D39483

    (cherry picked from commit d116b8430b90212e308fe9945038c7bd98edf1bc)

 tests/sys/net/Makefile               |  1 +
 tests/sys/net/if_epair_test.sh (new) | 65 ++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)
Comment 7 commit-hook freebsd_committer freebsd_triage 2023-04-18 07:23:53 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=8011e2cd245e64469891a9afe23c32b3fef5e503

commit 8011e2cd245e64469891a9afe23c32b3fef5e503
Author:     Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2023-04-10 11:02:55 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2023-04-18 07:23:15 +0000

    if_epair: also remove vlan metadata from mbufs

    We already remove mbuf tags from packets transitting an if_epair, but we
    didn't remove vlan metadata.
    In certain configurations this could lead to unexpected vlan tags
    turning up on the rx side.

    PR:             270736
    Reviewed by:    markj
    MFC after:      1 week
    Sponsored by:   Rubicon Communications, LLC ("Netgate")
    Differential Revision:  https://reviews.freebsd.org/D39482

    (cherry picked from commit c69ae8419734829404bdb47d694d105c85f9835e)

 sys/net/if_epair.c | 6 ++++++
 1 file changed, 6 insertions(+)
Comment 8 commit-hook freebsd_committer freebsd_triage 2023-08-30 09:39:10 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=b22aae410bc7e4e9a6b43e556dc34be72deadb65

commit b22aae410bc7e4e9a6b43e556dc34be72deadb65
Author:     Zhenlei Huang <zlei@FreeBSD.org>
AuthorDate: 2023-08-30 09:36:38 +0000
Commit:     Zhenlei Huang <zlei@FreeBSD.org>
CommitDate: 2023-08-30 09:36:38 +0000

    net: Remove vlan metadata on pcp / vlan encapsulation

    For oubound traffic, the flag M_VLANTAG is set in mbuf packet header to
    indicate the underlaying interface do hardware VLAN tag insertion if
    capable, otherwise the net stack will do 802.1Q encapsulation instead.

    Commit 868aabb4708d introduced per-flow priority which set the priority ID
    in the mbuf packet header. There's a corner case that when the driver is
    disabled to do hardware VLAN tag insertion, and the net stack do 802.1Q
    encapsulation, then it will result double tagged packets if the driver do
    not check the enabled capability (hardware VLAN tag insertion).

    Unfortunately some drivers, currently known cxgbe(4) re(4) ure(4) igc(4)
    and vmx(4), have this issue. From a quick review for other interface
    drivers I believe a lot more drivers have the same issue. It makes more
    sense to fix in net stack than to try to change every single driver.

    PR:     270736
    Reviewed by:    kp
    Fixes:  868aabb4708d Add IP(V6)_VLAN_PCP to set 802.1 priority per-flow
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D39499

 sys/net/if_ethersubr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Comment 9 commit-hook freebsd_committer freebsd_triage 2023-09-06 04:10:11 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=494de30b63de9ef31587dcc3fb8e7249535e840a

commit 494de30b63de9ef31587dcc3fb8e7249535e840a
Author:     Zhenlei Huang <zlei@FreeBSD.org>
AuthorDate: 2023-08-30 09:36:38 +0000
Commit:     Zhenlei Huang <zlei@FreeBSD.org>
CommitDate: 2023-09-06 04:07:21 +0000

    net: Remove vlan metadata on pcp / vlan encapsulation

    For oubound traffic, the flag M_VLANTAG is set in mbuf packet header to
    indicate the underlaying interface do hardware VLAN tag insertion if
    capable, otherwise the net stack will do 802.1Q encapsulation instead.

    Commit 868aabb4708d introduced per-flow priority which set the priority ID
    in the mbuf packet header. There's a corner case that when the driver is
    disabled to do hardware VLAN tag insertion, and the net stack do 802.1Q
    encapsulation, then it will result double tagged packets if the driver do
    not check the enabled capability (hardware VLAN tag insertion).

    Unfortunately some drivers, currently known cxgbe(4) re(4) ure(4) igc(4)
    and vmx(4), have this issue. From a quick review for other interface
    drivers I believe a lot more drivers have the same issue. It makes more
    sense to fix in net stack than to try to change every single driver.

    PR:             270736
    Reviewed by:    kp
    Approved by:    re (gjb)
    Fixes:  868aabb4708d Add IP(V6)_VLAN_PCP to set 802.1 priority per-flow
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D39499

    (cherry picked from commit b22aae410bc7e4e9a6b43e556dc34be72deadb65)

 sys/net/if_ethersubr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Comment 10 commit-hook freebsd_committer freebsd_triage 2023-09-06 04:20:15 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=337475505b4cdf40510bd2e2788fdce5c0c47367

commit 337475505b4cdf40510bd2e2788fdce5c0c47367
Author:     Zhenlei Huang <zlei@FreeBSD.org>
AuthorDate: 2023-08-30 09:36:38 +0000
Commit:     Zhenlei Huang <zlei@FreeBSD.org>
CommitDate: 2023-09-06 04:18:13 +0000

    net: Remove vlan metadata on pcp / vlan encapsulation

    For oubound traffic, the flag M_VLANTAG is set in mbuf packet header to
    indicate the underlaying interface do hardware VLAN tag insertion if
    capable, otherwise the net stack will do 802.1Q encapsulation instead.

    Commit 868aabb4708d introduced per-flow priority which set the priority ID
    in the mbuf packet header. There's a corner case that when the driver is
    disabled to do hardware VLAN tag insertion, and the net stack do 802.1Q
    encapsulation, then it will result double tagged packets if the driver do
    not check the enabled capability (hardware VLAN tag insertion).

    Unfortunately some drivers, currently known cxgbe(4) re(4) ure(4) igc(4)
    and vmx(4), have this issue. From a quick review for other interface
    drivers I believe a lot more drivers have the same issue. It makes more
    sense to fix in net stack than to try to change every single driver.

    PR:             270736
    Reviewed by:    kp
    Fixes:  868aabb4708d Add IP(V6)_VLAN_PCP to set 802.1 priority per-flow
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D39499

    (cherry picked from commit b22aae410bc7e4e9a6b43e556dc34be72deadb65)
    (cherry picked from commit 494de30b63de9ef31587dcc3fb8e7249535e840a)

 sys/net/if_ethersubr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Comment 11 commit-hook freebsd_committer freebsd_triage 2023-09-06 04:53:30 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=c0038743d357da9b5ebf756f4fc3b2cf7d2ef3e9

commit c0038743d357da9b5ebf756f4fc3b2cf7d2ef3e9
Author:     Zhenlei Huang <zlei@FreeBSD.org>
AuthorDate: 2023-08-30 09:36:38 +0000
Commit:     Zhenlei Huang <zlei@FreeBSD.org>
CommitDate: 2023-09-06 04:32:56 +0000

    net: Remove vlan metadata on pcp / vlan encapsulation

    For oubound traffic, the flag M_VLANTAG is set in mbuf packet header to
    indicate the underlaying interface do hardware VLAN tag insertion if
    capable, otherwise the net stack will do 802.1Q encapsulation instead.

    Commit 868aabb4708d introduced per-flow priority which set the priority ID
    in the mbuf packet header. There's a corner case that when the driver is
    disabled to do hardware VLAN tag insertion, and the net stack do 802.1Q
    encapsulation, then it will result double tagged packets if the driver do
    not check the enabled capability (hardware VLAN tag insertion).

    Unfortunately some drivers, currently known cxgbe(4) re(4) ure(4) igc(4)
    and vmx(4), have this issue. From a quick review for other interface
    drivers I believe a lot more drivers have the same issue. It makes more
    sense to fix in net stack than to try to change every single driver.

    PR:             270736
    Reviewed by:    kp
    Fixes:  868aabb4708d Add IP(V6)_VLAN_PCP to set 802.1 priority per-flow
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D39499

    (cherry picked from commit b22aae410bc7e4e9a6b43e556dc34be72deadb65)
    (cherry picked from commit 494de30b63de9ef31587dcc3fb8e7249535e840a)
    (cherry picked from commit 337475505b4cdf40510bd2e2788fdce5c0c47367)

 sys/net/if_ethersubr.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
Comment 12 Zhenlei Huang freebsd_committer freebsd_triage 2023-09-07 01:31:27 UTC
Works solidly now.