[PATCH net-next v2 0/7] AF_PACKET transport_offset fix

[PATCH net-next v2 0/7] AF_PACKET transport_offset fix

[Maxim Mikityanskiy]

This patch series contains the implementation of the RFC that was posted
on this mailing list previously:
https://www.spinics.net/lists/netdev/msg541709.html

It fixes having incorrect skb->transport_header values in cases when
dissect fails. Having correct values set by the kernel fixes mlx5
operation and allows to remove some unnecessary code flows in mlx5.

v2 changes:

- Rebase against the fresh net-next.
- Don't return bool from skb_probe_transport_header (and don't rename
  the function).
- WARN_ON_ONCE and error path in case of GSO without the L4 header.

Maxim Mikityanskiy (7):
  net: Don't set transport offset to invalid value
  net: Introduce parse_protocol header_ops callback
  net/ethernet: Add parse_protocol header_ops support
  net/packet: Ask driver for protocol if not provided by user
  net/packet: Remove redundant skb->protocol set
  net/mlx5e: Remove the wrong assumption about transport offset
  net/mlx5e: Trust kernel regarding transport offset

 .../net/ethernet/mellanox/mlx5/core/en_tx.c   | 15 ++---------
 drivers/net/tap.c                             |  4 +--
 drivers/net/tun.c                             |  4 +--
 drivers/net/xen-netback/netback.c             | 15 ++++++++---
 include/linux/etherdevice.h                   |  1 +
 include/linux/netdevice.h                     | 10 +++++++
 include/linux/skbuff.h                        |  5 +---
 include/linux/virtio_net.h                    |  2 +-
 net/ethernet/eth.c                            | 13 ++++++++++
 net/packet/af_packet.c                        | 26 +++++++++----------
 10 files changed, 56 insertions(+), 39 deletions(-)

-- 
2.19.1

[PATCH net-next v2 1/7] net: Don't set transport offset to invalid value

[Maxim Mikityanskiy]

If the socket was created with socket(AF_PACKET, SOCK_RAW, 0),
skb->protocol will be unset, __skb_flow_dissect() will fail, and
skb_probe_transport_header() will fall back to the offset_hint, making
the resulting skb_transport_offset incorrect.

If, however, there is no transport header in the packet,
transport_header shouldn't be set to an arbitrary value.

Fix it by leaving the transport offset unset if it couldn't be found, to
be explicit rather than to fill it with some wrong value. It changes the
behavior, but if some code relied on the old behavior, it would be
broken anyway, as the old one is incorrect.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 drivers/net/tap.c                 |  4 ++--
 drivers/net/tun.c                 |  4 ++--
 drivers/net/xen-netback/netback.c | 15 ++++++++++++---
 include/linux/skbuff.h            |  5 +----
 include/linux/virtio_net.h        |  2 +-
 net/packet/af_packet.c            |  6 +++---
 6 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index c0b52e48f0e6..2ea9b4976f4a 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -712,7 +712,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 			goto err_kfree;
 	}
 
-	skb_probe_transport_header(skb, ETH_HLEN);
+	skb_probe_transport_header(skb);
 
 	/* Move network header to the right position for VLAN tagged packets */
 	if ((skb->protocol == htons(ETH_P_8021Q) ||
@@ -1187,7 +1187,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
 	tap = rcu_dereference(q->tap);
 	if (tap) {
 		skb->dev = tap->dev;
-		skb_probe_transport_header(skb, ETH_HLEN);
+		skb_probe_transport_header(skb);
 		dev_queue_xmit(skb);
 	} else {
 		kfree_skb(skb);
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index fed298c0cb39..80bff1b4ec17 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1929,7 +1929,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	}
 
 	skb_reset_network_header(skb);
-	skb_probe_transport_header(skb, 0);
+	skb_probe_transport_header(skb);
 
 	if (skb_xdp) {
 		struct bpf_prog *xdp_prog;
@@ -2482,7 +2482,7 @@ static int tun_xdp_one(struct tun_struct *tun,
 
 	skb->protocol = eth_type_trans(skb, tun->dev);
 	skb_reset_network_header(skb);
-	skb_probe_transport_header(skb, 0);
+	skb_probe_transport_header(skb);
 
 	if (skb_xdp) {
 		err = do_xdp_generic(xdp_prog, skb);
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 80aae3a32c2a..c801a832851c 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1169,15 +1169,24 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
 			continue;
 		}
 
-		skb_probe_transport_header(skb, 0);
+		skb_probe_transport_header(skb);
 
 		/* If the packet is GSO then we will have just set up the
 		 * transport header offset in checksum_setup so it's now
 		 * straightforward to calculate gso_segs.
 		 */
 		if (skb_is_gso(skb)) {
-			int mss = skb_shinfo(skb)->gso_size;
-			int hdrlen = skb_transport_header(skb) -
+			int mss, hdrlen;
+
+			/* GSO implies having the L4 header. */
+			WARN_ON_ONCE(!skb_transport_header_was_set(skb));
+			if (unlikely(!skb_transport_header_was_set(skb))) {
+				kfree_skb(skb);
+				continue;
+			}
+
+			mss = skb_shinfo(skb)->gso_size;
+			hdrlen = skb_transport_header(skb) -
 				skb_mac_header(skb) +
 				tcp_hdrlen(skb);
 
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 2069fb90a559..27beb549ffbe 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2429,8 +2429,7 @@ static inline void skb_pop_mac_header(struct sk_buff *skb)
 	skb->mac_header = skb->network_header;
 }
 
-static inline void skb_probe_transport_header(struct sk_buff *skb,
-					      const int offset_hint)
+static inline void skb_probe_transport_header(struct sk_buff *skb)
 {
 	struct flow_keys_basic keys;
 
@@ -2439,8 +2438,6 @@ static inline void skb_probe_transport_header(struct sk_buff *skb,
 
 	if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0))
 		skb_set_transport_header(skb, keys.control.thoff);
-	else if (offset_hint >= 0)
-		skb_set_transport_header(skb, offset_hint);
 }
 
 static inline void skb_mac_header_rebuild(struct sk_buff *skb)
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 71f2394abbf7..6728bf581e98 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -62,7 +62,7 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 		 * probe and drop if does not match one of the above types.
 		 */
 		if (gso_type) {
-			skb_probe_transport_header(skb, -1);
+			skb_probe_transport_header(skb);
 			if (!skb_transport_header_was_set(skb))
 				return -EINVAL;
 		}
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 1cd1d83a4be0..6afd6369d19e 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1970,7 +1970,7 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg,
 	if (unlikely(extra_len == 4))
 		skb->no_fcs = 1;
 
-	skb_probe_transport_header(skb, 0);
+	skb_probe_transport_header(skb);
 
 	dev_queue_xmit(skb);
 	rcu_read_unlock();
@@ -2519,7 +2519,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		len = ((to_write > len_max) ? len_max : to_write);
 	}
 
-	skb_probe_transport_header(skb, 0);
+	skb_probe_transport_header(skb);
 
 	return tp_len;
 }
@@ -2925,7 +2925,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		virtio_net_hdr_set_proto(skb, &vnet_hdr);
 	}
 
-	skb_probe_transport_header(skb, reserve);
+	skb_probe_transport_header(skb);
 
 	if (unlikely(extra_len == 4))
 		skb->no_fcs = 1;
-- 
2.19.1

[PATCH net-next v2 2/7] net: Introduce parse_protocol header_ops callback

[Maxim Mikityanskiy]

Introduce a new optional header_ops callback called parse_protocol and a
wrapper function dev_parse_header_protocol, similar to dev_parse_header.

The new callback's purpose is to extract the protocol number from the L2
header, the format of which is known to the driver, but not to the upper
layers of the stack.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 include/linux/netdevice.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index aab4d9f6613d..6997f62cb6a0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -274,6 +274,7 @@ struct header_ops {
 				const struct net_device *dev,
 				const unsigned char *haddr);
 	bool	(*validate)(const char *ll_header, unsigned int len);
+	__be16	(*parse_protocol)(const struct sk_buff *skb);
 };
 
 /* These flag bits are private to the generic network queueing
@@ -2939,6 +2940,15 @@ static inline int dev_parse_header(const struct sk_buff *skb,
 	return dev->header_ops->parse(skb, haddr);
 }
 
+static inline __be16 dev_parse_header_protocol(const struct sk_buff *skb)
+{
+	const struct net_device *dev = skb->dev;
+
+	if (!dev->header_ops || !dev->header_ops->parse_protocol)
+		return 0;
+	return dev->header_ops->parse_protocol(skb);
+}
+
 /* ll_header must have at least hard_header_len allocated */
 static inline bool dev_validate_header(const struct net_device *dev,
 				       char *ll_header, int len)
-- 
2.19.1

[PATCH net-next v2 3/7] net/ethernet: Add parse_protocol header_ops support

[Maxim Mikityanskiy]

The previous commit introduced parse_protocol callback which should
extract the protocol number from the L2 header. Make all Ethernet
devices support it.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 include/linux/etherdevice.h |  1 +
 net/ethernet/eth.c          | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 2c0af7b00715..e2f3b21cd72a 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -44,6 +44,7 @@ int eth_header_cache(const struct neighbour *neigh, struct hh_cache *hh,
 		     __be16 type);
 void eth_header_cache_update(struct hh_cache *hh, const struct net_device *dev,
 			     const unsigned char *haddr);
+__be16 eth_header_parse_protocol(const struct sk_buff *skb);
 int eth_prepare_mac_addr_change(struct net_device *dev, void *p);
 void eth_commit_mac_addr_change(struct net_device *dev, void *p);
 int eth_mac_addr(struct net_device *dev, void *p);
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 4c520110b04f..f7a3d7a171c7 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -264,6 +264,18 @@ void eth_header_cache_update(struct hh_cache *hh,
 }
 EXPORT_SYMBOL(eth_header_cache_update);
 
+/**
+ * eth_header_parser_protocol - extract protocol from L2 header
+ * @skb: packet to extract protocol from
+ */
+__be16 eth_header_parse_protocol(const struct sk_buff *skb)
+{
+	const struct ethhdr *eth = eth_hdr(skb);
+
+	return eth->h_proto;
+}
+EXPORT_SYMBOL(eth_header_parse_protocol);
+
 /**
  * eth_prepare_mac_addr_change - prepare for mac change
  * @dev: network device
@@ -346,6 +358,7 @@ const struct header_ops eth_header_ops ____cacheline_aligned = {
 	.parse		= eth_header_parse,
 	.cache		= eth_header_cache,
 	.cache_update	= eth_header_cache_update,
+	.parse_protocol	= eth_header_parse_protocol,
 };
 
 /**
-- 
2.19.1

[PATCH net-next v2 4/7] net/packet: Ask driver for protocol if not provided by user

[Maxim Mikityanskiy]

If a socket was created with socket(AF_PACKET, SOCK_RAW, 0), the
protocol number is unavailable. Try to ask the driver to extract it from
the L2 header in order for skb_try_probe_transport_header to succeed.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 net/packet/af_packet.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 6afd6369d19e..cac4c1a3f807 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1850,6 +1850,15 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
 	return 0;
 }
 
+static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
+{
+	if (!skb->protocol && sock->type == SOCK_RAW) {
+		skb_reset_mac_header(skb);
+		skb->protocol = dev_parse_header_protocol(skb);
+	}
+
+	skb_probe_transport_header(skb);
+}
 
 /*
  *	Output a raw packet to a device layer. This bypasses all the other
@@ -1970,7 +1979,7 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg,
 	if (unlikely(extra_len == 4))
 		skb->no_fcs = 1;
 
-	skb_probe_transport_header(skb);
+	packet_parse_headers(skb, sock);
 
 	dev_queue_xmit(skb);
 	rcu_read_unlock();
@@ -2519,7 +2528,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		len = ((to_write > len_max) ? len_max : to_write);
 	}
 
-	skb_probe_transport_header(skb);
+	packet_parse_headers(skb, sock);
 
 	return tp_len;
 }
@@ -2925,7 +2934,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		virtio_net_hdr_set_proto(skb, &vnet_hdr);
 	}
 
-	skb_probe_transport_header(skb);
+	packet_parse_headers(skb, sock);
 
 	if (unlikely(extra_len == 4))
 		skb->no_fcs = 1;
-- 
2.19.1

[PATCH net-next v2 5/7] net/packet: Remove redundant skb->protocol set

[Maxim Mikityanskiy]

c72219b75f introduced tpacket_set_protocol that parses the Ethernet L2
header and sets skb->protocol if it's unset. It is no longer needed
since the introduction of packet_parse_headers. In case of SOCK_RAW and
unset skb->protocol, packet_parse_headers asks the driver to tell the
protocol number, and it's implemented for all Ethernet devices. As the
old function supported only Ethernet, no functionality is lost.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 net/packet/af_packet.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index cac4c1a3f807..8376bc1c1508 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2413,15 +2413,6 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
 	sock_wfree(skb);
 }
 
-static void tpacket_set_protocol(const struct net_device *dev,
-				 struct sk_buff *skb)
-{
-	if (dev->type == ARPHRD_ETHER) {
-		skb_reset_mac_header(skb);
-		skb->protocol = eth_hdr(skb)->h_proto;
-	}
-}
-
 static int __packet_snd_vnet_parse(struct virtio_net_hdr *vnet_hdr, size_t len)
 {
 	if ((vnet_hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
@@ -2492,8 +2483,6 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 			return err;
 		if (!dev_validate_header(dev, skb->data, hdrlen))
 			return -EINVAL;
-		if (!skb->protocol)
-			tpacket_set_protocol(dev, skb);
 
 		data += hdrlen;
 		to_write -= hdrlen;
-- 
2.19.1

[PATCH net-next v2 6/7] net/mlx5e: Remove the wrong assumption about transport offset

[Maxim Mikityanskiy]

skb_transport_offset() == 0 is not a special value. The only special
value is when skb->transport_header is ~0U, and it's checked by
skb_transport_header_was_set().

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index c1334a8ac8f3..6ca834702306 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -172,15 +172,8 @@ static inline u16 mlx5e_calc_min_inline(enum mlx5_inline_modes mode,
 			hlen += VLAN_HLEN;
 		break;
 	case MLX5_INLINE_MODE_IP:
-		/* When transport header is set to zero, it means no transport
-		 * header. When transport header is set to 0xff's, it means
-		 * transport header wasn't set.
-		 */
-		if (skb_transport_offset(skb)) {
-			hlen = mlx5e_skb_l3_header_offset(skb);
-			break;
-		}
-		/* fall through */
+		hlen = mlx5e_skb_l3_header_offset(skb);
+		break;
 	case MLX5_INLINE_MODE_L2:
 	default:
 		hlen = mlx5e_skb_l2_header_offset(skb);
-- 
2.19.1

[PATCH net-next v2 7/7] net/mlx5e: Trust kernel regarding transport offset

[Maxim Mikityanskiy]

After AF_PACKET is fixed to calculate the transport header offset
correctly, trust the value set by the kernel. If the offset wasn't set,
it means there is no transport header in the packet.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index 6ca834702306..e7aae45a01f8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -148,12 +148,8 @@ static inline int mlx5e_skb_l2_header_offset(struct sk_buff *skb)
 
 static inline int mlx5e_skb_l3_header_offset(struct sk_buff *skb)
 {
-	struct flow_keys keys;
-
 	if (skb_transport_header_was_set(skb))
 		return skb_transport_offset(skb);
-	else if (skb_flow_dissect_flow_keys(skb, &keys, 0))
-		return keys.control.thoff;
 	else
 		return mlx5e_skb_l2_header_offset(skb);
 }
-- 
2.19.1

Re: [PATCH net-next v2 1/7] net: Don't set transport offset to invalid value

[Willem de Bruijn]

On Thu, Feb 21, 2019 at 7:40 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> If the socket was created with socket(AF_PACKET, SOCK_RAW, 0),
> skb->protocol will be unset, __skb_flow_dissect() will fail, and
> skb_probe_transport_header() will fall back to the offset_hint, making
> the resulting skb_transport_offset incorrect.
>
> If, however, there is no transport header in the packet,
> transport_header shouldn't be set to an arbitrary value.
>
> Fix it by leaving the transport offset unset if it couldn't be found, to
> be explicit rather than to fill it with some wrong value. It changes the
> behavior, but if some code relied on the old behavior, it would be
> broken anyway, as the old one is incorrect.
>
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>

qdisc_pkt_len_init also expects skb_transport_header(skb) to always be
set for gso packets.

Once net is merged into net-next, commit d5be7f632bad ("net: validate
untrusted gso packets without csum offload") will ensure that packets
that fail flow dissection do not make it into the stack. But we have
to skip dissection in some cases, like tun [1].

I think we need to add a check in qdisc_pkt_len_init to skip the gso
size estimation branch if !skb_transport_header_was_set(skb).

Otherwise this patch set looks good to me. To avoid resubmitting
everything we can fix up the qdisc_pkt_len_init in a follow-up, in
which case I'm happy to add my Acked-by to this series.

[1] http://patchwork.ozlabs.org/patch/1044429/

RE: [PATCH net-next v2 1/7] net: Don't set transport offset to invalid value

[Maxim Mikityanskiy]

> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Sent: 21 February, 2019 19:28
> To: Maxim Mikityanskiy <maximmi@mellanox.com>
> Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> <tariqt@mellanox.com>
> Subject: Re: [PATCH net-next v2 1/7] net: Don't set transport offset to
> invalid value
> 
> On Thu, Feb 21, 2019 at 7:40 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> wrote:
> >
> > If the socket was created with socket(AF_PACKET, SOCK_RAW, 0),
> > skb->protocol will be unset, __skb_flow_dissect() will fail, and
> > skb_probe_transport_header() will fall back to the offset_hint, making
> > the resulting skb_transport_offset incorrect.
> >
> > If, however, there is no transport header in the packet,
> > transport_header shouldn't be set to an arbitrary value.
> >
> > Fix it by leaving the transport offset unset if it couldn't be found, to
> > be explicit rather than to fill it with some wrong value. It changes the
> > behavior, but if some code relied on the old behavior, it would be
> > broken anyway, as the old one is incorrect.
> >
> > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> 
> qdisc_pkt_len_init also expects skb_transport_header(skb) to always be
> set for gso packets.
> 
> Once net is merged into net-next, commit d5be7f632bad ("net: validate

This commit is already in net-next, isn't it?

> untrusted gso packets without csum offload") will ensure that packets
> that fail flow dissection do not make it into the stack. But we have
> to skip dissection in some cases, like tun [1].

OK, got you. However, is everything OK with patch [1]? It fixes false
positives, when a packet was dropped because network_header had not been
set yet for dissection to succeed, but what about evil packets that have
no network_offset at the moment of calling virtio_net_hdr_to_skb? Why
are all of them considered valid?

> I think we need to add a check in qdisc_pkt_len_init to skip the gso
> size estimation branch if !skb_transport_header_was_set(skb).
> 
> Otherwise this patch set looks good to me. To avoid resubmitting
> everything we can fix up the qdisc_pkt_len_init in a follow-up, in
> which case I'm happy to add my Acked-by to this series.

I'll add this check and submit the patch soon. Thanks for reviewing!

> [1] http://patchwork.ozlabs.org/patch/1044429/

[PATCH net-next] net: Skip GSO length estimation if transport header is not set

[Maxim Mikityanskiy]

qdisc_pkt_len_init expects transport_header to be set for GSO packets.
Patch [1] skips transport_header validation for GSO packets that don't
have network_header set at the moment of calling virtio_net_hdr_to_skb,
and allows them to pass into the stack. After patch [2] no placeholder
value is assigned to transport_header if dissection fails, so this patch
adds a check to the place where the value of transport_header is used.

[1] https://patchwork.ozlabs.org/patch/1044429/
[2] https://patchwork.ozlabs.org/patch/1046122/

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index a3d13f5e2bfc..8a0da95ff4cc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3421,7 +3421,7 @@ static void qdisc_pkt_len_init(struct sk_buff *skb)
 	/* To get more precise estimation of bytes sent on wire,
 	 * we add to pkt_len the headers size of all segments
 	 */
-	if (shinfo->gso_size)  {
+	if (shinfo->gso_size && skb_transport_header_was_set(skb)) {
 		unsigned int hdr_len;
 		u16 gso_segs = shinfo->gso_segs;
 
-- 
2.19.1

Re: [PATCH net-next v2 1/7] net: Don't set transport offset to invalid value

[Willem de Bruijn]

On Fri, Feb 22, 2019 at 7:30 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > -----Original Message-----
> > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Sent: 21 February, 2019 19:28
> > To: Maxim Mikityanskiy <maximmi@mellanox.com>
> > Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> > <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> > <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> > netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> > <tariqt@mellanox.com>
> > Subject: Re: [PATCH net-next v2 1/7] net: Don't set transport offset to
> > invalid value
> >
> > On Thu, Feb 21, 2019 at 7:40 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> > wrote:
> > >
> > > If the socket was created with socket(AF_PACKET, SOCK_RAW, 0),
> > > skb->protocol will be unset, __skb_flow_dissect() will fail, and
> > > skb_probe_transport_header() will fall back to the offset_hint, making
> > > the resulting skb_transport_offset incorrect.
> > >
> > > If, however, there is no transport header in the packet,
> > > transport_header shouldn't be set to an arbitrary value.
> > >
> > > Fix it by leaving the transport offset unset if it couldn't be found, to
> > > be explicit rather than to fill it with some wrong value. It changes the
> > > behavior, but if some code relied on the old behavior, it would be
> > > broken anyway, as the old one is incorrect.
> > >
> > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> >
> > qdisc_pkt_len_init also expects skb_transport_header(skb) to always be
> > set for gso packets.
> >
> > Once net is merged into net-next, commit d5be7f632bad ("net: validate
>
> This commit is already in net-next, isn't it?

It is now yes.

> > untrusted gso packets without csum offload") will ensure that packets
> > that fail flow dissection do not make it into the stack. But we have
> > to skip dissection in some cases, like tun [1].
>
> OK, got you. However, is everything OK with patch [1]? It fixes false
> positives, when a packet was dropped because network_header had not been
> set yet for dissection to succeed, but what about evil packets that have
> no network_offset at the moment of calling virtio_net_hdr_to_skb? Why
> are all of them considered valid?

They are not considered valid. But it is the cost of avoiding false
positives. We cannot break legitimate traffic.

The two patches as is restrict a large class of illegal packets
from packet sockets. Which always set network header, so
this cannot be worked around.

Extending these checks to tun will require an independent
additional fix.

> > I think we need to add a check in qdisc_pkt_len_init to skip the gso
> > size estimation branch if !skb_transport_header_was_set(skb).
> >
> > Otherwise this patch set looks good to me. To avoid resubmitting
> > everything we can fix up the qdisc_pkt_len_init in a follow-up, in
> > which case I'm happy to add my Acked-by to this series.
>
> I'll add this check and submit the patch soon. Thanks for reviewing!
>
> > [1] http://patchwork.ozlabs.org/patch/1044429/

Looks good, thanks.

Re: [PATCH net-next] net: Skip GSO length estimation if transport header is not set

[Willem de Bruijn]

On Fri, Feb 22, 2019 at 7:56 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> qdisc_pkt_len_init expects transport_header to be set for GSO packets.
> Patch [1] skips transport_header validation for GSO packets that don't
> have network_header set at the moment of calling virtio_net_hdr_to_skb,
> and allows them to pass into the stack. After patch [2] no placeholder
> value is assigned to transport_header if dissection fails, so this patch
> adds a check to the place where the value of transport_header is used.
>
> [1] https://patchwork.ozlabs.org/patch/1044429/
> [2] https://patchwork.ozlabs.org/patch/1046122/
>
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>

Acked-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next v2 0/7] AF_PACKET transport_offset fix

[Willem de Bruijn]

On Thu, Feb 21, 2019 at 7:40 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> This patch series contains the implementation of the RFC that was posted
> on this mailing list previously:
> https://www.spinics.net/lists/netdev/msg541709.html
>
> It fixes having incorrect skb->transport_header values in cases when
> dissect fails. Having correct values set by the kernel fixes mlx5
> operation and allows to remove some unnecessary code flows in mlx5.
>
> v2 changes:
>
> - Rebase against the fresh net-next.
> - Don't return bool from skb_probe_transport_header (and don't rename
>   the function).
> - WARN_ON_ONCE and error path in case of GSO without the L4 header.
>
> Maxim Mikityanskiy (7):
>   net: Don't set transport offset to invalid value
>   net: Introduce parse_protocol header_ops callback
>   net/ethernet: Add parse_protocol header_ops support
>   net/packet: Ask driver for protocol if not provided by user
>   net/packet: Remove redundant skb->protocol set
>   net/mlx5e: Remove the wrong assumption about transport offset
>   net/mlx5e: Trust kernel regarding transport offset

For the series: Acked-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next v2 0/7] AF_PACKET transport_offset fix

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Fri, 22 Feb 2019 09:20:01 -0500

> On Thu, Feb 21, 2019 at 7:40 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>>
>> This patch series contains the implementation of the RFC that was posted
>> on this mailing list previously:
>> https://www.spinics.net/lists/netdev/msg541709.html
>>
>> It fixes having incorrect skb->transport_header values in cases when
>> dissect fails. Having correct values set by the kernel fixes mlx5
>> operation and allows to remove some unnecessary code flows in mlx5.
>>
>> v2 changes:
>>
>> - Rebase against the fresh net-next.
>> - Don't return bool from skb_probe_transport_header (and don't rename
>>   the function).
>> - WARN_ON_ONCE and error path in case of GSO without the L4 header.
>>
>> Maxim Mikityanskiy (7):
>>   net: Don't set transport offset to invalid value
>>   net: Introduce parse_protocol header_ops callback
>>   net/ethernet: Add parse_protocol header_ops support
>>   net/packet: Ask driver for protocol if not provided by user
>>   net/packet: Remove redundant skb->protocol set
>>   net/mlx5e: Remove the wrong assumption about transport offset
>>   net/mlx5e: Trust kernel regarding transport offset
> 
> For the series: Acked-by: Willem de Bruijn <willemb@google.com>

Series applied, thanks.

Re: [PATCH net-next] net: Skip GSO length estimation if transport header is not set

[David Miller]

From: Maxim Mikityanskiy <maximmi@mellanox.com>
Date: Fri, 22 Feb 2019 12:55:22 +0000

> qdisc_pkt_len_init expects transport_header to be set for GSO packets.
> Patch [1] skips transport_header validation for GSO packets that don't
> have network_header set at the moment of calling virtio_net_hdr_to_skb,
> and allows them to pass into the stack. After patch [2] no placeholder
> value is assigned to transport_header if dissection fails, so this patch
> adds a check to the place where the value of transport_header is used.
> 
> [1] https://patchwork.ozlabs.org/patch/1044429/
> [2] https://patchwork.ozlabs.org/patch/1046122/
> 
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>

Applied, thanks Maxim.