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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200630085705.txwn62zixvxxs7rt@lion.mk-sys.cz>
Date:   Tue, 30 Jun 2020 10:57:05 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Sam Ravnborg <sam@...nborg.org>,
        Andrii Nakryiko <andriin@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH 04/16] net: bpfilter: use 'userprogs' syntax to build
 bpfilter_umh

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@...e.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