linux-kernel - Re: [PATCH] selftests: kselftest.h: mark functions with 'noreturn'

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:   Fri, 29 Oct 2021 16:23:01 -0600
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Anders Roxell <anders.roxell@...aro.org>
Cc:     shuah@...nel.org, fenghua.yu@...el.com, reinette.chatre@...el.com,
        john.stultz@...aro.org, tglx@...utronix.de,
        akpm@...ux-foundation.org, nathan@...nel.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, llvm@...ts.linux.dev,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] selftests: kselftest.h: mark functions with 'noreturn'

On 10/29/21 4:08 PM, Nick Desaulniers wrote:
> On Fri, Oct 29, 2021 at 11:19 AM Shuah Khan <skhan@...uxfoundation.org> wrote:
>>
>> On 10/29/21 5:43 AM, Anders Roxell wrote:
>>> When building kselftests/capabilities the following warning shows up:
>>>
>>> clang -O2 -g -std=gnu99 -Wall    test_execve.c -lcap-ng -lrt -ldl -o test_execve
>>> test_execve.c:121:13: warning: variable 'have_outer_privilege' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>>>           } else if (unshare(CLONE_NEWUSER | CLONE_NEWNS) == 0) {
>>>                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> test_execve.c:136:9: note: uninitialized use occurs here
>>>           return have_outer_privilege;
>>>                  ^~~~~~~~~~~~~~~~~~~~
>>> test_execve.c:121:9: note: remove the 'if' if its condition is always true
>>>           } else if (unshare(CLONE_NEWUSER | CLONE_NEWNS) == 0) {
>>>                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> test_execve.c:94:27: note: initialize the variable 'have_outer_privilege' to silence this warning
>>>           bool have_outer_privilege;
>>>                                    ^
>>>                                     = false
>>>
>>> Rework so all the ksft_exit_*() functions have attribue
>>> '__attribute__((noreturn))' so the compiler knows that there wont be
>>> any return from the function. That said, without
>>> '__attribute__((noreturn))' the compiler warns about the above issue
>>> since it thinks that it will get back from the ksft_exit_skip()
>>> function, which it wont.
>>> Cleaning up the callers that rely on ksft_exit_*() return code, since
>>> the functions ksft_exit_*() have never returned anything.
>>>
>>> Signed-off-by: Anders Roxell <anders.roxell@...aro.org>
>>
>> Lot of changes to fix this warning. Is this necessary? I would
>> like to explore if there is an easier and localized change that
>> can fix the problem.
> 
> via `man 3 exit`:
> ```
> The  exit() function causes normal process termination ...
> ...
> RETURN VALUE
>         The exit() function does not return.
> ```
> so seeing `ksft_exit_pass`, `ksft_exit_fail`, `ksft_exit_fail_msg`,
> `ksft_exit_xfail`, `ksft_exit_xpass`, and `ksft_exit_skip` all
> unconditional call `exit` yet return an `int` looks wrong to me on
> first glance. So on that point this patch and its resulting diffstat
> LGTM.
> 
> That said, there are many changes that explicitly call `ksft_exit`
> with an expression; are those setting the correct exit code? Note that
> ksft_exit_pass is calling exit with KSFT_PASS which is 0.  So some of
> the negations don't look quite correct to me.  For example:
> 
> -       return !ksft_get_fail_cnt() ? ksft_exit_pass() : ksft_exit_fail();
> +       ksft_exit(!ksft_get_fail_cnt());
> 
> so if ksft_get_fail_cnt() returns 0, then we were calling
> ksft_exit_pass() which exited with 0. Now we'd be exiting with 1?
> 

Right. This is another concern I have that the tests will return
a different values and the wrapper will interpret them as failures.

So his doesn't look like the right change to fix the problem.

thanks,
-- Shuah