Re: [bpf-next PATCH 1/2] bpf, sockmap: add skb_adjust_room to pop bytes off ingress payload

[Jakub Sitnicki <jakub@cloudflare.com>]

On Sat, Sep 26, 2020 at 06:27 AM CEST, John Fastabend wrote:
> This implements a new helper skb_adjust_room() so users can push/pop
> extra bytes from a BPF_SK_SKB_STREAM_VERDICT program.
>
> Some protocols may include headers and other information that we may
> not want to include when doing a redirect from a BPF_SK_SKB_STREAM_VERDICT
> program. One use case is to redirect TLS packets into a receive socket
> that doesn't expect TLS data. In TLS case the first 13B or so contain the
> protocol header. With KTLS the payload is decrypted so we should be able
> to redirect this to a receiving socket, but the receiving socket may not
> be expecting to receive a TLS header and discard the data. Using the
> above helper we can pop the header off and put an appropriate header on
> the payload. This allows for creating a proxy between protocols without
> extra hops through the stack or userspace.

This is useful stuff. Apart from the TLS use-case, you might want to pop
off proxy headers like PROXY v1/v2 (CC Marek):

  https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

>
> So in order to fix this case add skb_adjust_room() so users can strip the
> header. After this the user can strip the header and an unmodified receiver
> thread will work correctly when data is redirected into the ingress path
> of a sock.
>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
>  net/core/filter.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 4d8dc7a31a78..d232358f1dcd 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -76,6 +76,7 @@
>  #include <net/bpf_sk_storage.h>
>  #include <net/transp_v6.h>
>  #include <linux/btf_ids.h>
> +#include <net/tls.h>
>
>  static const struct bpf_func_proto *
>  bpf_sk_base_func_proto(enum bpf_func_id func_id);
> @@ -3218,6 +3219,53 @@ static u32 __bpf_skb_max_len(const struct sk_buff *skb)
>  			  SKB_MAX_ALLOC;
>  }
>
> +BPF_CALL_4(sk_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
> +	   u32, mode, u64, flags)
> +{
> +	unsigned int len_diff_abs = abs(len_diff);
> +	bool shrink = len_diff < 0;
> +	int ret = 0;
> +
> +	if (unlikely(flags))
> +		return -EINVAL;
> +	if (unlikely(len_diff_abs > 0xfffU))
> +		return -EFAULT;
> +
> +	if (!shrink) {
> +		unsigned int grow = len_diff;
> +
> +		ret = skb_cow(skb, grow);
> +		if (likely(!ret)) {
> +			__skb_push(skb, len_diff_abs);
> +			memset(skb->data, 0, len_diff_abs);
> +		}
> +	} else {
> +		/* skb_ensure_writable() is not needed here, as we're
> +		 * already working on an uncloned skb.
> +		 */

I'm trying to digest the above comment. What if:

static int __strp_recv(…)
{
        …
	while (eaten < orig_len) {
		/* Always clone since we will consume something */
		skb = skb_clone(orig_skb, GFP_ATOMIC);
                …
		head = strp->skb_head;
		if (!head) {
			head = skb;
                        …
		} else {
                        …
		}
                …
		/* Give skb to upper layer */
		strp->cb.rcv_msg(strp, head); // → sk_psock_init_strp
                …
	}
        …
}

That looks like a code path where we pass a cloned SKB.

> +		if (unlikely(!pskb_may_pull(skb, len_diff_abs)))
> +			return -ENOMEM;
> +		__skb_pull(skb, len_diff_abs);
> +	}
> +	bpf_compute_data_end_sk_skb(skb);
> +	if (tls_sw_has_ctx_rx(skb->sk)) {
> +		struct strp_msg *rxm = strp_msg(skb);
> +
> +		rxm->full_len += len_diff;
> +	}
> +	return ret;
> +}

[...]