Message-ID: <AM6PR05MB5879FDF57EC44EC4C836B0C2D1B80@AM6PR05MB5879.eurprd05.prod.outlook.com>
Date:   Fri, 21 Dec 2018 08:29:34 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     "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>
CC:     Eran Ben Elisha <eranbe@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: [RFC] AF_PACKET transport_offset fix

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.

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.

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