On Thu, Apr 21, 2022 at 10:10 PM Hangbin Liu <liuhangbin@xxxxxxxxx> wrote: > > Hi Willem, > On Thu, Apr 21, 2022 at 10:15:16AM -0400, Willem de Bruijn wrote: > > Your approach does sound simpler than the above. Thanks for looking > > into that alternative, though. > > Sorry I have to bring this topic back. I just remembered that > tpacket_snd() already called skb_probe_transport_header() before calling > virtio_net_hdr_* functions. e.g. > > - tpacket_snd() > - tpacket_fill_skb() > - packet_parse_headers() > - skb_probe_transport_header() > - if (po->has_vnet_hdr) > - virtio_net_hdr_to_skb() > - virtio_net_hdr_set_proto() > > While for packet_snd() > > - packet_snd() > - if (has_vnet_hdr) > - virtio_net_hdr_to_skb() > - virtio_net_hdr_set_proto() > - packet_parse_headers() > - skb_probe_transport_header() > > If we split skb_probe_transport_header() from packet_parse_headers() and > move it before calling virtio_net_hdr_* function in packet_snd(). Should > we do the same for tpacket_snd(), i.e. move skb_probe_transport_header() > after the virtio_net_hdr_* function? That sounds like the inverse: "move after" instead of "move before"? But I thought the plan was to go back to your last patch which brings packet_snd in line with tpacket_snd by moving packet_parse_headers in its entirety before virtio_net_hdr_*? > I think it really doesn't matter whether calls skb_probe_transport_header() > before or after virtio_net_hdr_* functions if we could set the skb->protocol > and network header correctly. Because > > - skb_probe_transport_header() > - skb_flow_dissect_flow_keys_basic() > - __skb_flow_dissect() > > In __skb_flow_dissect() > ``` > * @data: raw buffer pointer to the packet, if NULL use skb->data > * @proto: protocol for which to get the flow, if @data is NULL use skb->protocol > * @nhoff: network header offset, if @data is NULL use skb_network_offset(skb) > * @hlen: packet header length, if @data is NULL use skb_headlen(skb) > ``` > > So when data is NULL, we need to make sure the protocol, network header offset, > packet header length are correct. > > Before this patch, the VLAN packet network header offset is incorrect when calls > skb_probe_transport_header(). After the fix, this issue should be gone > and we can call skb_probe_transport_header() safely. > > So my conclusion is. There is no need to split packet_parse_headers(). Move > packet_parse_headers() before calling virtio_net_hdr_* function in packet_snd() > should be safe. Ack. Sorry if my last response was not entirely clear on this point. > Please pardon me if I didn't make something clear. > Let's me know what do you think. > > Thanks > Hangbin _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
