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: <CA+FuTSfzcAUXrxzbLd-MPctTyLu8USJQ4gvsqPBfLpA+svYMYA@mail.gmail.com>
Date:   Fri, 22 Apr 2022 17:39:48 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        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

On Thu, Apr 21, 2022 at 10:10 PM Hangbin Liu <liuhangbin@...il.com> 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