Add a hook named update_socket_protocol in __sys_socket(), for bpf
progs to attach to and update socket protocol. One user case is to
force legacy TCP apps to create and use MPTCP sockets instead of
TCP ones.

Define a mod_ret set named bpf_mptcp_fmodret_ids, add the hook
update_socket_protocol into this set, and register it in
bpf_mptcp_kfunc_init().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/bpf.c | 17 +++++++++++++++++
 net/socket.c    | 25 +++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 5a0a84ad94af..c43aee31014d 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -12,6 +12,23 @@
 #include <linux/bpf.h>
 #include "protocol.h"
 
+#ifdef CONFIG_BPF_JIT
+BTF_SET8_START(bpf_mptcp_fmodret_ids)
+BTF_ID_FLAGS(func, update_socket_protocol)
+BTF_SET8_END(bpf_mptcp_fmodret_ids)
+
+static const struct btf_kfunc_id_set bpf_mptcp_fmodret_set = {
+	.owner = THIS_MODULE,
+	.set   = &bpf_mptcp_fmodret_ids,
+};
+
+static int __init bpf_mptcp_kfunc_init(void)
+{
+	return register_btf_fmodret_id_set(&bpf_mptcp_fmodret_set);
+}
+late_initcall(bpf_mptcp_kfunc_init);
+#endif /* CONFIG_BPF_JIT */
+
 struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
 {
 	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
diff --git a/net/socket.c b/net/socket.c
index 2b0e54b2405c..586a437d7a5e 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1644,11 +1644,36 @@ struct file *__sys_socket_file(int family, int type, int protocol)
 	return sock_alloc_file(sock, flags, NULL);
 }
 
+/**
+ *	A hook for bpf progs to attach to and update socket protocol.
+ *
+ *	A static noinline declaration here could cause the compiler to
+ *	optimize away the function. A global noinline declaration will
+ *	keep the definition, but may optimize away the callsite.
+ *	Therefore, __weak is needed to ensure that the call is still
+ *	emitted, by telling the compiler that we don't know what the
+ *	function might eventually be.
+ *
+ *	__diag_* below are needed to dismiss the missing prototype warning.
+ */
+
+__diag_push();
+__diag_ignore_all("-Wmissing-prototypes",
+		  "kfuncs which will be used in BPF programs");
+
+__weak noinline int update_socket_protocol(int family, int type, int protocol)
+{
+	return protocol;
+}
+
+__diag_pop();
+
 int __sys_socket(int family, int type, int protocol)
 {
 	struct socket *sock;
 	int flags;
 
+	protocol = update_socket_protocol(family, type, protocol);
 	sock = __sys_socket_create(family, type, protocol);
 	if (IS_ERR(sock))
 		return PTR_ERR(sock);
-- 
2.35.3

On Thu, Aug 03, 2023 at 03:30:39PM +0800, Geliang Tang wrote:
> Add a hook named update_socket_protocol in __sys_socket(), for bpf
> progs to attach to and update socket protocol. One user case is to
> force legacy TCP apps to create and use MPTCP sockets instead of
> TCP ones.
> 
> Define a mod_ret set named bpf_mptcp_fmodret_ids, add the hook
> update_socket_protocol into this set, and register it in
> bpf_mptcp_kfunc_init().
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>

...

> diff --git a/net/socket.c b/net/socket.c
> index 2b0e54b2405c..586a437d7a5e 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1644,11 +1644,36 @@ struct file *__sys_socket_file(int family, int type, int protocol)
>  	return sock_alloc_file(sock, flags, NULL);
>  }
>  
> +/**

Hi Geliang Tang,

nit: The format of the text below is not in kernel doc format,
     so it is probably better if the comment begins with '/*'
     rather than '/**'.

> + *	A hook for bpf progs to attach to and update socket protocol.
> + *
> + *	A static noinline declaration here could cause the compiler to
> + *	optimize away the function. A global noinline declaration will
> + *	keep the definition, but may optimize away the callsite.
> + *	Therefore, __weak is needed to ensure that the call is still
> + *	emitted, by telling the compiler that we don't know what the
> + *	function might eventually be.
> + *
> + *	__diag_* below are needed to dismiss the missing prototype warning.
> + */

...

On Thu, Aug 03, 2023 at 02:53:38PM +0200, Simon Horman wrote:
> On Thu, Aug 03, 2023 at 03:30:39PM +0800, Geliang Tang wrote:
> > Add a hook named update_socket_protocol in __sys_socket(), for bpf
> > progs to attach to and update socket protocol. One user case is to
> > force legacy TCP apps to create and use MPTCP sockets instead of
> > TCP ones.
> > 
> > Define a mod_ret set named bpf_mptcp_fmodret_ids, add the hook
> > update_socket_protocol into this set, and register it in
> > bpf_mptcp_kfunc_init().
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> 
> ...
> 
> > diff --git a/net/socket.c b/net/socket.c
> > index 2b0e54b2405c..586a437d7a5e 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -1644,11 +1644,36 @@ struct file *__sys_socket_file(int family, int type, int protocol)
> >  	return sock_alloc_file(sock, flags, NULL);
> >  }
> >  
> > +/**
> 
> Hi Geliang Tang,
> 
> nit: The format of the text below is not in kernel doc format,
>      so it is probably better if the comment begins with '/*'
>      rather than '/**'.

I do use '/*' here first, but got a checkpatch.pl warning:

  ./scripts/checkpatch.pl v8-0001-bpf-Add-update_socket_protocol-hook.patch 
  WARNING: networking block comments don't use an empty /* line, use /* Comment...
  #63: FILE: net/socket.c:1648:
  +/*
  + *	A hook for bpf progs to attach to and update socket protocol.

  total: 0 errors, 1 warnings, 0 checks, 59 lines checked

And I found that other comments in net/socket.c all begins with '/**'.
So I use '/**' here too.

Thanks,
-Geliang

> 
> > + *	A hook for bpf progs to attach to and update socket protocol.
> > + *
> > + *	A static noinline declaration here could cause the compiler to
> > + *	optimize away the function. A global noinline declaration will
> > + *	keep the definition, but may optimize away the callsite.
> > + *	Therefore, __weak is needed to ensure that the call is still
> > + *	emitted, by telling the compiler that we don't know what the
> > + *	function might eventually be.
> > + *
> > + *	__diag_* below are needed to dismiss the missing prototype warning.
> > + */
> 
> ...

Hi Geliang,

(only with MPTCP list because there are too many people involved and
that's just a detail)

On 03/08/2023 15:05, Geliang Tang wrote:
> On Thu, Aug 03, 2023 at 02:53:38PM +0200, Simon Horman wrote:
>> On Thu, Aug 03, 2023 at 03:30:39PM +0800, Geliang Tang wrote:
>>> Add a hook named update_socket_protocol in __sys_socket(), for bpf
>>> progs to attach to and update socket protocol. One user case is to
>>> force legacy TCP apps to create and use MPTCP sockets instead of
>>> TCP ones.
>>>
>>> Define a mod_ret set named bpf_mptcp_fmodret_ids, add the hook
>>> update_socket_protocol into this set, and register it in
>>> bpf_mptcp_kfunc_init().
>>>
>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>
>> ...
>>
>>> diff --git a/net/socket.c b/net/socket.c
>>> index 2b0e54b2405c..586a437d7a5e 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -1644,11 +1644,36 @@ struct file *__sys_socket_file(int family, int type, int protocol)
>>>  	return sock_alloc_file(sock, flags, NULL);
>>>  }
>>>  
>>> +/**
>>
>> Hi Geliang Tang,
>>
>> nit: The format of the text below is not in kernel doc format,
>>      so it is probably better if the comment begins with '/*'
>>      rather than '/**'.
> 
> I do use '/*' here first, but got a checkpatch.pl warning:
> 
>   ./scripts/checkpatch.pl v8-0001-bpf-Add-update_socket_protocol-hook.patch 
>   WARNING: networking block comments don't use an empty /* line, use /* Comment...
>   #63: FILE: net/socket.c:1648:
>   +/*
>   + *	A hook for bpf progs to attach to and update socket protocol.
> 
>   total: 0 errors, 1 warnings, 0 checks, 59 lines checked
> 
> And I found that other comments in net/socket.c all begins with '/**'.
> So I use '/**' here too.

From what I understand, when using a comment block starting with "/**",
it is to insert kernel doc format (similar to doxygen if you know about
it) where you can generally see the description of a function with
parameters starting with "@", etc. e.g. what is done above
"sock_recvmsg()" in "net/socket.c".

But I see indeed there are many other "random" comments starting with
"/**" but I guess that's because "net/socket.c" is very old, when
checkpatch.pl didn't exist.

I think the best is follow the "new" style, e.g. what is done above
"skb_is_swtx_tstamp()" in "net/socket.c", starting on the first line and
without tabulations after the "*", e.g.

  /* A hook for bpf progs to attach to and update socket protocol.
   *
   * A static noinline (...)
   * (...)
   */

WDYT?
Would it fix 'checkpatch.pl' warnings?

(if you send a new version, please add the Closes tag in this patch as
well if you don't mind. You can also keep my Reviewed-by and Acked-by
tags if you want but then put your Signed-off-by tag at the end: the
last tag should be a Signed-off-by tag of the person sending the
patches, you in this case :) )

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/79

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Hi Geliang,

On 03/08/2023 09:30, Geliang Tang wrote:
> Add a hook named update_socket_protocol in __sys_socket(), for bpf
> progs to attach to and update socket protocol. One user case is to
> force legacy TCP apps to create and use MPTCP sockets instead of
> TCP ones.
> 
> Define a mod_ret set named bpf_mptcp_fmodret_ids, add the hook
> update_socket_protocol into this set, and register it in
> bpf_mptcp_kfunc_init().

Thank you for having looked at that!

Because it is related to MPTCP:

Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>

I don't know if your scripts to apply patches support the "Closes" tag
but just in case:

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/79

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net