Received-SPF: pass (google.com: domain of linux-kernel+bounces-29620-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
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>
In-Reply-To: <342f1c7f-a8d3-dbba-a45f-66fc672883be@huaweicloud.com>
From: Song Liu <song@kernel.org>
Date: Wed, 17 Jan 2024 17:27:01 -0800
Message-ID: <CAPhsuW78ONz23-X_6AKt1SfVfepfNP=h=EUAjtUG+K1cKMVH2A@mail.gmail.com>
Subject: Re: [PATCH bpf-next v5 3/3] selftests/bpf: Skip callback tests if jit
 is disabled in test_verifier
To: Hou Tao <houtao@huaweicloud.com>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, Jan 17, 2024 at 5:11=E2=80=AFPM Hou Tao <houtao@huaweicloud.com> wr=
ote:
>
> Hi Song,
>
> On 1/18/2024 1:20 AM, Song Liu wrote:
> > On Wed, Jan 17, 2024 at 3:10=E2=80=AFAM Tiezhu Yang <yangtiezhu@loongso=
n.cn> wrote:
> > [...]
> >> @@ -1622,6 +1624,16 @@ static void do_test_single(struct bpf_test *tes=
t, bool unpriv,
> >>         alignment_prevented_execution =3D 0;
> >>
> >>         if (expected_ret =3D=3D ACCEPT || expected_ret =3D=3D VERBOSE_=
ACCEPT) {
> >> +               if (fd_prog < 0 && saved_errno =3D=3D EINVAL && jit_di=
sabled) {
> >> +                       for (i =3D 0; i < prog_len; i++, prog++) {
> >> +                               if (!insn_is_pseudo_func(prog))
> >> +                                       continue;
> >> +                               printf("SKIP (callbacks are not allowe=
d in non-JITed programs)\n");
> >> +                               skips++;
> >> +                               goto close_fds;
> >> +                       }
> >> +               }
> >> +
> > I would put this chunk above "alignment_prevented_execution =3D 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 =3D=3D EINVAL && jit_disabled) {
> > +               for (i =3D 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 =3D 0;
> >
> >         if (expected_ret =3D=3D ACCEPT || expected_ret =3D=3D VERBOSE_A=
CCEPT) {
> >
> > 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