From mboxrd@z Thu Jan 1 00:00:00 1970 From: Willem de Bruijn Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value Date: Mon, 14 Jan 2019 11:49:05 -0500 Message-ID: References: <20190114131841.1932-1-maximmi@mellanox.com> <20190114131841.1932-2-maximmi@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: "David S. Miller" , Saeed Mahameed , Willem de Bruijn , Jason Wang , Eric Dumazet , "netdev@vger.kernel.org" , Eran Ben Elisha , Tariq Toukan To: Maxim Mikityanskiy Return-path: Received: from mail-ed1-f68.google.com ([209.85.208.68]:42551 "EHLO mail-ed1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726673AbfANQto (ORCPT ); Mon, 14 Jan 2019 11:49:44 -0500 Received: by mail-ed1-f68.google.com with SMTP id y20so154200edw.9 for ; Mon, 14 Jan 2019 08:49:42 -0800 (PST) In-Reply-To: <20190114131841.1932-2-maximmi@mellanox.com> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Jan 14, 2019 at 8:20 AM Maxim Mikityanskiy 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 > --- > 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(-) This is a lot of code change. This would do. @@ -2434,8 +2434,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 - skb_set_transport_header(skb, offset_hint); } Though leaving an unused argument is a bit ugly. For net-next, indeed better to clean up (please mark your patchset with net or net-next, btw) > > 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); Can use skb_transport_header_was_set(). Then at least there is no need to change the function's return value. Normally, a GSO packet will also have skb_transport_header set even from packet sockets, due to skb_partial_csum_set called from virtio_net_hdr_to_skb. But I don't think we can be sure that TSO packets without checksum offload are dropped before reaching this code.