net: Remove vlan metadata on pcp / vlan encap

[zlei]

Summary

For oubound traffic, the flag M_VLANTAG is set to 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) and
igc(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
Fixes: 868aabb4708d Add IP(V6)_VLAN_PCP to set 802.1 priority per-flow
Discussed with: kp
MFC after: 1 week

Test Plan

cxl0 <---> clx1

# ifconfig cxl0 inet 192.168.2.1/24

# ifconfig cxl1 -vlanhwtag
# ifconfig cxl1 pcp 3
# ifconfig cxl1 inet 192.0.2.2/24
# ping -C5 -c1 192.0.2.1

[kp]

Arguably there's a bug in the cxl0 driver, because we've disabled hardware vlan handling, and the driver is still looking at the M_VLANTAG tag to add a vlan header.

We're not likely to 'fix' all drivers to address this though, so it does make sense to clear the flags here too.

sys/net/if_ethersubr.c

1470

I'm not entirely sure that we can assume m_pkthdr here. I think it ought to be there, but we should either check for M_PKTHDR, or add an M_ASSERTPKTHDR() to make sure.

[zlei]

In D39499#899270, @kp wrote:

Arguably there's a bug in the cxl0 driver, because we've disabled hardware vlan handling, and the driver is still looking at the M_VLANTAG tag to add a vlan header.

I think we should make the contract clearly.

IIUC, on outbound path, if a mbuf has M_VLANTAG, it means let the hardware / driver (if they are capable and enabled) insert VLAN header.
For the inbound path, hardwares / drivers set M_VLANTAG on mbuf, if they're able and have extract the VLAN info (priority, tag).

I see a lot of drivers rely on M_VLANTAG and process VLAN info specially. Then I trend to think they are doing right rather than bugs.

We're not likely to 'fix' all drivers to address this though, so it does make sense to clear the flags here too.

[kp]

In D39499#899281, @zlei wrote:

In D39499#899270, @kp wrote:

Arguably there's a bug in the cxl0 driver, because we've disabled hardware vlan handling, and the driver is still looking at the M_VLANTAG tag to add a vlan header.

I see a lot of drivers rely on M_VLANTAG and process VLAN info specially. Then I trend to think they are doing right rather than bugs.

Hence the 'arguably'. The interface flags indicate the hardware doesn't do vlan (because it's been disabled, in this case), so it's surprising that the driver still inserts the vlan header.

Either way, this is an improvement. All this patch needs is a check for M_PKTHDR or an M_ASSERTPKTHDR() to be ready.

[zlei]

In D39499#899374, @kp wrote:

In D39499#899281, @zlei wrote:

In D39499#899270, @kp wrote:

Arguably there's a bug in the cxl0 driver, because we've disabled hardware vlan handling, and the driver is still looking at the M_VLANTAG tag to add a vlan header.

I see a lot of drivers rely on M_VLANTAG and process VLAN info specially. Then I trend to think they are doing right rather than bugs.

Hence the 'arguably'. The interface flags indicate the hardware doesn't do vlan (because it's been disabled, in this case), so it's surprising that the driver still inserts the vlan header.

Either way, this is an improvement. All this patch needs is a check for M_PKTHDR or an M_ASSERTPKTHDR() to be ready.

I see. So the hardware / driver should check both ifp->if_capenable & IFCAP_VLAN_HWTAGGING and m->m_flags & M_VLANTAG , right ?

[kp]

In D39499#899377, @zlei wrote:

I see. So the hardware / driver should check both ifp->if_capenable & IFCAP_VLAN_HWTAGGING and m->m_flags & M_VLANTAG , right ?

That's what I'd have expected, but clearly that's not the case for at least one driver. It makes more sense to apply this patch than to try to change every single driver to match my expectations though.

[zlei]

Check M_VLANTAG flag before removing it.

[kp]

sys/net/if_ethersubr.c

1469

Why check for M_VLANTAG and not M_PKTHDR?

It seems fine to me to unconditionally clear the vlan information, but we need to check if there's a pkthdr (so check M_PKTHDR) before we use (*mp)->m_pkthdr.

[zlei]

@melifaro I've taken some fixes from D39536 to this one.

[zlei]

sys/net/if_ethersubr.c

1470

It is sufficient to mask off the flag M_VLANTAG, although left some garbage in m->m_pkthdr.ether_vtag .

[kp]

sys/net/if_ethersubr.c

451

Is there a reason to check for M_VLANTAG rather than unconditionally clearing it?

[zlei]

sys/net/if_ethersubr.c

451

Either should be fine.

Maybe it will have some benefit to unconditionally clear it.

1. Simpler code
2. Less branch (less predictions when CPU execute)

[kp]

sys/net/if_ethersubr.c

451

I don't object to either version, with perhaps a small preference for the unconditional clear.

[zlei]

sys/net/if_ethersubr.c

451

2. Less branch (less predictions when CPU execute)

My previous assumption is wrong.

A simple test which clang 14 and gcc 12.3, it shows that modern compilers are smart enough, they can optimize such logic and opt out the condition ;)