Page MenuHomeFreeBSD

net: Check per-flow priority code point for untagged traffic
ClosedPublic

Authored by zlei on Apr 12 2023, 4:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 25 2024, 12:47 PM
Unknown Object (File)
Jan 9 2024, 5:51 AM
Unknown Object (File)
Jan 7 2024, 2:33 PM
Unknown Object (File)
Jan 7 2024, 2:32 PM
Unknown Object (File)
Jan 7 2024, 2:32 PM
Unknown Object (File)
Dec 30 2023, 5:17 PM
Unknown Object (File)
Dec 28 2023, 6:05 AM
Unknown Object (File)
Dec 23 2023, 2:29 AM

Details

Summary

Commit 868aabb4708d introduced per-flow priority. There's a fault in the
logic for untagged traffic, it does not check M_VLANTAG which is set in
the mbuf packet header and pass the mbuf to underlaying interface. When
the driver is incapable or disabled to do hardware VLAN tag insertion then
the outbound packets will not include the desired priority.

Some drivers happen to work due to bug mentioned in D39499, so it may not
repeat on those drivers.

The current logic also bypass soft pad processing and checking priority
set by firewall (currently only pf).

Fix by checking flag M_VLANTAG which is set in mbuf packet header and
priority set in mbuf tag MTAG_8021Q / MTAG_8021Q_PCP_OUT.

PR: 273431
Fixes: 868aabb4708d Add IP(V6)_VLAN_PCP to set 802.1 priority per-flow
MFC after: 1 week

Test Plan

Test per-flow priority and priority set by firewall (pf) .

em0 <---> igc0

Capture packets on igc0

# ifconfig igc0 up
# tcpdump -nevi igc0

Setup

# ifconfig em0 -vlanhwtag -pcp
# ifconfig em0 inet 192.0.2.1/24
# arp -s 192.0.2.2 0:1:2:3:4:5

Test per-flow priority

# ping -c1 -C3 192.0.2.2

Test priority set by firewall (pf) .

# echo "pass out proto icmp set prio 5" > /tmp/pf.conf
# kldload pf
# pfctl -f /tmp/pf.conf
# pfctl -e
pf enabled
# sysctl net.link.vlan.mtag_pcp=1
net.link.vlan.mtag_pcp: 0 -> 1
# ping -c1 192.0.2.2

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

zlei requested review of this revision.Apr 12 2023, 4:40 PM

I think that it happens to work because many hardwares have VLAN tag offload, and the driver rely on (m->m_flags & M_VLANTAG) to do VLAN tagging (without checking its capability IFCAP_VLAN_HWTAGGING).

sys/net/if_ethersubr.c
450–453
476

Do we need both M_VLANTAG and IFT_L2VLAN checks?
It's worth commenting the usecases here.

sys/net/if_ethersubr.c
476

Do we need both M_VLANTAG and IFT_L2VLAN checks?
It's worth commenting the usecases here.

Actually it is perfect valid for users to set per-flow PCP and the transmit interface is not if_vlan.

So when I added the capability, it happened to work incidenitally one the nic drivers i had tested this on? And this would extend the proper operation to other drivers (or skip the PCP setting, when no vlan tagging capabilities exist?

So when I added the capability, it happened to work incidenitally one the nic drivers i had tested this on?

I think so. I've tested with re(4) and cxgbe(4), both work even I disable vlanhwtag (ifconfig re0 -vlanhwtag).

And this would extend the proper operation to other drivers (or skip the PCP setting, when no vlan tagging capabilities exist?

Yes.
I do not have such devices, but I expect per-flow PCP will fail on if_epair with the recent change c69ae8419734 .

zlei retitled this revision from net: Check per-flow priority code point to net: Check per-flow priority code point for untagged traffic.
zlei edited the summary of this revision. (Show Details)
zlei edited the test plan for this revision. (Show Details)
zlei marked 2 inline comments as done.Aug 29 2023, 4:23 PM
zlei added a subscriber: kp.
zlei added inline comments.
sys/net/if_ethersubr.c
475

We should respect firewall (pf) 's PCP tag. Can @kp comment on this ?

See also mailing list https://lists.freebsd.org/archives/freebsd-net/2023-August/003852.html

sys/net/if_ethersubr.c
471

Blank lines are not needed (and style(9) actually prohibits them)

472
zlei edited the summary of this revision. (Show Details)
zlei edited the test plan for this revision. (Show Details)

Fix errors when compile.

zlei marked 2 inline comments as done.Aug 30 2023, 7:53 AM
zlei added inline comments.
sys/net/if_ethersubr.c
476

Do we need both M_VLANTAG and IFT_L2VLAN checks?
It's worth commenting the usecases here.

I may misread @melifaro 's comment.

IFT_L2VLAN check is still needed, as ether_output_frame() is also called by if_vlan and the ifp->if_type == IFT_L2VLAN. It is introduced in 47c39432b11a (Unbreak VLANs after r337943) by @np.

sys/net/if_ethersubr.c
475

pf uses vlan_set_pcp(), so it adds an MTAG_8021Q/MTAG_8021Q_PCP_OUT mbuf tag with the desired PCP.

ether_8021q_frame() looks for that, as does ether_do_pcp(), so I'd expect this to work fine with pf.

zlei marked an inline comment as done.
zlei added inline comments.
sys/net/if_ethersubr.c
475

Tested with pf, looks good so far.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 6 2023, 10:17 AM
This revision was automatically updated to reflect the committed changes.