在 2021/6/25 下午4:51, David Woodhouse 写道:

On Fri, 2021-06-25 at 15:41 +0800, Jason Wang wrote:

quoted

在 2021/6/24 下午8:30, David Woodhouse 写道:

quoted

From: David Woodhouse <redacted>

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().

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().

Looks like a bug.

quoted

    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.

We had already checked this in tun_get_user() before:

          if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP) {
                  align += NET_IP_ALIGN;
                  if (unlikely(len < ETH_HLEN ||
                               (gso.hdr_len && tun16_to_cpu(tun,
gso.hdr_len) < ETH_HLEN)))
                          return -EINVAL;
          }

We'd checked skb->len, but that doesn't mean we had a full Ethernet
header *linearly* at skb->data, does it?

The linear room is guaranteed through either:

1) tun_build_skb()

or

2) tun_alloc_skb()

For the basic tun_get_user() case I suppose we copy_from_user() into a
single linear skb anyway, even if userspace had fragment it and used
writev(). So we *are* probably safe there?

I'm sure we *can* contrive a proof that it's safe for that case, if we
must. But I think we should *need* that proof, if we're going to bypass
the check. And I wasn't comfortable touching that code without it.

We should also have a fairly good reason... it isn't clear to me *why*
we're bothering to avoid the check. Is it so slow, even in the case
where there's nothing to be done?

For a linear skb, the inline pskb_may_pull() is going to immediately
return true because ETH_HLEN < skb_headlen(skb), isn't it? Why optimise
*that* away?

Willem, was there a reason you made that conditional in the first
place?

If we're going to continue to *not* check on the XDP path, we similarly
need a proof that it can't be fragmented. And also a reason to bother
with the "optimisation", of course.

For XDP path, we simply need to add a length check since the packet is 
always a linear memory.

Thanks