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

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

On Thu, Jun 29, 2023 at 07:43:50PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/06/2023 04:12, Geliang Tang wrote:
> > This patch implements a new struct bpf_func_proto bpf_mptcpify_proto. And
> > define a new helper bpf_mptcpify() to mptcpify a TCP socket dynamically as
> > an MPTCP one.
> 
> Nice feature, thank you for working on that!
> 
> Is it linked to what Nicolas Rybowski looked at a few years ago? I think
> he put info in a Github ticket. Was there not an issue with this
> technique? Did you see it and fix it?
> 
> I didn't look at the patchset in detail but I have one question, please
> see below:
> 
> > 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.

The v3 works well now. We can get three MP_CAPABLEs in the log.

Thanks,
-Geliang

> 
> I thought it was not possible to hook at the right place when Nicolas
> looked at that and/or ensure the restriction was done but I might be
> mistaken and the situation has probably changed.
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net