Re: [PATCH 04/16] net: bpfilter: use 'userprogs' syntax to build bpfilter_umh

[Michal Kubecek]

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@xxxxxxx> 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