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.
Thanks,
Song
