[1/7] net: Don't set transport offset to invalid value
diff mbox series

Message ID 20190114131841.1932-2-maximmi@mellanox.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • AF_PACKET transport_offset fix
Related show

Commit Message

Maxim Mikityanskiy Jan. 14, 2019, 1:18 p.m. UTC
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(-)

Comments

Willem de Bruijn Jan. 14, 2019, 4:49 p.m. UTC | #1
On Mon, Jan 14, 2019 at 8:20 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>
> ---
>  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.
Maxim Mikityanskiy Jan. 17, 2019, 9:10 a.m. UTC | #2
> 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)

It's for net-next (I'll resend with the correct mark), so I'll stick
with the current implementation.

> > 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.

I suppose this comment relates to the previous one, and if we do it for
net-next, it's fine to make change I made, isn't it?

Thanks for reviewing.
Willem de Bruijn Jan. 17, 2019, 3:16 p.m. UTC | #3
On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > 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)
>
> It's for net-next (I'll resend with the correct mark), so I'll stick
> with the current implementation.

Absolutely, sounds good.

> > > 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.
>
> I suppose this comment relates to the previous one, and if we do it for
> net-next, it's fine to make change I made, isn't it?

If this is the only reason for the boolean return value, using
skb_transport_header_was_set() is more standard (I immediately know
what's happening when I read it), slightly less code change and avoids
introducing a situation where the majority of callers ignore a return
value. I think it's preferable. But these merits are certainly
debatable, so either is fine.
Maxim Mikityanskiy Jan. 23, 2019, 10:16 a.m. UTC | #4
> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Sent: 17 January, 2019 17:16
> 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 1/7] net: Don't set transport offset to invalid value
> 
> On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> wrote:
> >
> > > 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)
> >
> > It's for net-next (I'll resend with the correct mark), so I'll stick
> > with the current implementation.
> 
> Absolutely, sounds good.
> 
> > > > 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.
> >
> > I suppose this comment relates to the previous one, and if we do it for
> > net-next, it's fine to make change I made, isn't it?
> 
> If this is the only reason for the boolean return value, using
> skb_transport_header_was_set() is more standard (I immediately know
> what's happening when I read it), slightly less code change and avoids
> introducing a situation where the majority of callers ignore a return
> value. I think it's preferable. But these merits are certainly
> debatable, so either is fine.

From my side, I wanted to avoid calling skb_transport_header_was_set
twice, so I made skb_try_probe_transport_header return whether it
succeeded or not. I think "try" in the function name indicates this idea
pretty clearly. This result status is pretty useful, it just happened
that it's not needed in many places, but the general idea is that we
report this status, so if you say that my version is also good for you,
I'll leave it as is. It was just a rationale for my decision.
Willem de Bruijn Jan. 23, 2019, 2:11 p.m. UTC | #5
On Wed, Jan 23, 2019 at 5:17 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > -----Original Message-----
> > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Sent: 17 January, 2019 17:16
> > 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 1/7] net: Don't set transport offset to invalid value
> >
> > On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> > wrote:
> > >
> > > > 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)
> > >
> > > It's for net-next (I'll resend with the correct mark), so I'll stick
> > > with the current implementation.
> >
> > Absolutely, sounds good.
> >
> > > > > 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.
> > >
> > > I suppose this comment relates to the previous one, and if we do it for
> > > net-next, it's fine to make change I made, isn't it?
> >
> > If this is the only reason for the boolean return value, using
> > skb_transport_header_was_set() is more standard (I immediately know
> > what's happening when I read it), slightly less code change and avoids
> > introducing a situation where the majority of callers ignore a return
> > value. I think it's preferable. But these merits are certainly
> > debatable, so either is fine.
>
> From my side, I wanted to avoid calling skb_transport_header_was_set
> twice,  so I made skb_try_probe_transport_header return whether it
> succeeded or not. I think "try" in the function name indicates this idea
> pretty clearly. This result status is pretty useful, it just happened
> that it's not needed in many places,

Which is an indication that it's perhaps not needed.

> but the general idea is that we
> report this status, so if you say that my version is also good for you,
> I'll leave it as is. It was just a rationale for my decision.

It's fine. But please avoid the code churn in xenvif_tx_submit
with to extra indentation. This is equivalent:

-               if (skb_is_gso(skb)) {
+               if (skb_is_gso(skb) && th_set) {

More fundamentally, the code has the assumption that th_set
always holds if skb_is_gso(skb). Why add the check? This is
another example that the return value is not really needed.
Maxim Mikityanskiy Jan. 24, 2019, 9:47 a.m. UTC | #6
> > but the general idea is that we
> > report this status, so if you say that my version is also good for you,
> > I'll leave it as is. It was just a rationale for my decision.
> 
> It's fine. But please avoid the code churn in xenvif_tx_submit
> with to extra indentation. This is equivalent:
> 
> -               if (skb_is_gso(skb)) {
> +               if (skb_is_gso(skb) && th_set) {
> 
> More fundamentally, the code has the assumption that th_set
> always holds if skb_is_gso(skb). Why add the check? This is
> another example that the return value is not really needed.

What about

if (skb_is_gso(skb)) {
	BUG_ON(!skb_transport_header_was_set(skb));

?

I think it's cleaner than skipping the action here if dissect failed and
propagating a potential bug further.

> > > If this is the only reason for the boolean return value, using
> > > skb_transport_header_was_set() is more standard (I immediately know
> > > what's happening when I read it), slightly less code change and avoids
> > > introducing a situation where the majority of callers ignore a return
> > > value. I think it's preferable. But these merits are certainly
> > > debatable, so either is fine.
> >
> > From my side, I wanted to avoid calling skb_transport_header_was_set
> > twice,  so I made skb_try_probe_transport_header return whether it
> > succeeded or not. I think "try" in the function name indicates this idea
> > pretty clearly. This result status is pretty useful, it just happened
> > that it's not needed in many places,
> 
> Which is an indication that it's perhaps not needed.

Well, from the point of view of the function, it looks reasonable to
notify the caller whether the call was successful or not... You know,
many functions return error codes.

However, this case is rather special, because it turned out that we
don't actually need that error status immediately, but it can be
requested much later, and we already have skb_transport_header_was_set
for that.

So, considering these points, the return value can be removed, as all
use cases are "probe now, check later", not "probe now and handle errors
immediately".
Willem de Bruijn Jan. 24, 2019, 2:19 p.m. UTC | #7
On Thu, Jan 24, 2019 at 4:48 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > > but the general idea is that we
> > > report this status, so if you say that my version is also good for you,
> > > I'll leave it as is. It was just a rationale for my decision.
> >
> > It's fine. But please avoid the code churn in xenvif_tx_submit
> > with to extra indentation. This is equivalent:
> >
> > -               if (skb_is_gso(skb)) {
> > +               if (skb_is_gso(skb) && th_set) {
> >
> > More fundamentally, the code has the assumption that th_set
> > always holds if skb_is_gso(skb). Why add the check? This is
> > another example that the return value is not really needed.
>
> What about
>
> if (skb_is_gso(skb)) {
>         BUG_ON(!skb_transport_header_was_set(skb));
>
> ?

Good idea to validate. Please on error use WARN_ON_ONCE and free the
packet as with other errors in this function. BUG_ONs are problematic
when a path to them is found.

The other option is to stay as close to the current logic as possible
in this driver and set the mac header at offset 0 if probe fails.
Either through the return value or skb_transport_header_was_set.

> I think it's cleaner than skipping the action here if dissect failed and
> propagating a potential bug further.
>
> > > > If this is the only reason for the boolean return value, using
> > > > skb_transport_header_was_set() is more standard (I immediately know
> > > > what's happening when I read it), slightly less code change and avoids
> > > > introducing a situation where the majority of callers ignore a return
> > > > value. I think it's preferable. But these merits are certainly
> > > > debatable, so either is fine.
> > >
> > > From my side, I wanted to avoid calling skb_transport_header_was_set
> > > twice,  so I made skb_try_probe_transport_header return whether it
> > > succeeded or not. I think "try" in the function name indicates this idea
> > > pretty clearly. This result status is pretty useful, it just happened
> > > that it's not needed in many places,
> >
> > Which is an indication that it's perhaps not needed.
>
> Well, from the point of view of the function, it looks reasonable to
> notify the caller whether the call was successful or not... You know,
> many functions return error codes.
>
> However, this case is rather special, because it turned out that we
> don't actually need that error status immediately, but it can be
> requested much later, and we already have skb_transport_header_was_set
> for that.
>
> So, considering these points, the return value can be removed, as all
> use cases are "probe now, check later", not "probe now and handle errors
> immediately".

Patch
diff mbox series

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;