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?
