[net] net/packet: fix incoming receive for L3 devices without visible hard header

[net] net/packet: fix incoming receive for L3 devices without visible hard header

[Eyal Birger]

In the patchset merged by commit b9fcf0a0d826
("Merge branch 'support-AF_PACKET-for-layer-3-devices'") L3 devices which
did not have header_ops were given one for the purpose of protocol parsing
on af_packet transmit path.

That change made af_packet receive path regard these devices as having a
visible L3 header and therefore aligned incoming skb->data to point to the
skb's mac_header. Some devices, such as ipip, xfrmi, and others, do not
reset their mac_header prior to ingress and therefore their incoming
packets became malformed.

Ideally these devices would reset their mac headers, or af_packet would be
able to rely on dev->hard_header_len being 0 for such cases, but it seems
this is not the case.

Fix by changing af_packet RX ll visibility criteria to include the
existence of a '.create()' header operation, which is used when creating
a device hard header - via dev_hard_header() - by upper layers, and does
not exist in these L3 devices.

Fixes: b9fcf0a0d826 ("Merge branch 'support-AF_PACKET-for-layer-3-devices'")
Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
---
 net/packet/af_packet.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index cefbd50c1090..a241059fd536 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -93,8 +93,8 @@
 
 /*
    Assumptions:
-   - If the device has no dev->header_ops, there is no LL header visible
-     above the device. In this case, its hard_header_len should be 0.
+   - If the device has no dev->header_ops->create, there is no LL header
+     visible above the device. In this case, its hard_header_len should be 0.
      The device may prepend its own header internally. In this case, its
      needed_headroom should be set to the space needed for it to add its
      internal header.
@@ -108,21 +108,21 @@
 On receive:
 -----------
 
-Incoming, dev->header_ops != NULL
+Incoming, dev->header_ops != NULL && dev->header_ops->create != NULL
    mac_header -> ll header
    data       -> data
 
-Outgoing, dev->header_ops != NULL
+Outgoing, dev->header_ops != NULL && dev->header_ops->create != NULL
    mac_header -> ll header
    data       -> ll header
 
-Incoming, dev->header_ops == NULL
+Incoming, dev->header_ops == NULL || dev->header_ops->create == NULL
    mac_header -> data
      However drivers often make it point to the ll header.
      This is incorrect because the ll header should be invisible to us.
    data       -> data
 
-Outgoing, dev->header_ops == NULL
+Outgoing, dev->header_ops == NULL || dev->header_ops->create == NULL
    mac_header -> data. ll header is invisible to us.
    data       -> data
 
@@ -272,6 +272,18 @@ static bool packet_use_direct_xmit(const struct packet_sock *po)
 	return po->xmit == packet_direct_xmit;
 }
 
+static bool packet_ll_header_rcv_visible(const struct net_device *dev)
+{
+	/* The device has an explicit notion of ll header,
+	 * exported to higher levels
+	 *
+	 * Otherwise, the device hides details of its frame
+	 * structure, so that corresponding packet head is
+	 * never delivered to user.
+	 */
+	return dev->header_ops && dev->header_ops->create;
+}
+
 static u16 packet_pick_tx_queue(struct sk_buff *skb)
 {
 	struct net_device *dev = skb->dev;
@@ -2069,7 +2081,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	skb->dev = dev;
 
-	if (dev->header_ops) {
+	if (packet_ll_header_rcv_visible(dev)) {
 		/* The device has an explicit notion of ll header,
 		 * exported to higher levels.
 		 *
@@ -2198,7 +2210,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		goto drop;
 
-	if (dev->header_ops) {
+	if (packet_ll_header_rcv_visible(dev)) {
 		if (sk->sk_type != SOCK_DGRAM)
 			skb_push(skb, skb->data - skb_mac_header(skb));
 		else if (skb->pkt_type == PACKET_OUTGOING) {
-- 
2.25.1

Re: [net] net/packet: fix incoming receive for L3 devices without visible hard header

[Willem de Bruijn]

On Thu, Nov 19, 2020 at 10:05 PM Eyal Birger <eyal.birger@gmail.com> wrote:
>
> In the patchset merged by commit b9fcf0a0d826
> ("Merge branch 'support-AF_PACKET-for-layer-3-devices'") L3 devices which
> did not have header_ops were given one for the purpose of protocol parsing
> on af_packet transmit path.
>
> That change made af_packet receive path regard these devices as having a
> visible L3 header and therefore aligned incoming skb->data to point to the
> skb's mac_header. Some devices, such as ipip, xfrmi, and others, do not
> reset their mac_header prior to ingress and therefore their incoming
> packets became malformed.
>
> Ideally these devices would reset their mac headers, or af_packet would be
> able to rely on dev->hard_header_len being 0 for such cases, but it seems
> this is not the case.
>
> Fix by changing af_packet RX ll visibility criteria to include the
> existence of a '.create()' header operation, which is used when creating
> a device hard header - via dev_hard_header() - by upper layers, and does
> not exist in these L3 devices.
>
> Fixes: b9fcf0a0d826 ("Merge branch 'support-AF_PACKET-for-layer-3-devices'")
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>

Thanks for the fix. This makes sense to me.

Recent discussions on this point also agreed that whether or not
headers are exposed to code above the device driver depends
on dev->hard_header_len and dev->header_ops->create (and
the two have to be consistent with one another).

dev->header_ops->parse_protocol is a best effort approach to
infer a protocol in cases where the caller did not specify it. But
as best effort, its existence or absence does not define the
device header, so testing only header_ops != NULL is insufficient.

> ---
>  net/packet/af_packet.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index cefbd50c1090..a241059fd536 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -93,8 +93,8 @@
>
>  /*
>     Assumptions:
> -   - If the device has no dev->header_ops, there is no LL header visible
> -     above the device. In this case, its hard_header_len should be 0.
> +   - If the device has no dev->header_ops->create, there is no LL header
> +     visible above the device. In this case, its hard_header_len should be 0.
>       The device may prepend its own header internally. In this case, its
>       needed_headroom should be set to the space needed for it to add its
>       internal header.
> @@ -108,21 +108,21 @@
>  On receive:
>  -----------
>
> -Incoming, dev->header_ops != NULL
> +Incoming, dev->header_ops != NULL && dev->header_ops->create != NULL
>     mac_header -> ll header
>     data       -> data
>
> -Outgoing, dev->header_ops != NULL
> +Outgoing, dev->header_ops != NULL && dev->header_ops->create != NULL
>     mac_header -> ll header
>     data       -> ll header
>
> -Incoming, dev->header_ops == NULL
> +Incoming, dev->header_ops == NULL || dev->header_ops->create == NULL
>     mac_header -> data
>       However drivers often make it point to the ll header.
>       This is incorrect because the ll header should be invisible to us.
>     data       -> data
>
> -Outgoing, dev->header_ops == NULL
> +Outgoing, dev->header_ops == NULL || dev->header_ops->create == NULL
>     mac_header -> data. ll header is invisible to us.
>     data       -> data
>
> @@ -272,6 +272,18 @@ static bool packet_use_direct_xmit(const struct packet_sock *po)
>         return po->xmit == packet_direct_xmit;
>  }
>
> +static bool packet_ll_header_rcv_visible(const struct net_device *dev)
> +{
> +       /* The device has an explicit notion of ll header,
> +        * exported to higher levels
> +        *
> +        * Otherwise, the device hides details of its frame
> +        * structure, so that corresponding packet head is
> +        * never delivered to user.
> +        */
> +       return dev->header_ops && dev->header_ops->create;
> +}
> +

Perhaps a dev_has_header(..) in include/linux/netdevice.h

And then the same in the Incoming/Outgoing comments above.