[PATCH 0/3] net: bpfilter: clean-up build rules

[PATCH 0/3] net: bpfilter: clean-up build rules

[Masahiro Yamada]

I am unhappy with ugly hacks in this Makefile.
(I could understand what is the hand-crafted .bpfilter_umh.o.cmd
used for.)

Moreover, CONFIG_OUTPUT_FORMAT is causing build errors
because it is defined only for x86.

As far as I understood, what is happening here is
to embed the 'bpfilter_umh' as a blob.
Instead, we can use '.incbin' to include it into a *.S file.

Also, let's stop (ab)using host-program rules.

Only build test is done.



Masahiro Yamada (3):
  bpfilter: add bpfilter_umh to .gitignore
  bpfilter: include bpfilter_umh in assembly instead of using objcopy
  bpfilter: do not (ab)use host-program build rule

 net/bpfilter/.gitignore                 |  1 +
 net/bpfilter/Makefile                   | 30 ++++++++++++------------------
 net/bpfilter/bpfilter_kern.c            | 11 +++++------
 net/bpfilter/{main.c => bpfilter_umh.c} |  0
 net/bpfilter/bpfilter_umh_blob.S        |  7 +++++++
 5 files changed, 25 insertions(+), 24 deletions(-)
 create mode 100644 net/bpfilter/.gitignore
 rename net/bpfilter/{main.c => bpfilter_umh.c} (100%)
 create mode 100644 net/bpfilter/bpfilter_umh_blob.S

-- 
2.7.4

[PATCH 1/3] bpfilter: add bpfilter_umh to .gitignore

[Masahiro Yamada]

bpfilter_umh is a generated file.  It should be ignored by git.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 net/bpfilter/.gitignore | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 net/bpfilter/.gitignore

diff --git a/net/bpfilter/.gitignore b/net/bpfilter/.gitignore
new file mode 100644
index 0000000..e97084e
--- /dev/null
+++ b/net/bpfilter/.gitignore
@@ -0,0 +1 @@
+bpfilter_umh
-- 
2.7.4

[PATCH 2/3] bpfilter: include bpfilter_umh in assembly instead of using objcopy

[Masahiro Yamada]

Do not use the troublesome ELF magic.  What is happening here is to
embed a user-space program into the kernel.  Simply wrap it in the
assembly with the '.incbin' directive.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 net/bpfilter/Makefile            | 15 ++-------------
 net/bpfilter/bpfilter_kern.c     | 11 +++++------
 net/bpfilter/bpfilter_umh_blob.S |  7 +++++++
 3 files changed, 14 insertions(+), 19 deletions(-)
 create mode 100644 net/bpfilter/bpfilter_umh_blob.S

diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
index aafa720..39c6980 100644
--- a/net/bpfilter/Makefile
+++ b/net/bpfilter/Makefile
@@ -15,18 +15,7 @@ ifeq ($(CONFIG_BPFILTER_UMH), y)
 HOSTLDFLAGS += -static
 endif
 
-# a bit of elf magic to convert bpfilter_umh binary into a binary blob
-# inside bpfilter_umh.o elf file referenced by
-# _binary_net_bpfilter_bpfilter_umh_start symbol
-# which bpfilter_kern.c passes further into umh blob loader at run-time
-quiet_cmd_copy_umh = GEN $@
-      cmd_copy_umh = echo ':' > $(obj)/.bpfilter_umh.o.cmd; \
-      $(OBJCOPY) -I binary -O $(CONFIG_OUTPUT_FORMAT) \
-      -B `$(OBJDUMP) -f $<|grep architecture|cut -d, -f1|cut -d' ' -f2` \
-      --rename-section .data=.init.rodata $< $@
-
-$(obj)/bpfilter_umh.o: $(obj)/bpfilter_umh
-	$(call cmd,copy_umh)
+$(obj)/bpfilter_umh_blob.o: $(obj)/bpfilter_umh
 
 obj-$(CONFIG_BPFILTER_UMH) += bpfilter.o
-bpfilter-objs += bpfilter_kern.o bpfilter_umh.o
+bpfilter-objs += bpfilter_kern.o bpfilter_umh_blob.o
diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
index b13d058..fcc1a7c 100644
--- a/net/bpfilter/bpfilter_kern.c
+++ b/net/bpfilter/bpfilter_kern.c
@@ -10,11 +10,8 @@
 #include <linux/file.h>
 #include "msgfmt.h"
 
-#define UMH_start _binary_net_bpfilter_bpfilter_umh_start
-#define UMH_end _binary_net_bpfilter_bpfilter_umh_end
-
-extern char UMH_start;
-extern char UMH_end;
+extern char bpfilter_umh_start;
+extern char bpfilter_umh_end;
 
 static struct umh_info info;
 /* since ip_getsockopt() can run in parallel, serialize access to umh */
@@ -89,7 +86,9 @@ static int __init load_umh(void)
 	int err;
 
 	/* fork usermode process */
-	err = fork_usermode_blob(&UMH_start, &UMH_end - &UMH_start, &info);
+	err = fork_usermode_blob(&bpfilter_umh_end,
+				 &bpfilter_umh_end - &bpfilter_umh_start,
+				 &info);
 	if (err)
 		return err;
 	pr_info("Loaded bpfilter_umh pid %d\n", info.pid);
diff --git a/net/bpfilter/bpfilter_umh_blob.S b/net/bpfilter/bpfilter_umh_blob.S
new file mode 100644
index 0000000..40311d1
--- /dev/null
+++ b/net/bpfilter/bpfilter_umh_blob.S
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+	.section .init.rodata, "a"
+	.global bpfilter_umh_start
+bpfilter_umh_start:
+	.incbin "net/bpfilter/bpfilter_umh"
+	.global bpfilter_umh_end
+bpfilter_umh_end:
-- 
2.7.4

[PATCH 3/3] bpfilter: do not (ab)use host-program build rule

[Masahiro Yamada]

It is an ugly hack to overwrite $(HOSTCC) with $(CC) to reuse the
build rules from scripts/Makefile.host.  It should not be tedious
to write a build rule for its own.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 net/bpfilter/Makefile                   | 17 +++++++++++------
 net/bpfilter/{main.c => bpfilter_umh.c} |  0
 2 files changed, 11 insertions(+), 6 deletions(-)
 rename net/bpfilter/{main.c => bpfilter_umh.c} (100%)

diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
index 39c6980..6571b30 100644
--- a/net/bpfilter/Makefile
+++ b/net/bpfilter/Makefile
@@ -3,18 +3,23 @@
 # Makefile for the Linux BPFILTER layer.
 #
 
-hostprogs-y := bpfilter_umh
-bpfilter_umh-objs := main.o
-HOSTCFLAGS += -I. -Itools/include/ -Itools/include/uapi
-HOSTCC := $(CC)
-
 ifeq ($(CONFIG_BPFILTER_UMH), y)
 # builtin bpfilter_umh should be compiled with -static
 # since rootfs isn't mounted at the time of __init
 # function is called and do_execv won't find elf interpreter
-HOSTLDFLAGS += -static
+STATIC := -static
 endif
 
+quiet_cmd_cc_user = CC      $@
+      cmd_cc_user = $(CC) -Wall -Wmissing-prototypes -O2 -std=gnu89 \
+		    -I$(srctree) -I$(srctree)/tools/include/ \
+		    -I$(srctree)/tools/include/uapi $(STATIC) -o $@ $<
+
+$(obj)/bpfilter_umh: $(src)/bpfilter_umh.c FORCE
+	$(call if_changed,cc_user)
+
+targets += bpfilter_umh
+
 $(obj)/bpfilter_umh_blob.o: $(obj)/bpfilter_umh
 
 obj-$(CONFIG_BPFILTER_UMH) += bpfilter.o
diff --git a/net/bpfilter/main.c b/net/bpfilter/bpfilter_umh.c
similarity index 100%
rename from net/bpfilter/main.c
rename to net/bpfilter/bpfilter_umh.c
-- 
2.7.4

Re: [PATCH 2/3] bpfilter: include bpfilter_umh in assembly instead of using objcopy

[Alexei Starovoitov]

On Sat, Jun 09, 2018 at 02:12:09AM +0900, Masahiro Yamada wrote:
> Do not use the troublesome ELF magic.  What is happening here is to
> embed a user-space program into the kernel.  Simply wrap it in the
> assembly with the '.incbin' directive.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  net/bpfilter/Makefile            | 15 ++-------------
>  net/bpfilter/bpfilter_kern.c     | 11 +++++------
>  net/bpfilter/bpfilter_umh_blob.S |  7 +++++++
>  3 files changed, 14 insertions(+), 19 deletions(-)
>  create mode 100644 net/bpfilter/bpfilter_umh_blob.S
> 
> diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
> index aafa720..39c6980 100644
> --- a/net/bpfilter/Makefile
> +++ b/net/bpfilter/Makefile
> @@ -15,18 +15,7 @@ ifeq ($(CONFIG_BPFILTER_UMH), y)
>  HOSTLDFLAGS += -static
>  endif
>  
> -# a bit of elf magic to convert bpfilter_umh binary into a binary blob
> -# inside bpfilter_umh.o elf file referenced by
> -# _binary_net_bpfilter_bpfilter_umh_start symbol
> -# which bpfilter_kern.c passes further into umh blob loader at run-time
> -quiet_cmd_copy_umh = GEN $@
> -      cmd_copy_umh = echo ':' > $(obj)/.bpfilter_umh.o.cmd; \
> -      $(OBJCOPY) -I binary -O $(CONFIG_OUTPUT_FORMAT) \
> -      -B `$(OBJDUMP) -f $<|grep architecture|cut -d, -f1|cut -d' ' -f2` \
> -      --rename-section .data=.init.rodata $< $@
> -
> -$(obj)/bpfilter_umh.o: $(obj)/bpfilter_umh
> -	$(call cmd,copy_umh)
> +$(obj)/bpfilter_umh_blob.o: $(obj)/bpfilter_umh
>  
>  obj-$(CONFIG_BPFILTER_UMH) += bpfilter.o
> -bpfilter-objs += bpfilter_kern.o bpfilter_umh.o
> +bpfilter-objs += bpfilter_kern.o bpfilter_umh_blob.o
> diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
> index b13d058..fcc1a7c 100644
> --- a/net/bpfilter/bpfilter_kern.c
> +++ b/net/bpfilter/bpfilter_kern.c
> @@ -10,11 +10,8 @@
>  #include <linux/file.h>
>  #include "msgfmt.h"
>  
> -#define UMH_start _binary_net_bpfilter_bpfilter_umh_start
> -#define UMH_end _binary_net_bpfilter_bpfilter_umh_end
> -
> -extern char UMH_start;
> -extern char UMH_end;
> +extern char bpfilter_umh_start;
> +extern char bpfilter_umh_end;
>  
>  static struct umh_info info;
>  /* since ip_getsockopt() can run in parallel, serialize access to umh */
> @@ -89,7 +86,9 @@ static int __init load_umh(void)
>  	int err;
>  
>  	/* fork usermode process */
> -	err = fork_usermode_blob(&UMH_start, &UMH_end - &UMH_start, &info);
> +	err = fork_usermode_blob(&bpfilter_umh_end,
> +				 &bpfilter_umh_end - &bpfilter_umh_start,
> +				 &info);
>  	if (err)
>  		return err;
>  	pr_info("Loaded bpfilter_umh pid %d\n", info.pid);
> diff --git a/net/bpfilter/bpfilter_umh_blob.S b/net/bpfilter/bpfilter_umh_blob.S
> new file mode 100644
> index 0000000..40311d1
> --- /dev/null
> +++ b/net/bpfilter/bpfilter_umh_blob.S
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +	.section .init.rodata, "a"
> +	.global bpfilter_umh_start
> +bpfilter_umh_start:
> +	.incbin "net/bpfilter/bpfilter_umh"

Interesting. I think this is good idea. Looks cleaner than objcopy magic.
btw CONFIG_OUTPUT_FORMAT already fixed by
commit 8d97ca6b6755 ("bpfilter: fix OUTPUT_FORMAT") in net tree.
Could you please rebase on top of that tree?

Re: [PATCH 3/3] bpfilter: do not (ab)use host-program build rule

[Alexei Starovoitov]

On Sat, Jun 09, 2018 at 02:12:10AM +0900, Masahiro Yamada wrote:
> It is an ugly hack to overwrite $(HOSTCC) with $(CC) to reuse the
> build rules from scripts/Makefile.host.  It should not be tedious
> to write a build rule for its own.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  net/bpfilter/Makefile                   | 17 +++++++++++------
>  net/bpfilter/{main.c => bpfilter_umh.c} |  0
>  2 files changed, 11 insertions(+), 6 deletions(-)
>  rename net/bpfilter/{main.c => bpfilter_umh.c} (100%)
> 
> diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
> index 39c6980..6571b30 100644
> --- a/net/bpfilter/Makefile
> +++ b/net/bpfilter/Makefile
> @@ -3,18 +3,23 @@
>  # Makefile for the Linux BPFILTER layer.
>  #
>  
> -hostprogs-y := bpfilter_umh
> -bpfilter_umh-objs := main.o
> -HOSTCFLAGS += -I. -Itools/include/ -Itools/include/uapi
> -HOSTCC := $(CC)

that is a hack indeed. I don't like it either, but see below.

> -
>  ifeq ($(CONFIG_BPFILTER_UMH), y)
>  # builtin bpfilter_umh should be compiled with -static
>  # since rootfs isn't mounted at the time of __init
>  # function is called and do_execv won't find elf interpreter
> -HOSTLDFLAGS += -static
> +STATIC := -static
>  endif
>  
> +quiet_cmd_cc_user = CC      $@
> +      cmd_cc_user = $(CC) -Wall -Wmissing-prototypes -O2 -std=gnu89 \
> +		    -I$(srctree) -I$(srctree)/tools/include/ \
> +		    -I$(srctree)/tools/include/uapi $(STATIC) -o $@ $<
> +
> +$(obj)/bpfilter_umh: $(src)/bpfilter_umh.c FORCE
> +	$(call if_changed,cc_user)

Does this scale?
Please see two top patches here:
https://git.kernel.org/pub/scm/linux/kernel/git/ast/bpf.git/log/?h=ipt_bpf
that add more meat to bpfilter and a lot more files.
Recompiling all of them at once isn't nice either.
This Makefile needs different .c -> .o rules for bpfilter_kern.c files
that get compiled into kernel module and for the rest of umh files:
main.c ctor.c init.c gen.c etc
that need to be compiled .c -> .o differently.
I don't see how to do it without ugly hacks in Makefile.
In that sense HOSTCC = CC hack looked the least ugly to me that's
why I went with it.
Better ideas?

Re: [PATCH 2/3] bpfilter: include bpfilter_umh in assembly instead of using objcopy

[Masahiro Yamada]

2018-06-09 5:47 GMT+09:00 Alexei Starovoitov <alexei.starovoitov@gmail.com>:
> On Sat, Jun 09, 2018 at 02:12:09AM +0900, Masahiro Yamada wrote:
>> Do not use the troublesome ELF magic.  What is happening here is to
>> embed a user-space program into the kernel.  Simply wrap it in the
>> assembly with the '.incbin' directive.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  net/bpfilter/Makefile            | 15 ++-------------
>>  net/bpfilter/bpfilter_kern.c     | 11 +++++------
>>  net/bpfilter/bpfilter_umh_blob.S |  7 +++++++
>>  3 files changed, 14 insertions(+), 19 deletions(-)
>>  create mode 100644 net/bpfilter/bpfilter_umh_blob.S
>>
>> diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
>> index aafa720..39c6980 100644
>> --- a/net/bpfilter/Makefile
>> +++ b/net/bpfilter/Makefile
>> @@ -15,18 +15,7 @@ ifeq ($(CONFIG_BPFILTER_UMH), y)
>>  HOSTLDFLAGS += -static
>>  endif
>>
>> -# a bit of elf magic to convert bpfilter_umh binary into a binary blob
>> -# inside bpfilter_umh.o elf file referenced by
>> -# _binary_net_bpfilter_bpfilter_umh_start symbol
>> -# which bpfilter_kern.c passes further into umh blob loader at run-time
>> -quiet_cmd_copy_umh = GEN $@
>> -      cmd_copy_umh = echo ':' > $(obj)/.bpfilter_umh.o.cmd; \
>> -      $(OBJCOPY) -I binary -O $(CONFIG_OUTPUT_FORMAT) \
>> -      -B `$(OBJDUMP) -f $<|grep architecture|cut -d, -f1|cut -d' ' -f2` \
>> -      --rename-section .data=.init.rodata $< $@
>> -
>> -$(obj)/bpfilter_umh.o: $(obj)/bpfilter_umh
>> -     $(call cmd,copy_umh)
>> +$(obj)/bpfilter_umh_blob.o: $(obj)/bpfilter_umh
>>
>>  obj-$(CONFIG_BPFILTER_UMH) += bpfilter.o
>> -bpfilter-objs += bpfilter_kern.o bpfilter_umh.o
>> +bpfilter-objs += bpfilter_kern.o bpfilter_umh_blob.o
>> diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
>> index b13d058..fcc1a7c 100644
>> --- a/net/bpfilter/bpfilter_kern.c
>> +++ b/net/bpfilter/bpfilter_kern.c
>> @@ -10,11 +10,8 @@
>>  #include <linux/file.h>
>>  #include "msgfmt.h"
>>
>> -#define UMH_start _binary_net_bpfilter_bpfilter_umh_start
>> -#define UMH_end _binary_net_bpfilter_bpfilter_umh_end
>> -
>> -extern char UMH_start;
>> -extern char UMH_end;
>> +extern char bpfilter_umh_start;
>> +extern char bpfilter_umh_end;
>>
>>  static struct umh_info info;
>>  /* since ip_getsockopt() can run in parallel, serialize access to umh */
>> @@ -89,7 +86,9 @@ static int __init load_umh(void)
>>       int err;
>>
>>       /* fork usermode process */
>> -     err = fork_usermode_blob(&UMH_start, &UMH_end - &UMH_start, &info);
>> +     err = fork_usermode_blob(&bpfilter_umh_end,
>> +                              &bpfilter_umh_end - &bpfilter_umh_start,
>> +                              &info);
>>       if (err)
>>               return err;
>>       pr_info("Loaded bpfilter_umh pid %d\n", info.pid);
>> diff --git a/net/bpfilter/bpfilter_umh_blob.S b/net/bpfilter/bpfilter_umh_blob.S
>> new file mode 100644
>> index 0000000..40311d1
>> --- /dev/null
>> +++ b/net/bpfilter/bpfilter_umh_blob.S
>> @@ -0,0 +1,7 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +     .section .init.rodata, "a"
>> +     .global bpfilter_umh_start
>> +bpfilter_umh_start:
>> +     .incbin "net/bpfilter/bpfilter_umh"
>
> Interesting. I think this is good idea. Looks cleaner than objcopy magic.
> btw CONFIG_OUTPUT_FORMAT already fixed by
> commit 8d97ca6b6755 ("bpfilter: fix OUTPUT_FORMAT") in net tree.
> Could you please rebase on top of that tree?
>


OK, I will rebase it.

BTW, I only compile-tested this patch.
Could you check if it really works?



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 3/3] bpfilter: do not (ab)use host-program build rule

[Masahiro Yamada]

2018-06-09 5:52 GMT+09:00 Alexei Starovoitov <alexei.starovoitov@gmail.com>:
> On Sat, Jun 09, 2018 at 02:12:10AM +0900, Masahiro Yamada wrote:
>> It is an ugly hack to overwrite $(HOSTCC) with $(CC) to reuse the
>> build rules from scripts/Makefile.host.  It should not be tedious
>> to write a build rule for its own.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  net/bpfilter/Makefile                   | 17 +++++++++++------
>>  net/bpfilter/{main.c => bpfilter_umh.c} |  0
>>  2 files changed, 11 insertions(+), 6 deletions(-)
>>  rename net/bpfilter/{main.c => bpfilter_umh.c} (100%)
>>
>> diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
>> index 39c6980..6571b30 100644
>> --- a/net/bpfilter/Makefile
>> +++ b/net/bpfilter/Makefile
>> @@ -3,18 +3,23 @@
>>  # Makefile for the Linux BPFILTER layer.
>>  #
>>
>> -hostprogs-y := bpfilter_umh
>> -bpfilter_umh-objs := main.o
>> -HOSTCFLAGS += -I. -Itools/include/ -Itools/include/uapi
>> -HOSTCC := $(CC)
>
> that is a hack indeed. I don't like it either, but see below.
>
>> -
>>  ifeq ($(CONFIG_BPFILTER_UMH), y)
>>  # builtin bpfilter_umh should be compiled with -static
>>  # since rootfs isn't mounted at the time of __init
>>  # function is called and do_execv won't find elf interpreter
>> -HOSTLDFLAGS += -static
>> +STATIC := -static
>>  endif
>>
>> +quiet_cmd_cc_user = CC      $@
>> +      cmd_cc_user = $(CC) -Wall -Wmissing-prototypes -O2 -std=gnu89 \
>> +                 -I$(srctree) -I$(srctree)/tools/include/ \
>> +                 -I$(srctree)/tools/include/uapi $(STATIC) -o $@ $<
>> +
>> +$(obj)/bpfilter_umh: $(src)/bpfilter_umh.c FORCE
>> +     $(call if_changed,cc_user)
>
> Does this scale?
> Please see two top patches here:
> https://git.kernel.org/pub/scm/linux/kernel/git/ast/bpf.git/log/?h=ipt_bpf
> that add more meat to bpfilter and a lot more files.

Oh, I just thought main.c was the only user-program file.

> Recompiling all of them at once isn't nice either.

Indeed.

It should be able to compile .c -> .o separately.

> This Makefile needs different .c -> .o rules for bpfilter_kern.c files
> that get compiled into kernel module and for the rest of umh files:
> main.c ctor.c init.c gen.c etc
> that need to be compiled .c -> .o differently.
> I don't see how to do it without ugly hacks in Makefile.
> In that sense HOSTCC = CC hack looked the least ugly to me that's
> why I went with it.
> Better ideas?


I will think about it after your patches land.

I will drop this for now.


-- 
Best Regards
Masahiro Yamada