On Mon, Jan 14, 2019 at 8:20 AM Maxim Mikityanskiy <maxi...@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 <maxi...@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.
