Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO

[Hangbin Liu <liuhangbin@gmail.com>]

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?

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.

Please pardon me if I didn't make something clear.
Let's me know what do you think.

Thanks
Hangbin