All of lore.kernel.org
 help / color / mirror / Atom feed
From: Maxim Mikityanskiy <maximmi@mellanox.com>
To: "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>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Eran Ben Elisha <eranbe@mellanox.com>,
	Tariq Toukan <tariqt@mellanox.com>,
	Maxim Mikityanskiy <maximmi@mellanox.com>
Subject: [PATCH 1/7] net: Don't set transport offset to invalid value
Date: Mon, 14 Jan 2019 13:18:56 +0000	[thread overview]
Message-ID: <20190114131841.1932-2-maximmi@mellanox.com> (raw)
In-Reply-To: <20190114131841.1932-1-maximmi@mellanox.com>

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 | 19 +++++++++++--------
 include/linux/skbuff.h            | 14 +++++++-------
 net/packet/af_packet.c            |  6 +++---
 5 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 443b2694130c..a35b44b13a34 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_try_probe_transport_header(skb);
 
 	/* Move network header to the right position for VLAN tagged packets */
 	if ((skb->protocol == htons(ETH_P_8021Q) ||
@@ -1177,7 +1177,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
 			goto err_kfree;
 	}
 
-	skb_probe_transport_header(skb, ETH_HLEN);
+	skb_try_probe_transport_header(skb);
 
 	/* Move network header to the right position for VLAN tagged packets */
 	if ((skb->protocol == htons(ETH_P_8021Q) ||
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index a4fdad475594..f73a156379e6 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1927,7 +1927,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_try_probe_transport_header(skb);
 
 	if (skb_xdp) {
 		struct bpf_prog *xdp_prog;
@@ -2480,7 +2480,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_try_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..b49b6e56ca47 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1105,6 +1105,7 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
 		struct xen_netif_tx_request *txp;
 		u16 pending_idx;
 		unsigned data_len;
+		bool th_set;
 
 		pending_idx = XENVIF_TX_CB(skb)->pending_idx;
 		txp = &queue->pending_tx_info[pending_idx].req;
@@ -1169,20 +1170,22 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
 			continue;
 		}
 
-		skb_probe_transport_header(skb, 0);
+		th_set = skb_try_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) -
-				skb_mac_header(skb) +
-				tcp_hdrlen(skb);
-
-			skb_shinfo(skb)->gso_segs =
-				DIV_ROUND_UP(skb->len - hdrlen, mss);
+			if (likely(th_set)) { /* GSO implies having L4 header */
+				int mss = skb_shinfo(skb)->gso_size;
+				int hdrlen = skb_transport_header(skb) -
+					skb_mac_header(skb) +
+					tcp_hdrlen(skb);
+
+				skb_shinfo(skb)->gso_segs =
+					DIV_ROUND_UP(skb->len - hdrlen, mss);
+			}
 		}
 
 		queue->stats.rx_bytes += skb->len;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 2a57a365c711..b3aa2be1afb3 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2424,18 +2424,18 @@ 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 bool skb_try_probe_transport_header(struct sk_buff *skb)
 {
 	struct flow_keys_basic keys;
 
 	if (skb_transport_header_was_set(skb))
-		return;
+		return true;
 
-	if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0))
-		skb_set_transport_header(skb, keys.control.thoff);
-	else
-		skb_set_transport_header(skb, offset_hint);
+	if (!skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0, 0))
+		return false;
+
+	skb_set_transport_header(skb, keys.control.thoff);
+	return true;
 }
 
 static inline void skb_mac_header_rebuild(struct sk_buff *skb)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index eedacdebcd4c..8fc76e68777a 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_try_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_try_probe_transport_header(skb);
 
 	return tp_len;
 }
@@ -2924,7 +2924,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_try_probe_transport_header(skb);
 
 	if (unlikely(extra_len == 4))
 		skb->no_fcs = 1;
-- 
2.19.1

  reply	other threads:[~2019-01-14 13:19 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-14 13:18 [PATCH 0/7] AF_PACKET transport_offset fix Maxim Mikityanskiy
2019-01-14 13:18 ` Maxim Mikityanskiy [this message]
2019-01-14 16:49   ` [PATCH 1/7] net: Don't set transport offset to invalid value Willem de Bruijn
2019-01-17  9:10     ` Maxim Mikityanskiy
2019-01-17 15:16       ` Willem de Bruijn
2019-01-23 10:16         ` Maxim Mikityanskiy
2019-01-23 14:11           ` Willem de Bruijn
2019-01-24  9:47             ` Maxim Mikityanskiy
2019-01-24 14:19               ` Willem de Bruijn
2019-01-14 13:18 ` [PATCH 2/7] net: Introduce parse_protocol header_ops callback Maxim Mikityanskiy
2019-01-14 13:19 ` [PATCH 3/7] net/ethernet: Add parse_protocol header_ops support Maxim Mikityanskiy
2019-01-23 14:14   ` Willem de Bruijn
2019-01-24  9:47     ` Maxim Mikityanskiy
2019-01-24 14:21       ` Willem de Bruijn
2019-01-25  8:51         ` Maxim Mikityanskiy
2019-01-25 13:52           ` Willem de Bruijn
2019-01-14 13:19 ` [PATCH 4/7] net/packet: Ask driver for protocol if not provided by user Maxim Mikityanskiy
2019-01-14 16:52   ` Willem de Bruijn
2019-01-15 16:32     ` Willem de Bruijn
2019-01-17  9:10       ` Maxim Mikityanskiy
2019-01-17 15:41         ` Willem de Bruijn
2019-01-14 13:19 ` [PATCH 5/7] net/packet: Remove redundant skb->protocol set Maxim Mikityanskiy
2019-01-14 13:19 ` [PATCH 6/7] net/mlx5e: Remove the wrong assumption about transport offset Maxim Mikityanskiy
2019-01-14 13:19 ` [PATCH 7/7] net/mlx5e: Trust kernel regarding " Maxim Mikityanskiy
2019-01-14 13:52 ` [PATCH 0/7] AF_PACKET transport_offset fix Maxim Mikityanskiy
2019-01-23 10:16 ` Maxim Mikityanskiy
2019-01-23 14:12   ` Willem de Bruijn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190114131841.1932-2-maximmi@mellanox.com \
    --to=maximmi@mellanox.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=eranbe@mellanox.com \
    --cc=jasowang@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=saeedm@mellanox.com \
    --cc=tariqt@mellanox.com \
    --cc=willemb@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.