Received-SPF: pass (google.com: domain of linux-kernel+bounces-29624-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Subject: Re: [PATCH bpf-next v5 3/3] selftests/bpf: Skip callback tests if jit
 is disabled in test_verifier
To: Song Liu <song@kernel.org>
Cc: Tiezhu Yang <yangtiezhu@loongson.cn>, Alexei Starovoitov
 <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>,
 Andrii Nakryiko <andrii@kernel.org>, Eduard Zingerman <eddyz87@gmail.com>,
 John Fastabend <john.fastabend@gmail.com>, Jiri Olsa <jolsa@kernel.org>,
 bpf@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240117111000.12763-1-yangtiezhu@loongson.cn>
 <20240117111000.12763-4-yangtiezhu@loongson.cn>
 <CAPhsuW6mWoQQ1M-uPE_i+RWv=t5GaVqUDAObWgpEC-PCYSbwHQ@mail.gmail.com>
 <342f1c7f-a8d3-dbba-a45f-66fc672883be@huaweicloud.com>
 <CAPhsuW78ONz23-X_6AKt1SfVfepfNP=h=EUAjtUG+K1cKMVH2A@mail.gmail.com>
From: Hou Tao <houtao@huaweicloud.com>
Message-ID: <eb7734bc-b516-f179-49bf-937e205f1fae@huaweicloud.com>
Date: Thu, 18 Jan 2024 09:32:57 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <CAPhsuW78ONz23-X_6AKt1SfVfepfNP=h=EUAjtUG+K1cKMVH2A@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US

Hi,

On 1/18/2024 9:27 AM, Song Liu wrote:
> Hi,
>
> On Wed, Jan 17, 2024 at 5:11 PM Hou Tao <houtao@huaweicloud.com> wrote:
>> Hi Song,
>>
>> On 1/18/2024 1:20 AM, Song Liu wrote:
>>> On Wed, Jan 17, 2024 at 3:10 AM Tiezhu Yang <yangtiezhu@loongson.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
> I missed this part while reading the history of the set.
>
>> 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.
> I was thinking jit_disabled is not a common use case so that it is OK for
> this path to be a little expensive.
>
>> 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.
> That said, I won't object if we ship this version as-is.

I see and thanks for the explanation.
> Thanks,
> Song