if_epair: also remove vlan metadata from packets

[kp]

Summary

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

PR: 270736
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")

[bz]

Will that mean that one has to create VLAN interfaces on top of a base interface and then bridge individual vlans over various epairs into vnet jails? Does that even work with bridges and vlans that way?

[kp]

In D39482#898787, @bz wrote:

Will that mean that one has to create VLAN interfaces on top of a base interface and then bridge individual vlans over various epairs into vnet jails? Does that even work with bridges and vlans that way?

I don't quite follow.

Note that we're removing vlan metadata from the mbuf, not vlan tags in the Ethernet packet.

D39483 illustrates the problem this fixes.

[markj]

sys/net/if_epair.c

145

It looks like this function already assumes M_PKTHDR is set?

[bz]

In D39482#898808, @kp wrote:

In D39482#898787, @bz wrote:

Will that mean that one has to create VLAN interfaces on top of a base interface and then bridge individual vlans over various epairs into vnet jails? Does that even work with bridges and vlans that way?

I don't quite follow.

Note that we're removing vlan metadata from the mbuf, not vlan tags in the Ethernet packet.

Oh doh. Sorry, maybe make the first line of the commit message more detailed like this.

[kp]

sys/net/if_epair.c

145

Ah yes. It will always be called from a path where that's already asserted, but I'll add another one in this function so next time I'm here I might notice I don't need the check.

[kp]

• don't check for M_PKTHDR

[markj]

sys/net/if_epair.c

145

I'm not sure why it's useful to have separate epair_clear_mbuf() and epair_prepare_mbuf() functions, but it doesn't hurt.

[zlei]

For epair(4), I think it can land as is.

Further more, I would proposal to handle the removal of M_VLANTAG in tx side of net stack, instead of in driver stack.

bool
ether_8021q_frame(struct mbuf **mp, struct ifnet *ife, struct ifnet *p,
    struct ether_8021q_tag *qtag)
{
...
if ((p->if_capenable & IFCAP_VLAN_HWTAGGING) &&
	    (qtag->proto == ETHERTYPE_VLAN)) {
		(*mp)->m_pkthdr.ether_vtag = tag;
		(*mp)->m_flags |= M_VLANTAG;
	} else {
		*mp = ether_vlanencap_proto(*mp, tag, qtag->proto);
		if (*mp == NULL) {
			if_printf(ife, "unable to prepend 802.1Q header");
			return (false);
		}
+++         (*mp)->m_flags &= ~M_VLANTAG;
+++         (*mp)->m_pkthdr.ether_vtag = 0;
	}
}

Some tests with ifconfig re(4) -vlanhwtag pcp 3 shows the same symptom with epair(4). I'll test this approach and report later.

This approach however has larger impact, it will affect all drivers (with vlanhwtag off).

[kp]

In D39482#899185, @zlei wrote:

For epair(4), I think it can land as is.

Further more, I would proposal to handle the removal of M_VLANTAG in tx side of net stack, instead of in driver stack.

bool
ether_8021q_frame(struct mbuf **mp, struct ifnet *ife, struct ifnet *p,
    struct ether_8021q_tag *qtag)
{
...
if ((p->if_capenable & IFCAP_VLAN_HWTAGGING) &&
	    (qtag->proto == ETHERTYPE_VLAN)) {
		(*mp)->m_pkthdr.ether_vtag = tag;
		(*mp)->m_flags |= M_VLANTAG;
	} else {
		*mp = ether_vlanencap_proto(*mp, tag, qtag->proto);
		if (*mp == NULL) {
			if_printf(ife, "unable to prepend 802.1Q header");
			return (false);
		}
+++         (*mp)->m_flags &= ~M_VLANTAG;
+++         (*mp)->m_pkthdr.ether_vtag = 0;
	}
}

That was my initial version of this fix, but my understanding was (based on tcpdump on both ends of the epair) that the problem occurred on the rx side.

Some tests with ifconfig re(4) -vlanhwtag pcp 3 shows the same symptom with epair(4). I'll test this approach and report later.

That's surprising. If it also occurs in that setup I must be misinterpreting what I've seen, and this fix is insufficient.