Message-ID: <CAF=yD-KEW7UV0rafi02K4UtJNCJ0GkzVQDWnRGkpU+pEq8bhSw@mail.gmail.com>
Date:   Fri, 21 Dec 2018 11:36:10 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Maxim Mikityanskiy <maximmi@...lanox.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Jason Wang <jasowang@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: Re: [RFC] AF_PACKET transport_offset fix

On Fri, Dec 21, 2018 at 10:24 AM Maxim Mikityanskiy
<maximmi@...lanox.com> wrote:
>
> Hi everyone,
>
> Currently, sockets created as socket(AF_PACKET, SOCK_RAW, 0) have a problem:
> skb->transport_header is set to an incorrect value. In short, packet_snd()
> function calls skb_probe_transport_header() while skb->protocol == 0 (zero is
> taken from the third parameter of the socket() call), which causes
> __skb_flow_dissect() to fail, and skb_probe_transport_header() sets the
> transport header offset to offset_hint, which is 14 (dev->hard_header_len), when
> in reality the transport header starts much farther.
>
> Having a wrong transport header offset is not good for drivers that use this
> value. I suggest to fix it in the following way:
>
> 0. AF_PACKET has access to the net_device, which has header_ops, which includes
> parse callback. This callback helps the socket layer get the source MAC address
> from the L2 header by calling into the driver, which has knowledge about the L2
> header format. AF_PACKET uses this callback by calling dev_parse_header()
> wrapper. For Ethernet devices, header_ops are implemented by eth_header_ops, and
> parse is implemented by eth_header_parse(). This existing setup can be extended
> to make it possible for the socket layer to extract the protocol number from the
> L2 header by calling into the driver, and to prevent __skb_flow_dissect() from
> failing.

This addresses a separate problem from what to set the transport header to
on flow dissection failure. That said, it sounds fine to me. It does add an
indirect function call in the hot path, but only for sockets that do not set
sll_protocol, so this is under application control.

> 1. Extend header_ops with parse_protocol. It may be an optional callback (just
> like the existing parse), so we don't need to implement it for all kinds of MAC
> layer.
>
> 2. Create a wrapper function dev_parse_header_protocol() that will look just
> like dev_parse_header(), but call our new parse_protocol callback.
>
> 3. Create a function eth_header_parse_protocol() that will extract the protocol
> from eth_hdr, just like eth_header_parse() extracts the source MAC address.
>
> 4. Extend eth_header_ops with `.parse_protocol = eth_header_parse_protocol`. Now
> all the Ethernet drivers get the support for this feature.
>
> 5. In AF_PACKET, if skb->protocol is zero, assign `skb->protocol =
> dev_parse_header_protocol(skb)`. It may still remain zero if the driver doesn't
> support parse_protocol. In this case, the driver will get transport_header
> unset, and if it needs it, it should implement parse_protocol, which is easy.
> All Ethernet drivers will support it automatically after step 4.
>
> 6. After that, AF_PACKET calls the dissect routine. If it fails, it means that
> there is no transport header (for the drivers that implement parse_protocol). In
> this case, we don't fall back to any offset_hint, but just leave
> transport_header == ~0U, as it was before, which means it's not set. There is a
> function called skb_transport_header_was_set() that checks for this special
> value.

This sounds good to me.

With the caveat that we have to make sure that no path depends on the
transport header always being set, and that this change in
skb_probe_transport_header does not cause unintended side effects
for the other users (tun/tap/xen). But I expect that to be fine.

Optionally, as a separate follow-up patch we could add a value -2U that
denotes transport header is unset and already parsed. To avoid additional
flow dissection attempts. But this will have to be cleared on various
operations, such as cross-netns forwarding or packet mangling. So
probably not worth the effort.


> Pros:
>
> - The driver always receives the most correct information from the kernel that
> can be determined. If the kernel sets the transport header offset, it's correct.
> If not, there is no transport header.
>
> - Non-intrusive: of course, it requires adding a new callback to header_ops, but
> it's optional (if it's not provided, the driver just won't receive the transport
> header offset), automatically provided for Ethernet drivers, and the most
> important, AF_PACKET already uses header_ops, so it doesn't create additional
> relation between the socket code and drivers.
>
> I'm looking forward to hearing your feedback for this approach. Please send your
> comments, considerations, objections, ideas.
>
> Thanks,
> Max

Powered by blists - more mailing lists