[bpf-next,v4,01/10] xfrm: bpf: Move xfrm_interface_bpf.c to xfrm_bpf.c

Commit Message

Daniel Xu Dec. 4, 2023, 8:56 p.m. UTC

This commit moves the contents of xfrm_interface_bpf.c into a new file,
xfrm_bpf.c This is in preparation for adding more xfrm kfuncs. We'd like
to keep all the bpf integrations in a single file.

Signed-off-by: Daniel Xu <[e-mail]>
---
 net/xfrm/Makefile                             |  7 +------
 net/xfrm/{xfrm_interface_bpf.c => xfrm_bpf.c} | 12 ++++++++----
 2 files changed, 9 insertions(+), 10 deletions(-)
 rename net/xfrm/{xfrm_interface_bpf.c => xfrm_bpf.c} (88%)

Comments

Alexei Starovoitov Dec. 5, 2023, 1:58 a.m. UTC | #1

On Mon, Dec 4, 2023 at 12:56 PM Daniel Xu <[e-mail]> wrote:
>
> This commit moves the contents of xfrm_interface_bpf.c into a new file,
> xfrm_bpf.c This is in preparation for adding more xfrm kfuncs. We'd like
> to keep all the bpf integrations in a single file.
>
> Signed-off-by: Daniel Xu <[e-mail]>
> ---
>  net/xfrm/Makefile                             |  7 +------
>  net/xfrm/{xfrm_interface_bpf.c => xfrm_bpf.c} | 12 ++++++++----
>  2 files changed, 9 insertions(+), 10 deletions(-)
>  rename net/xfrm/{xfrm_interface_bpf.c => xfrm_bpf.c} (88%)
>
> diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
> index cd47f88921f5..29fff452280d 100644
> --- a/net/xfrm/Makefile
> +++ b/net/xfrm/Makefile
> @@ -5,12 +5,6 @@
>
>  xfrm_interface-$(CONFIG_XFRM_INTERFACE) += xfrm_interface_core.o
>
> -ifeq ($(CONFIG_XFRM_INTERFACE),m)
> -xfrm_interface-$(CONFIG_DEBUG_INFO_BTF_MODULES) += xfrm_interface_bpf.o
> -else ifeq ($(CONFIG_XFRM_INTERFACE),y)
> -xfrm_interface-$(CONFIG_DEBUG_INFO_BTF) += xfrm_interface_bpf.o
> -endif
> -
>  obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
>                       xfrm_input.o xfrm_output.o \
>                       xfrm_sysctl.o xfrm_replay.o xfrm_device.o
> @@ -21,3 +15,4 @@ obj-$(CONFIG_XFRM_USER_COMPAT) += xfrm_compat.o
>  obj-$(CONFIG_XFRM_IPCOMP) += xfrm_ipcomp.o
>  obj-$(CONFIG_XFRM_INTERFACE) += xfrm_interface.o
>  obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o
> +obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_bpf.o
...
> +#if IS_BUILTIN(CONFIG_XFRM_INTERFACE) || \
> +    (IS_MODULE(CONFIG_XFRM_INTERFACE) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
> +
>  /* bpf_xfrm_info - XFRM metadata information
>   *
>   * Members:
> @@ -108,3 +110,5 @@ int __init register_xfrm_interface_bpf(void)
>         return register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS,
>                                          &xfrm_interface_kfunc_set);
>  }
> +
> +#endif /* xfrm interface */

imo the original approach was cleaner.
#ifdefs in .c should be avoided when possible.
But I'm not going to insist.

ipsec folks please ack the first 3 patches.

Steffen Klassert Dec. 7, 2023, 11:52 a.m. UTC | #2

On Mon, Dec 04, 2023 at 01:56:21PM -0700, Daniel Xu wrote:
> This commit moves the contents of xfrm_interface_bpf.c into a new file,
> xfrm_bpf.c This is in preparation for adding more xfrm kfuncs. We'd like
> to keep all the bpf integrations in a single file.
> 
> Signed-off-by: Daniel Xu <[e-mail]>
> ---
>  net/xfrm/Makefile                             |  7 +------
>  net/xfrm/{xfrm_interface_bpf.c => xfrm_bpf.c} | 12 ++++++++----
>  2 files changed, 9 insertions(+), 10 deletions(-)
>  rename net/xfrm/{xfrm_interface_bpf.c => xfrm_bpf.c} (88%)

Adding Eyal to Cc, he added the xfrm_interface_bpf.c file.

Acked-by: Steffen Klassert <[e-mail]>

Eyal Birger Dec. 7, 2023, 9:08 p.m. UTC | #3

Hi Daniel,

On Thu, Dec 7, 2023 at 3:52 AM Steffen Klassert via Devel
<[e-mail]> wrote:
>
> On Mon, Dec 04, 2023 at 01:56:21PM -0700, Daniel Xu wrote:
> > This commit moves the contents of xfrm_interface_bpf.c into a new file,
> > xfrm_bpf.c This is in preparation for adding more xfrm kfuncs. We'd like
> > to keep all the bpf integrations in a single file.

This takes away the nice ability to reload the xfrm interface
related kfuncs when reloading the xfrm interface.

I also find it a little strange that the kfuncs would be available
when the xfrm interface isn't loaded.

So imho it makes sense that these kfuncs would be built
as part of the module and not as part of the core.

Eyal.

Steffen Klassert Dec. 8, 2023, 8:35 a.m. UTC | #4

On Thu, Dec 07, 2023 at 01:08:08PM -0800, Eyal Birger wrote:
> Hi Daniel,
> 
> On Thu, Dec 7, 2023 at 3:52 AM Steffen Klassert via Devel
> <[e-mail]> wrote:
> >
> > On Mon, Dec 04, 2023 at 01:56:21PM -0700, Daniel Xu wrote:
> > > This commit moves the contents of xfrm_interface_bpf.c into a new file,
> > > xfrm_bpf.c This is in preparation for adding more xfrm kfuncs. We'd like
> > > to keep all the bpf integrations in a single file.
> 
> This takes away the nice ability to reload the xfrm interface
> related kfuncs when reloading the xfrm interface.
> 
> I also find it a little strange that the kfuncs would be available
> when the xfrm interface isn't loaded.
> 
> So imho it makes sense that these kfuncs would be built
> as part of the module and not as part of the core.

I proposed to merge all the bpf extensions into one file.
With that I wanted to avoid to have 'many' files under
/net/xfrm that fall basically under bpf maintainance
scope. But if there are practical reasons to have these
spilted, I'm OK with that too.

Daniel Xu Dec. 9, 2023, 12:04 a.m. UTC | #5

Hi Eyal,

On Thu, Dec 07, 2023 at 01:08:08PM -0800, Eyal Birger wrote:
> Hi Daniel,
> 
> On Thu, Dec 7, 2023 at 3:52 AM Steffen Klassert via Devel
> <[e-mail]> wrote:
> >
> > On Mon, Dec 04, 2023 at 01:56:21PM -0700, Daniel Xu wrote:
> > > This commit moves the contents of xfrm_interface_bpf.c into a new file,
> > > xfrm_bpf.c This is in preparation for adding more xfrm kfuncs. We'd like
> > > to keep all the bpf integrations in a single file.
> 
> This takes away the nice ability to reload the xfrm interface
> related kfuncs when reloading the xfrm interface.
> 
> I also find it a little strange that the kfuncs would be available
> when the xfrm interface isn't loaded.

I think technically since the xfrm interface module does the kfunc
registration, the kfuncs would only be available after the module is
loaded.

> 
> So imho it makes sense that these kfuncs would be built
> as part of the module and not as part of the core.

But yeah, point taken. I will revert this commit for v5.

> 
> Eyal.

Thanks,
Daniel

Patch

diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
index cd47f88921f5..29fff452280d 100644
--- a/net/xfrm/Makefile
+++ b/net/xfrm/Makefile
@@ -5,12 +5,6 @@ 
 
 xfrm_interface-$(CONFIG_XFRM_INTERFACE) += xfrm_interface_core.o
 
-ifeq ($(CONFIG_XFRM_INTERFACE),m)
-xfrm_interface-$(CONFIG_DEBUG_INFO_BTF_MODULES) += xfrm_interface_bpf.o
-else ifeq ($(CONFIG_XFRM_INTERFACE),y)
-xfrm_interface-$(CONFIG_DEBUG_INFO_BTF) += xfrm_interface_bpf.o
-endif
-
 obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
 		      xfrm_input.o xfrm_output.o \
 		      xfrm_sysctl.o xfrm_replay.o xfrm_device.o
@@ -21,3 +15,4 @@  obj-$(CONFIG_XFRM_USER_COMPAT) += xfrm_compat.o
 obj-$(CONFIG_XFRM_IPCOMP) += xfrm_ipcomp.o
 obj-$(CONFIG_XFRM_INTERFACE) += xfrm_interface.o
 obj-$(CONFIG_XFRM_ESPINTCP) += espintcp.o
+obj-$(CONFIG_DEBUG_INFO_BTF) += xfrm_bpf.o
diff --git a/net/xfrm/xfrm_interface_bpf.c b/net/xfrm/xfrm_bpf.c
similarity index 88%
rename from net/xfrm/xfrm_interface_bpf.c
rename to net/xfrm/xfrm_bpf.c
index 7d5e920141e9..3d3018b87f96 100644
--- a/net/xfrm/xfrm_interface_bpf.c
+++ b/net/xfrm/xfrm_bpf.c
@@ -1,9 +1,8 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
-/* Unstable XFRM Helpers for TC-BPF hook
+/* Unstable XFRM BPF helpers.
  *
- * These are called from SCHED_CLS BPF programs. Note that it is
- * allowed to break compatibility for these functions since the interface they
- * are exposed through to BPF programs is explicitly unstable.
+ * Note that it is allowed to break compatibility for these functions since the
+ * interface they are exposed through to BPF programs is explicitly unstable.
  */
 
 #include <linux/bpf.h>
@@ -12,6 +11,9 @@ 
 #include <net/dst_metadata.h>
 #include <net/xfrm.h>
 
+#if IS_BUILTIN(CONFIG_XFRM_INTERFACE) || \
+    (IS_MODULE(CONFIG_XFRM_INTERFACE) && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
+
 /* bpf_xfrm_info - XFRM metadata information
  *
  * Members:
@@ -108,3 +110,5 @@  int __init register_xfrm_interface_bpf(void)
 	return register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS,
 					 &xfrm_interface_kfunc_set);
 }
+
+#endif /* xfrm interface */