netdev - Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YmIOLBihyeLy+PCS@Laptop-X1>
Date:   Fri, 22 Apr 2022 10:08:44 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     netdev@...r.kernel.org, "Michael S . Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maxim Mikityanskiy <maximmi@...lanox.com>,
        virtualization@...ts.linux-foundation.org,
        Balazs Nemeth <bnemeth@...hat.com>,
        Mike Pattrick <mpattric@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET
 SOCK_RAW GSO

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