[04/16] net: bpfilter: use 'userprogs' syntax to build bpfilter_umh
diff mbox series

Message ID 20200423073929.127521-5-masahiroy@kernel.org
State New
Headers show
Series
  • kbuild: support 'userprogs' syntax
Related show

Commit Message

Masahiro Yamada April 23, 2020, 7:39 a.m. UTC
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>
---

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

Comments

kernel test robot April 28, 2020, 1:46 a.m. UTC | #1
Hi Masahiro,

I love your patch! Yet something to improve:

[auto build test ERROR on kbuild/for-next]
[cannot apply to linus/master v5.7-rc3 next-20200424]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Masahiro-Yamada/kbuild-support-userprogs-syntax/20200426-114001
base:   https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   /usr/bin/ld: i386 architecture of input file `net/bpfilter/main.o' is incompatible with i386:x86-64 output
>> collect2: error: ld returned 1 exit status

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Masahiro Yamada April 28, 2020, 5:13 a.m. UTC | #2
On Tue, Apr 28, 2020 at 10:46 AM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Masahiro,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on kbuild/for-next]
> [cannot apply to linus/master v5.7-rc3 next-20200424]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Masahiro-Yamada/kbuild-support-userprogs-syntax/20200426-114001
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git for-next
> config: i386-allyesconfig (attached as .config)
> compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>    /usr/bin/ld: i386 architecture of input file `net/bpfilter/main.o' is incompatible with i386:x86-64 output
> >> collect2: error: ld returned 1 exit status
>

Thanks.
-m32/-m64 is missing in the link time of the bpfilter_umh.
I will fix it soon.
Michal Kubecek June 8, 2020, 11:56 a.m. UTC | #3
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 <masahiroy@kernel.org>
> Reported-by: kbuild test robot <lkp@intel.com>
> ---
> 
>  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
Masahiro Yamada June 8, 2020, 1:32 p.m. UTC | #4
On Mon, Jun 8, 2020 at 8:56 PM Michal Kubecek <mkubecek@suse.cz> 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 <masahiroy@kernel.org>
> > Reported-by: kbuild test robot <lkp@intel.com>
> > ---
> >
> >  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.
Masahiro Yamada June 30, 2020, 6:30 a.m. UTC | #5
Hi Michal, Alexei,

On Mon, Jun 8, 2020 at 8:56 PM Michal Kubecek <mkubecek@suse.cz> 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 <masahiroy@kernel.org>
> > Reported-by: kbuild test robot <lkp@intel.com>
> > ---
> >
> >  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.
Michal Kubecek June 30, 2020, 8:57 a.m. UTC | #6
On Tue, Jun 30, 2020 at 03:30:04PM +0900, Masahiro Yamada wrote:
> On Mon, Jun 8, 2020 at 8:56 PM Michal Kubecek <mkubecek@suse.cz> 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
Alexei Starovoitov June 30, 2020, 4:47 p.m. UTC | #7
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 <mkubecek@suse.cz> 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 <masahiroy@kernel.org>
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > > ---
> > >
> > >  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.

Patch
diff mbox series

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