On Thu, Jul 27, 2023 at 01:13:10PM +0200, Jesper Dangaard Brouer wrote:

On 27/07/2023 11.30, Paolo Abeni wrote:

quoted

On Thu, 2023-07-27 at 14:07 +0800, Jason Wang wrote:

quoted

On Thu, Jul 27, 2023 at 8:27 AM David Ahern [off-list ref] wrote:

quoted

On 7/26/23 1:37 PM, David Ahern wrote:

quoted

On 7/26/23 3:02 AM, Jesper Dangaard Brouer wrote:

quoted

Cc. John and Ahern

On 26/07/2023 04.09, Jason Wang wrote:

quoted

On Tue, Jul 25, 2023 at 11:54 PM Andrew Kanner
[off-list ref] wrote:

quoted

Syzkaller reported the following issue:
=======================================
Too BIG xdp->frame_sz = 131072

Is this a contiguous physical memory allocation?

131072 bytes equal order 5 page.

Looking at tun.c code I cannot find a code path that could create
order-5 skb->data, but only SKB with order-0 fragments.  But I guess it
is the netif_receive_generic_xdp() what will realloc to make this linear
(via skb_linearize())

get_tun_user is passed an iov_iter with a single segment of 65007
total_len. The alloc_skb path is hit with an align size of only 64. That
is insufficient for XDP so the netif_receive_generic_xdp hits the
pskb_expand_head path. Something is off in the math in
netif_receive_generic_xdp resulting in the skb markers being off. That
causes bpf_prog_run_generic_xdp to compute the wrong frame_sz.

BTW, it is pskb_expand_head that turns it from a 64kB to a 128 kB
allocation. But the 128kB part is not relevant to the "bug" here really.

True, it is another "bug"/unexpected-behavior that SKB gets reallocated
to be 128KiB. We should likely solve this in another patch.

quoted

quoted

quoted

The warn on getting tripped in bpf_xdp_adjust_tail is because xdp
generic path is skb based and can have a frame_sz > 4kB. That's what the
splat is about.

Agree, that the warn condition should be changed, even removed.
It is interesting that this warn caught this unexpected-behavior of
expanding to 128KiB.

quoted

quoted

Other possibility:

tun_can_build_skb() doesn't count XDP_PACKET_HEADROOM this may end up
with producing a frame_sz which is greater than PAGE_SIZE as well in
tun_build_skb().

True, and the way I read the tun_build_skb() code, via
skb_page_frag_refill(),
it can produce an SKB with data size (buflen) upto order-3 = 32KiB
(SKB_FRAG_PAGE_ORDER).

Thus, the existing check in tun_can_build_skb() for PAGE_SIZE can/should be
relaxed?
(Please correct me as I don't fully understand tun_get_user() code)

quoted

quoted

And rethink this patch, it looks wrong since it basically drops all
packets whose buflen is greater than PAGE_SIZE since it can't fall
back to tun_alloc_skb().

I agree, this is why I reacted, as this version of the patch could
potentially cause issues and packet drops.

quoted

quoted

quoted

Perhaps the solution is to remove the WARN_ON.

Yes, that is what I'm asking if this warning still makes sense in V1.

I understand the consensus is solving the issue by changing/removing
the WARN_ON() in XDP. I think it makes sense, I guess the same warn can
be reached via packet socket xmit on veth or similar topology.

Yes, we can completely remove this check.  The original intend was to
catch cases where XDP drivers have not been updated to use xdp.frame_sz,
but that is not longer a concern (since xdp_init_buff).

It was added (by me) in commit:
 - c8741e2bfe87 ("xdp: Allow bpf_xdp_adjust_tail() to grow packet size")
   - v5.8-rc1
 - as part of merge 5cc5924d8315

I'm sure it is safe to remove since commit:
 - 43b5169d8355 ("net, xdp: Introduce xdp_init_buff utility routine")
   - v5.12-rc1

where we introduced xdp_init_buff() helper, which all XDP driver use today.
Question is what "Fixes:" tag should the patch have?

To Andrew, will you
 (1) send a new patch that removes this check instead?
 (2) have cycles to investigate why the unexpected-behavior of
expanding to 128KiB happens?

--Jesper

Thanks, everyone.

If we summarize the discussion - there are 3 issues here:
1. tun_can_build_skb() doesn't count XDP_PACKET_HEADROOM (minor and
   most trivial)
2. WARN_ON_ONCE from net/core/filter.c, which may be too strict / not
   needed at all.
3. strange behaviour with reallocationg SKB (65007 -> 131072)

I can check these issues. I have to dive a little deeper with 2-3,
most likely with kgdb and syzkaller repro. But seems this is not
somewhat urgent and lives quite a long time without being noticed.

BTW: Attached the ftrace logs using the original syzkaller repro
(starting with tun_get_user()). They answer Jesper's question about
contiguous physical memory allocation (kmem_cache_alloc_node() /
kmalloc_reserve()). But I'll check it one more time before submitting
a new PATCH V4 or another patch / patch series.

-- 
Andrew Kanner