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 <[email protected]>
---
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
--
2.25.1
On Thu, Apr 23, 2020 at 04:39:17PM +0900, Masahiro Yamada wrote:
> 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 <[email protected]>
> Reported-by: kbuild test robot <[email protected]>
> ---
>
> 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
On Mon, Jun 8, 2020 at 8:56 PM Michal Kubecek <[email protected]> wrote:
>
> On Thu, Apr 23, 2020 at 04:39:17PM +0900, Masahiro Yamada wrote:
> > 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 <[email protected]>
> > Reported-by: kbuild test robot <[email protected]>
> > ---
> >
> > 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
Sorry. ifeq was accidentally dropped.
I will restore it.
--
Best Regards
Masahiro Yamada
Hi Michal, Alexei,
On Mon, Jun 8, 2020 at 8:56 PM Michal Kubecek <[email protected]> wrote:
>
> On Thu, Apr 23, 2020 at 04:39:17PM +0900, Masahiro Yamada wrote:
> > 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 <[email protected]>
> > Reported-by: kbuild test robot <[email protected]>
> > ---
> >
> > 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
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.
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.
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.
If you look at net/bpfilter/Kconfig,
BPFILTER_UMH already has 'default m'.
So, I assume the most expected use-case
is modular.
My suggestion is to replace 'default m' with 'depends on m'.
config BPFILTER_UMH
tristate "bpfilter kernel module with user mode helper"
depends on CC_CAN_LINK
depends on m
Then BPFILTER_UMH will be restricted to either m or n.
Link umh dynamically because we can expect rootfs
is already mounted for the module case.
Comments are appreciated.
--
Best Regards
Masahiro Yamada
On Tue, Jun 30, 2020 at 03:30:04PM +0900, Masahiro Yamada wrote:
> On Mon, Jun 8, 2020 at 8:56 PM Michal Kubecek <[email protected]> wrote:
> >
> > 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?
>
> 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
>
>
>
> 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.
>
>
> 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.
>
> 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.
I wouldn't be very happy with this solution as that would mean adding an
extra build dependency which we don't really need. We might even
consider disabling CONFIG_BPFILTER_UMH instead.
> Idea 2:
>
> Allow umh to become only modular,
> and drop -static flag entirely.
>
> If you look at net/bpfilter/Kconfig,
> BPFILTER_UMH already has 'default m'.
> So, I assume the most expected use-case
> is modular.
>
> My suggestion is to replace 'default m' with 'depends on m'.
>
> config BPFILTER_UMH
> tristate "bpfilter kernel module with user mode helper"
> depends on CC_CAN_LINK
> depends on m
>
> Then BPFILTER_UMH will be restricted to either m or n.
> Link umh dynamically because we can expect rootfs
> is already mounted for the module case.
This wouldn't be a problem for me or openSUSE kernels as we already have
CONFIG_BPFILTER_UMH=m. But I can't speak for others, I'm not sure if
there are some use cases requiring CONFIG_BPFILTER_UMH=y.
Michal
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 <[email protected]> wrote:
> >
> > On Thu, Apr 23, 2020 at 04:39:17PM +0900, Masahiro Yamada wrote:
> > > 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 <[email protected]>
> > > Reported-by: kbuild test robot <[email protected]>
> > > ---
> > >
> > > 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.