On Mon, May 27, 2024 at 11:40 PM Chengen Du chengen.du@canonical.com 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?