Re: [PATCH mptcp-next 2/4] bpf: Add bpf_mptcpify helper

[Geliang Tang <geliang.tang@suse.com>]

On Mon, Jul 03, 2023 at 02:58:33PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 03/07/2023 08:54, Geliang Tang wrote:
> > On Thu, Jun 29, 2023 at 07:43:50PM +0200, Matthieu Baerts wrote:
> >> On 29/06/2023 04:12, Geliang Tang wrote:
> >>> diff --git a/net/core/filter.c b/net/core/filter.c
> >>> index 968139f4a1ac..e439f8b5f203 100644
> >>> --- a/net/core/filter.c
> >>> +++ b/net/core/filter.c
> >>> @@ -11587,6 +11587,24 @@ const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
> >>>  	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_MPTCP],
> >>>  };
> >>>  
> >>> +BPF_CALL_1(bpf_mptcpify, struct sock *, sk)
> >>> +{
> >>> +	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP) {
> >>> +		sk->sk_protocol = IPPROTO_MPTCP;
> >>> +		return (unsigned long)sk;
> >>> +	}
> >>
> >> How do we ensure such modifications are done at the right moment? I
> >> mean: we can only change the protocol ID in very few places, before even
> >> creating the socket (__sock_create()?). If we change it after, we will
> >> break stuff: tcp ops, security labels, etc.
> > 
> > You're right, Matt, we need to do the modifications at the very beginning
> > of sys_socket(). In v3, a new wrapper socket_create() is added, it's the
> > right place to do the modifications.
> 
> Thank you for this v3 and for your replies: it is clearer with your replies!
> 
> I hope such wrapper can be accepted upstream :)

Hi Matt,

I just sent a v4, adding hooks in __sock_create(), instead of adding a
wrapper. I think v4 is much more acceptable by upstream.

> 
> It is then important to fail if bpf_mptcpify() is not executed at the
> beginning of this new "socket_create()" wrapper. Will it be the case?
> 
> I guess the verifier will make sure bpf_mptcpify() is only called with a
> function having "struct socket_args" as first argument so "by accident",
> it will only be bpf_mptcpify(), right? Maybe there is a way to clearly
> mark that bpf_mptcpify() cannot be called elsewhere? Or make sure it
> fails if the socket has already been created? (maybe that's something we
> need to ask to BPF devs)
> 
> One last thing: in your v3, I think you restrict the conversion to IPv4
> only sockets, no?
> 
>   (args->family == AF_INET || args->family == AF_INET6)

I updated this in v4.

Thanks,
-Geliang

> 
> (and I guess you saw that the kernel test robot complained about one of
> the patch because the new wrapper is not declared in the "include" dir)
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net