netdev - Re: BUG:af_packet fails to TX TSO frames

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: <872e2660-01fd-82ae-7a8f-16535dbc91d8@cambridgegreys.com>
Date:   Thu, 12 Oct 2017 20:55:31 +0100
From:   Anton Ivanov <anton.ivanov@...bridgegreys.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: BUG:af_packet fails to TX TSO frames

[snip]

>> There is no requirement to bind to ETH_P_IP either and most code examples
>> going back more than 10 years to the days of TCP Illustrated use ALL.
> For send only sockets, it is often advised to pass 0 as protocol to
> socket(), so as to avoid having to spend cycles on packet reception
> at all.

Normally, it would have been a single rw socket. Thankfully, I built
into the UML vector IO patchset the ability to use different fds for
read and write per virtual NIC.

Coming back to "normally" - as an application developer I would have
expected a rw socket to work and not to need 2+ sockets for this. Having
to work around at that level is IMHO a bit over the top.

>
> Commit c72219b75fde ("packet: infer protocol from ethernet header
> if unset") explicitly added logic to infer skb->protocol for this common
> case of sockets, if using rings.
I will look into it.

>
>> I just did the v6 test - if you bind with ETH_P_IP GSO on TCPv6 is broken
>> and returns NOBUF and vice versa.
> Given that skb->protocol is set from proto, that is indeed not expected to work.
>
>>>>> I think what is happening is that this value is taken into account when
>>>>> looking at "what should I use to segment it with" in skb_mac_gso_segment
>>>>> which is invoked at the end of the verification chain which starts in
>>>>> packet_direct_xmit in af_packet.c
>>>> packet_snd sets skb->protocol based on the protocol that the packet
>>>> socket is bound to. Binding to ETH_P_IP is the right choice here.
>>> To avoid having to open multiple sockets for different protocols,
>>> sockaddr_ll can also be passed in the msg_name argument on
>>> each call.
>>
>> Does not work for vnet headers - it honors what you bound with. I tried to
>> bind with ETH_ALL and pass ETH_P_IP as an arg and it ENOBUF-ed
> Odd. The code for looking up proto in packet_snd looks fairly straightforward:

I will double-check it tomorrow and send you a pull request for the
updated test application.

>
>         /*
>          *      Get and verify the address.
>          */
>
>         if (likely(saddr == NULL)) {
>                 dev     = packet_cached_dev_get(po);
>                 proto   = po->num;
>                 addr    = NULL;
>         } else {
>                 err = -EINVAL;
>                 if (msg->msg_namelen < sizeof(struct sockaddr_ll))
>                         goto out;
>                 if (msg->msg_namelen < (saddr->sll_halen +
> offsetof(struct sockaddr_ll, sll_addr)))
>                         goto out;
>                 proto   = saddr->sll_protocol;
>                 addr    = saddr->sll_addr;
>                 dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
>         }
>
> followed later by
>
>         skb->protocol = proto;
>         skb->dev = dev;
>


-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661