| Message ID | 20190114131841.1932-2-maximmi@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | AF_PACKET transport_offset fix | expand |
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.
> 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.
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.
> -----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.
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.
> > 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".
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".
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;
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(-)