linux-kernel - Re: [PATCH bpf-next v5 3/3] selftests/bpf: Skip callback tests if jit is disabled in test

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: <342f1c7f-a8d3-dbba-a45f-66fc672883be@huaweicloud.com>
Date: Thu, 18 Jan 2024 09:11:14 +0800
From: Hou Tao <houtao@...weicloud.com>
To: Song Liu <song@...nel.org>, Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc: Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Eduard Zingerman <eddyz87@...il.com>,
 John Fastabend <john.fastabend@...il.com>, Jiri Olsa <jolsa@...nel.org>,
 bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v5 3/3] selftests/bpf: Skip callback tests if jit
 is disabled in test_verifier

Hi Song,

On 1/18/2024 1:20 AM, Song Liu wrote:
> On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:
> [...]
>> @@ -1622,6 +1624,16 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>>         alignment_prevented_execution = 0;
>>
>>         if (expected_ret == ACCEPT || expected_ret == VERBOSE_ACCEPT) {
>> +               if (fd_prog < 0 && saved_errno == EINVAL && jit_disabled) {
>> +                       for (i = 0; i < prog_len; i++, prog++) {
>> +                               if (!insn_is_pseudo_func(prog))
>> +                                       continue;
>> +                               printf("SKIP (callbacks are not allowed in non-JITed programs)\n");
>> +                               skips++;
>> +                               goto close_fds;
>> +                       }
>> +               }
>> +
> I would put this chunk above "alignment_prevented_execution = 0;".
>
> @@ -1619,6 +1621,16 @@ static void do_test_single(struct bpf_test
> *test, bool unpriv,
>                 goto close_fds;
>         }
>
> +       if (fd_prog < 0 && saved_errno == EINVAL && jit_disabled) {
> +               for (i = 0; i < prog_len; i++, prog++) {
> +                       if (!insn_is_pseudo_func(prog))
> +                               continue;
> +                       printf("SKIP (callbacks are not allowed in
> non-JITed programs)\n");
> +                       skips++;
> +                       goto close_fds;
> +               }
> +       }
> +
>         alignment_prevented_execution = 0;
>
>         if (expected_ret == ACCEPT || expected_ret == VERBOSE_ACCEPT) {
>
> Other than this,

The check was placed before the checking of expected_ret in v3. However
I suggested Tiezhu to move it after the checking of expected_ret due to
the following two reasons:
1) when the expected result is REJECT, the return value in about one
third of these test cases is -EINVAL. And I think we should not waste
the cpu to check the pseudo func and exit prematurely, instead we should
let test_verifier check expected_err.
2) As for now all expected_ret of these failed cases are ACCEPT when jit
is disabled, so I think it will be enough for current situation and we
can revise it later if the checking of pseudo func is too later.

So wdyt ?

>
> Acked-by: Song Liu <song@...nel.org>
>
> Thanks,
> Song
>
> .