Hi,
On 1/18/2024 9:27 AM, Song Liu wrote:
> Hi,
>
> On Wed, Jan 17, 2024 at 5:11 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxx> 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
