> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.ker...@gmail.com>
> Sent: 21 February, 2019 19:28
> To: Maxim Mikityanskiy <maxi...@mellanox.com>
> Cc: David S. Miller <da...@davemloft.net>; Saeed Mahameed
> <sae...@mellanox.com>; Willem de Bruijn <will...@google.com>; Jason Wang
> <jasow...@redhat.com>; Eric Dumazet <eduma...@google.com>;
> netdev@vger.kernel.org; Eran Ben Elisha <era...@mellanox.com>; Tariq Toukan
> <tar...@mellanox.com>
> Subject: Re: [PATCH net-next v2 1/7] net: Don't set transport offset to
> invalid value
>
> On Thu, Feb 21, 2019 at 7:40 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>
>
> qdisc_pkt_len_init also expects skb_transport_header(skb) to always be
> set for gso packets.
>
> Once net is merged into net-next, commit d5be7f632bad ("net: validate
This commit is already in net-next, isn't it?
> untrusted gso packets without csum offload") will ensure that packets
> that fail flow dissection do not make it into the stack. But we have
> to skip dissection in some cases, like tun [1].
OK, got you. However, is everything OK with patch [1]? It fixes false
positives, when a packet was dropped because network_header had not been
set yet for dissection to succeed, but what about evil packets that have
no network_offset at the moment of calling virtio_net_hdr_to_skb? Why
are all of them considered valid?
> I think we need to add a check in qdisc_pkt_len_init to skip the gso
> size estimation branch if !skb_transport_header_was_set(skb).
>
> Otherwise this patch set looks good to me. To avoid resubmitting
> everything we can fix up the qdisc_pkt_len_init in a follow-up, in
> which case I'm happy to add my Acked-by to this series.
I'll add this check and submit the patch soon. Thanks for reviewing!
> [1] http://patchwork.ozlabs.org/patch/1044429/
