On Tue, Jun 30, 2020 at 03:30:04PM +0900, Masahiro Yamada wrote:

Hi Michal, Alexei,

On Mon, Jun 8, 2020 at 8:56 PM Michal Kubecek [off-list ref] wrote:

quoted

On Thu, Apr 23, 2020 at 04:39:17PM +0900, Masahiro Yamada wrote:

quoted

The user mode helper should be compiled for the same architecture as
the kernel.

This Makefile reuses the 'hostprogs' syntax by overriding HOSTCC with CC.

Now that Kbuild provides the syntax 'userprogs', use it to fix the
Makefile mess.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Reported-by: kbuild test robot <redacted>
---

 net/bpfilter/Makefile | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/bpfilter/Makefile b/net/bpfilter/Makefile
index 36580301da70..6ee650c6badb 100644
--- a/net/bpfilter/Makefile
+++ b/net/bpfilter/Makefile

@@ -3,17 +3,14 @@
 # Makefile for the Linux BPFILTER layer.
 #

-hostprogs := bpfilter_umh
+userprogs := bpfilter_umh
 bpfilter_umh-objs := main.o
-KBUILD_HOSTCFLAGS += -I $(srctree)/tools/include/ -I $(srctree)/tools/include/uapi
-HOSTCC := $(CC)
+user-ccflags += -I $(srctree)/tools/include/ -I $(srctree)/tools/include/uapi

-ifeq ($(CONFIG_BPFILTER_UMH), y)
-# builtin bpfilter_umh should be compiled with -static
+# builtin bpfilter_umh should be linked with -static
 # since rootfs isn't mounted at the time of __init
 # function is called and do_execv won't find elf interpreter
-KBUILD_HOSTLDFLAGS += -static
-endif
+bpfilter_umh-ldflags += -static

 $(obj)/bpfilter_umh_blob.o: $(obj)/bpfilter_umh

Hello,

I just noticed that this patch (now in mainline as commit 8a2cc0505cc4)
drops the test if CONFIG_BPFILTER_UMH is "y" so that -static is now
passed to the linker even if bpfilter_umh is built as a module which
wasn't the case in v5.7.

This is not mentioned in the commit message and the comment still says
"*builtin* bpfilter_umh should be linked with -static" so this change
doesn't seem to be intentional. Did I miss something?

Michal Kubecek

I was away for a while from this because I saw long discussion in
"net/bpfilter: Remove this broken and apparently unmaintained"


Please let me resume this topic now.


The original behavior of linking umh was like this:
  - If CONFIG_BPFILTER_UMH=y, bpfilter_umh was linked with -static
  - If CONFIG_BPFILTER_UMH=m, bpfilter_umh was linked without -static

That was done to make sure both static and dynamic linking work.
For production -static is necessary.
For debugging of usermode blob dynamic is beneficial.

Restoring the original behavior will add more complexity because
now we have CONFIG_CC_CAN_LINK and CONFIG_CC_CAN_LINK_STATIC
since commit b1183b6dca3e0d5

If CONFIG_BPFILTER_UMH=y, we need to check CONFIG_CC_CAN_LINK_STATIC.
If CONFIG_BPFILTER_UMH=m, we need to check CONFIG_CC_CAN_LINK.
This would make the Kconfig dependency logic too complicated.

Currently I'm working on adding bpf_iter to use 'user mode driver'
(old user mode blob) facility on top of Eric's patches.
So there will be quite a bit more complexity to build system.
Folks who don't want to deal with -static requirement should
just disable the feature.

To make it simpler, I'd like to suggest two options.



Idea 1:

  Always use -static irrespective of whether
  CONFIG_BPFILTER_UMH is y or m.

I don't think it's making it much simpler.
It's a tiny change to makefile.
I could be missing something.
Requiring -static for =y and =m is fine.

  Add two more lines to clarify this
  in the comment in net/bpfilter/Makefile:

  # builtin bpfilter_umh should be linked with -static
  # since rootfs isn't mounted at the time of __init
  # function is called and do_execv won't find elf interpreter.
  # Static linking is not required when bpfilter is modular, but
  # we always pass -static to keep the 'depends on' in Kconfig simple.



Idea 2:

   Allow umh to become only modular,
   and drop -static flag entirely.

absolutely not.
Both =y and =m are mandatory.

   If you look at net/bpfilter/Kconfig,
   BPFILTER_UMH already has 'default m'.
   So, I assume the most expected use-case
   is modular.

The default for BPFILTER is =N.
Distros should NOT be turning that to =y

Same thing with upcoming bpf_iter. It will default to =n.