Eric Dumazet wrote:
> With IFF_NAPI_FRAGS, tun_napi_alloc_frags() gets its skb from
> napi_get_frags(). If GRO merged the previous frame with
> GRO_MERGED_FREE, that skb was recycled by napi_reuse_skb(), which does
> not reset skb->protocol set by napi_frags_skb() for the previous frame.
>
> On the IFF_TAP path, tun_get_user() calls tun_vnet_hdr_tnl_to_skb()
> before eth_type_trans(), so the virtio header checks see the protocol
> of the previous frame: the outer protocol check of UDP tunnel GSO
> packets rejects a valid frame when the two ethertypes differ, and
> GSO packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are dissected with
> the wrong protocol.
>
> This needs GRO to hold packets across writes, for example with
> napi_defer_hard_irqs and gro_flush_timeout set on the device.
>
> Clear skb->protocol for IFF_TAP frames, as for skbs allocated by
> tun_alloc_skb(). eth_type_trans() sets it right after.
>
> Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver")
> Reported-by: Sashiko <[email protected]>
> Closes:
> https://lore.kernel.org/netdev/[email protected]/
> Signed-off-by: Eric Dumazet <[email protected]>
Reviewed-by: Willem de Bruijn <[email protected]>
> ---
> drivers/net/tun.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 3920b3dff1e8..6b648f21018c 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1941,6 +1941,11 @@ static ssize_t tun_get_user(struct tun_struct *tun,
> struct tun_file *tfile,
> goto drop;
> }
> skb_set_network_header(skb, ETH_HLEN);
> + /* A NAPI frags skb recycled by napi_reuse_skb() keeps the
> + * protocol of a previous frame. eth_type_trans() sets it
> + * below, the virtio header checks must not see a stale one.
> + */
> + skb->protocol = 0;
This resets it for all IFF_TAP.
Would it make sense to do this in tun_napi_alloc_frags if it is only
needed for IFF_NAPI_FRAGS?
> break;
> }
>
> --
> 2.53.0
>
