| Message ID | 20210617000953.2787453-4-zenczykowski@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 6/17/21 2:09 AM, Maciej Żenczykowski wrote: > From: Maciej Żenczykowski <maze@google.com> > > This is to more closely match behaviour of bpf_skb_change_proto() > which now does not adjust gso_size, and thus thoretically supports > all gso types, and does not need to set SKB_GSO_DODGY nor reset > gso_segs to zero. > > Something similar should probably be done with bpf_skb_net_grow(), > but that code scares me. Took in all except this one, would be good to have a complete solution for both bpf_skb_net_{shrink,grow}(). If you don't have the cycles, I'll look into it. Thanks, Daniel
On Thu, Jun 24, 2021 at 7:05 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 6/17/21 2:09 AM, Maciej Żenczykowski wrote: > > From: Maciej Żenczykowski <maze@google.com> > > > > This is to more closely match behaviour of bpf_skb_change_proto() > > which now does not adjust gso_size, and thus thoretically supports > > all gso types, and does not need to set SKB_GSO_DODGY nor reset > > gso_segs to zero. > > > > Something similar should probably be done with bpf_skb_net_grow(), > > but that code scares me. > > Took in all except this one, would be good to have a complete solution for > both bpf_skb_net_{shrink,grow}(). If you don't have the cycles, I'll look > into it. > > Thanks, > Daniel I very much don't understand all the complexities of all the different encap/tunneling stuff that is handled in ..._grow(). In principle I think changing the gso_size is probably a bad idea in general, but I'm not at all sure that's a change we can make now, without breaking backward compatibility with some userspace somewhere (not Android though, we don't currently use either of these helpers yet) or causing other trouble. I'd love it if there was some truly good documentation of how all the fields/offloads in an skb interact, as I find myself constantly having to figure this out via code examination, and never feel like I really truly understand things (or perhaps some helper function that would 'validate' an skb as well formed, ideally in debug mode we could call it both before and after a bpf program mucks with things and check it still passes). I'm not sure who would be the expert here... you? Willem? Tom? someone else? As such I'll leave this up to one of you. I sent the patch for ..._shrink() because that one seemed simple enough. (I don't really understand why shrink is so much simpler than grow...) What I will try to send you is an extension to 4<->6 protocol conversion to deal with the extra 8 bytes of overhead in an ipv6 fragment (48 instead of 40 byte header converted to/from 20 byte ipv4 frag header). Though this isn't something I even have ready atm, it's just on a todo list as a relatively unimportant thing. - Maciej
diff --git a/net/core/filter.c b/net/core/filter.c index 8f05498f497e..faf2bae0309b 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3506,11 +3506,10 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff, BPF_F_ADJ_ROOM_NO_CSUM_RESET))) return -EINVAL; - if (skb_is_gso(skb) && !skb_is_gso_tcp(skb)) { - /* udp gso_size delineates datagrams, only allow if fixed */ - if (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) || - !(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) - return -ENOTSUPP; + if (skb_is_gso(skb) && + !skb_is_gso_tcp(skb) && + !(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) { + return -ENOTSUPP; } ret = skb_unclone(skb, GFP_ATOMIC); @@ -3521,12 +3520,11 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff, if (unlikely(ret < 0)) return ret; - if (skb_is_gso(skb)) { + if (skb_is_gso(skb) && !(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) { struct skb_shared_info *shinfo = skb_shinfo(skb); /* Due to header shrink, MSS can be upgraded. */ - if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) - skb_increase_gso_size(shinfo, len_diff); + skb_increase_gso_size(shinfo, len_diff); /* Header must be checked, and gso_segs recomputed. */ shinfo->gso_type |= SKB_GSO_DODGY;