[bpf-next,v8,1/4] bpf: Add update_socket_protocol hook

Commit Message

Geliang Tang Aug. 3, 2023, 7:30 a.m. UTC

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 <[e-mail]>
---
 net/mptcp/bpf.c | 17 +++++++++++++++++
 net/socket.c    | 25 +++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

Comments

Matthieu Baerts Aug. 3, 2023, 8:02 a.m. UTC | #1

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 <[e-mail]>

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

Simon Horman Aug. 3, 2023, 12:53 p.m. UTC | #2

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 <[e-mail]>

...

> 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.
> + */

...

Geliang Tang Aug. 3, 2023, 1:05 p.m. UTC | #3

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 <[e-mail]>
> 
> ...
> 
> > 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.
> > + */
> 
> ...

kernel test robot Aug. 3, 2023, 1:17 p.m. UTC | #4

Hi Geliang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Geliang-Tang/bpf-Add-update_socket_protocol-hook/20230803-153209
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/120b307aacd1791fac016d33e112069ffb7db21a.1691047403.git.geliang.tang%40suse.com
patch subject: [PATCH bpf-next v8 1/4] bpf: Add update_socket_protocol hook
config: nios2-randconfig-r006-20230731 (https://download.01.org/0day-ci/archive/20230803/[e-mail]/config)
compiler: nios2-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230803/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All warnings (new ones prefixed by >>):

>> net/socket.c:1648: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    *      A hook for bpf progs to attach to and update socket protocol.


vim +1648 net/socket.c

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

Patch

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);