[bpf-next,v2,4/4] bpf: more lenient bpf_skb_net_shrink() with BPF_F_ADJ_ROOM_FIXED_GSO

Commit Message

Maciej Żenczykowski June 17, 2021, 12:09 a.m. UTC

From: Maciej Żenczykowski <[e-mail]>

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.

Cc: Daniel Borkmann <[e-mail]>
Cc: Willem de Bruijn <[e-mail]>
Signed-off-by: Maciej Żenczykowski <[e-mail]>
---
 net/core/filter.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Daniel Borkmann June 24, 2021, 2:05 p.m. UTC | #1

On 6/17/21 2:09 AM, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <[e-mail]>
> 
> 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

Maciej Żenczykowski June 24, 2021, 5:13 p.m. UTC | #2

On Thu, Jun 24, 2021 at 7:05 AM Daniel Borkmann <[e-mail]> wrote:
>
> On 6/17/21 2:09 AM, Maciej Żenczykowski wrote:
> > From: Maciej Żenczykowski <[e-mail]>
> >
> > 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

Patch

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;