Re: [PATCH v3 4/5] net: tun: fix tun_xdp_one() for IFF_TUN mode

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: David Woodhouse <dwmw2@infradead.org>
Cc: netdev@vger.kernel.org, "Jason Wang" <jasowang@redhat.com>,
	"Eugenio Pérez" <eperezma@redhat.com>
Subject: Re: [PATCH v3 4/5] net: tun: fix tun_xdp_one() for IFF_TUN mode
Date: Fri, 25 Jun 2021 14:43:47 -0400	[thread overview]
Message-ID: <CA+FuTSc-8uO2wUNZH_HhcJ-+SrncyCvJ2NaASCzixpoZQ6cfpQ@mail.gmail.com> (raw)
In-Reply-To: <20210624123005.1301761-4-dwmw2@infradead.org>

On Thu, Jun 24, 2021 at 8:30 AM David Woodhouse <dwmw2@infradead.org> wrote:
>
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> In tun_get_user(), skb->protocol is either taken from the tun_pi header
> or inferred from the first byte of the packet in IFF_TUN mode, while
> eth_type_trans() is called only in the IFF_TAP mode where the payload
> is expected to be an Ethernet frame.
>
> The equivalent code path in tun_xdp_one() was unconditionally using
> eth_type_trans(), which is the wrong thing to do in IFF_TUN mode and
> corrupts packets.
>
> Pull the logic out to a separate tun_skb_set_protocol() function, and
> call it from both tun_get_user() and tun_xdp_one().

I think this should be two patches. The support for parsing pi is an
independent fix.

> XX: It is not entirely clear to me why it's OK to call eth_type_trans()
> in some cases without first checking that enough of the Ethernet header
> is linearly present by calling pskb_may_pull(). Such a check was never
> present in the tun_xdp_one() code path, and commit 96aa1b22bd6bb ("tun:
> correct header offsets in napi frags mode") deliberately added it *only*
> for the IFF_NAPI_FRAGS mode.

IFF_NAPI_FRAGS exercises napi_gro_frags, which uses the frag0
optimization where all data is in frags. The other receive paths do
not.

For the other cases, linear is guaranteed to include the link layer
header. __tun_build_skb, for instance, just allocates one big
skb->data. It is admittedly not trivial to prove this point
exhaustively for all paths.

commit 96aa1b22bd6bb restricted the new test to the frags case, to
limit the potential blast radius of a bug fix to only the code path
affected by the bug.

> I would like to see specific explanations of *why* it's ever valid and
> necessary (is it so much faster?) to skip the pskb_may_pull()

It was just not needed and that did not complicate anything until this
patch. It's fine to unconditionally check if that simplifies this
change.