Re: [PATCH] net: fix udp gso skb_segment after pull from frag_list

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Shiming Cheng wrote:
> Commit 3382a1ed7f77 ("net: fix udp gso skb_segment after  pull from
> frag_list")
> if gso_type is not SKB_GSO_FRAGLIST but skb->head_frag is zero,

What codepath triggers this scenario?

We should make sure that the fix covers all such instances. Likely
instances of where some module in the datapath, like a BPF program,
modifies a valid skb into one that is not safe to pass to skb_segment.

I don't fully understand yet that skb->head_frag == 0 is the only
such condition in scope.

> then detected invalid geometry in frag_list skbs and call
> skb_segment. But some packets with modified geometry can also hit
> bugs in that code. Instead, linearize all these packets that fail
> the basic invariants on gso fraglist skbs. That is more robust.
> call stack information, see below.
> 
> Valid SKB_GSO_FRAGLIST skbs
> - consist of two or more segments
> - the head_skb holds the protocol headers plus first gso_size
> - one or more frag_list skbs hold exactly one segment
> - all but the last must be gso_size
> 
> Optional datapath hooks such as NAT and BPF (bpf_skb_pull_data) can
> modify fraglist skbs, breaking these invariants.
> 
> In extreme cases they pull one part of data into skb linear. For UDP,
> this  causes three payloads with lengths of (11,11,10) bytes were
> pulled tail to become (12,10,10) bytes.
> 
> The skbs no longer meets the above SKB_GSO_FRAGLIST conditions because
> payload was pulled into head_skb, it needs to be linearized before pass
> to regular skb_segment.

Most of this commit message duplicates the text in commit 3382a1ed7f77
("net: fix udp gso skb_segment after  pull from frag_list"). And
somewhat garbles it, as in the first sentence.

But this is a different datapath, not related to SKB_GSO_FRAGLIST.
So the fixes tag is also incorrect. The blamed commit fixes an issue
with fraglist GRO. This new issue is with skbs that have a fraglist,
but not one created with that feature. (the naming is confusing, but
fraglist-gro is only one use of the skb frag_list).

>   skb_segment+0xcd0/0xd14
>   __udp_gso_segment+0x334/0x5f4
>   udp4_ufo_fragment+0x118/0x15c
>   inet_gso_segment+0x164/0x338
>   skb_mac_gso_segment+0xc4/0x13c
>   __skb_gso_segment+0xc4/0x124
>   validate_xmit_skb+0x9c/0x2c0
>   validate_xmit_skb_list+0x4c/0x80
>   sch_direct_xmit+0x70/0x404
>   __dev_queue_xmit+0x64c/0xe5c
>   neigh_resolve_output+0x178/0x1c4
>   ip_finish_output2+0x37c/0x47c
>   __ip_finish_output+0x194/0x240
>   ip_finish_output+0x20/0xf4
>   ip_output+0x100/0x1a0
>   NF_HOOK+0xc4/0x16c
>   ip_forward+0x314/0x32c
>   ip_rcv+0x90/0x118
>   __netif_receive_skb+0x74/0x124
>   process_backlog+0xe8/0x1a4
>   __napi_poll+0x5c/0x1f8
>   net_rx_action+0x154/0x314
>   handle_softirqs+0x154/0x4b8
> 
>   [118.376811] [C201134] rxq0_pus: [name:bug&]kernel BUG at net/core/skbuff.c:4278!
>   [118.376829] [C201134] rxq0_pus: [name:traps&]Internal error: Oops - BUG: 00000000f2000800 [#1]
>   [118.470774] [C201134] rxq0_pus: [name:mrdump&]Kernel Offset: 0x178cc00000 from 0xffffffc008000000
>   [118.470810] [C201134] rxq0_pus: [name:mrdump&]PHYS_OFFSET: 0x40000000
>   [118.470827] [C201134] rxq0_pus: [name:mrdump&]pstate: 60400005 (nZCv daif +PAN -UAO)
>   [118.470848] [C201134] rxq0_pus: [name:mrdump&]pc : [0xffffffd79598aefc] skb_segment+0xcd0/0xd14
>   [118.470900] [C201134] rxq0_pus: [name:mrdump&]lr : [0xffffffd79598a5e8] skb_segment+0x3bc/0xd14
>   [118.470928] [C201134] rxq0_pus: [name:mrdump&]sp : ffffffc008013770
> 
> Fixes: 3382a1ed7f77 ("net: fix udp gso skb_segment after pull from frag_list")
> Signed-off-by: Shiming Cheng <shiming.cheng@mediatek.com>
> ---
>  net/ipv4/udp_offload.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 19d0b5b09ffa..606d9ce8c98e 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -535,6 +535,12 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  			uh->check = ~udp_v4_check(gso_skb->len,
>  						  ip_hdr(gso_skb)->saddr,
>  						  ip_hdr(gso_skb)->daddr, 0);
> +	} else if (skb_shinfo(gso_skb)->frag_list && gso_skb->head_frag == 0) {
> +		if (skb_pagelen(gso_skb) - sizeof(*uh) != skb_shinfo(gso_skb)->gso_size) {
> +			ret = __skb_linearize(gso_skb);
> +			if (ret)
> +				return ERR_PTR(ret);
> +		}
>  	}
>  
>  	skb_pull(gso_skb, sizeof(*uh));
> -- 
> 2.45.2
>