| Message ID | 20200423073929.127521-5-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: support 'userprogs' syntax | expand |
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
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.
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
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.
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.
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
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.
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
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(-)