linux-kernel - Re: [PATCH] bpf: force inc_active()/dec

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]

Date:   Sun, 23 Jul 2023 20:31:47 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Alexei Starovoitov" <alexei.starovoitov@...il.com>,
        "Yafang Shao" <laoar.shao@...il.com>
Cc:     "Arnd Bergmann" <arnd@...nel.org>,
        "Alexei Starovoitov" <ast@...nel.org>,
        "Daniel Borkmann" <daniel@...earbox.net>,
        "Andrii Nakryiko" <andrii@...nel.org>,
        "Hou Tao" <houtao1@...wei.com>,
        "Martin KaFai Lau" <martin.lau@...ux.dev>,
        "Song Liu" <song@...nel.org>, "Yonghong Song" <yhs@...com>,
        "John Fastabend" <john.fastabend@...il.com>,
        "KP Singh" <kpsingh@...nel.org>,
        "Stanislav Fomichev" <sdf@...gle.com>,
        "Hao Luo" <haoluo@...gle.com>, "Jiri Olsa" <jolsa@...nel.org>,
        "Kumar Kartikeya Dwivedi" <memxor@...il.com>,
        bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] bpf: force inc_active()/dec_active() to be inline functions

On Sun, Jul 23, 2023, at 18:46, Alexei Starovoitov wrote:
> On Sun, Jul 23, 2023 at 7:25 AM Yafang Shao <laoar.shao@...il.com> wrote:
>> On Sat, Jul 22, 2023 at 3:48 PM Arnd Bergmann <arnd@...nel.org> wrote:
>> > From: Arnd Bergmann <arnd@...db.de>
>> >
>> > Splitting these out into separate helper functions means that we
>> > actually pass an uninitialized variable into another function call
>> > if dec_active() happens to not be inlined, and CONFIG_PREEMPT_RT
>> > is disabled:
>>
>> Do you mean that the compiler can remove the flags automatically when
>> dec_active() is inlined, but can't remove it automatically when
>> dec_active() is not inlined ?

My educated guess is that it's fine when neither of them are inlined,
since then gcc can assume that 'flags' gets initialized by
inc_active(), and it's fine when both are inlined since dead code
elimination then gets rid of both the initialization and the use.

The only broken case should be when inc_active() is inlined and
gcc can tell that there is never an initialization, but 
dec_active() is not inlined, so gcc assumes it is actually used.

>> If so, why can't we improve the compiler ?
>
> Agree.
> Sounds like a compiler bug.

I don't know what you might want to change in the compiler
to avoid this. Compilers are free to decide which functions to
inline in the absence of noinline or always_inline flags.

One difference between gcc and clang is that gcc tries to
be smart about warnings by using information from inlining
to produce better warnings, while clang never uses information
across function boundaries for generated warnings, so it won't
find this one, but also would ignore an unconditional use
of the uninitialized variable. 

>> If we have to change the kernel, what about the change below?
>
> To workaround the compiler bug we can simply init flag=0 to silence
> the warn, but even that is silly. Passing flag=0 into irqrestore is buggy.

Maybe inc_active() could return the flags instead of modifying
the stack variable? that would also result in slightly better
code when it's not inlined.

     Arnd