Re: [PATCH v3] af_packet: Handle outgoing VLAN packets without hardware offloading

[Willem de Bruijn]

On Mon, May 27, 2024 at 11:40 PM Chengen Du <chengen.du@xxxxxxxxxxxxx> wrote:
>
> Hi Willem,
>
> Thank you for your suggestions on the patch.
> However, there are some parts I am not familiar with, and I would appreciate more detailed information from your side.

Please respond with plain-text email. This message did not make it to
the list. Also no top posting.

https://docs.kernel.org/process/submitting-patches.html
https://subspace.kernel.org/etiquette.html

> > > @@ -2457,7 +2465,8 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> > >       sll->sll_halen = dev_parse_header(skb, sll->sll_addr);
> > >       sll->sll_family = AF_PACKET;
> > >       sll->sll_hatype = dev->type;
> > > -     sll->sll_protocol = skb->protocol;
> > > +     sll->sll_protocol = (skb->protocol == htons(ETH_P_8021Q)) ?
> > > +             vlan_eth_hdr(skb)->h_vlan_encapsulated_proto : skb->protocol;
> >
> > In SOCK_RAW mode, the VLAN tag will be present, so should be returned.
>
> Based on libpcap's handling, the SLL may not be used in SOCK_RAW mode.

The kernel fills in the sockaddr_ll fields in tpacket_rcv for both
SOCK_RAW and SOCK_DGRAM. Libpcap already can use both SOCK_RAW and
SOCK_DGRAM. And constructs the sll2_header pseudo header that tcpdump
sees itself, in pcap_handle_packet_mmap.

> Do you recommend evaluating the mode and maintaining the original logic in SOCK_RAW mode,
> or should we use the same logic for both SOCK_DGRAM and SOCK_RAW modes?

I suggest keeping as is for SOCK_RAW, as returning data that starts at
a VLAN header together with skb->protocol of ETH_P_IPV6 would be just
as confusing as the inverse that we do today on SOCK_DGRAM.

> >
> > I'm concerned about returning a different value between SOCK_RAW and
> > SOCK_DGRAM. But don't immediately see a better option. And for
> > SOCK_DGRAM this approach is indistinguishable from the result on a
> > device with hardware offload, so is acceptable.
> >
> > This test for ETH_P_8021Q ignores the QinQ stacked VLAN case. When
> > fixing VLAN encap, both variants should be addressed at the same time.
> > Note that ETH_P_8021AD is included in the eth_type_vlan test you call
> > above.
>
> In patch 1, the eth_type_vlan() function is used to determine if we need to set the sll_protocol to the VLAN-encapsulated protocol, which includes both ETH_P_8021Q and ETH_P_8021AD.
> You mentioned previously that we might want the true network protocol instead of the inner VLAN tag in the QinQ case (which means 802.1ad?).
> I believe I may have misunderstood your point.

I mean that if SOCK_DGRAM strips all VLAN headers to return the data
from the start of the true network header, then skb->protocol should
return that network protocol.

With vlan stacking, your patch currently returns ETH_P_8021Q.

See the packet formats in
https://en.wikipedia.org/wiki/IEEE_802.1ad#Frame_format if you're
confused about how stacking works.

> Could you please confirm if both ETH_P_8021Q and ETH_P_8021AD should use the VLAN-encapsulated protocol when VLAN hardware offloading is unavailable?
> Or are there other aspects that this judgment does not handle correctly?