On Thu, 21 Nov 2024 at 21:43, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2024-11-20 at 16:53 -0800, Kumar Kartikeya Dwivedi wrote:
> > Include tests that check for rejection in erroneous cases, like
> > unbalanced IRQ-disabled counts, within and across subprogs, invalid IRQ
> > flag state or input to kfuncs, behavior upon overwriting IRQ saved state
> > on stack, interaction with sleepable kfuncs/helpers, global functions,
> > and out of order restore. Include some success scenarios as well to
> > demonstrate usage.
> >
> > #123/1 irq/irq_restore_missing_1:OK
> > #123/2 irq/irq_restore_missing_2:OK
> > #123/3 irq/irq_restore_missing_3:OK
> > #123/4 irq/irq_restore_missing_3_minus_2:OK
> > #123/5 irq/irq_restore_missing_1_subprog:OK
> > #123/6 irq/irq_restore_missing_2_subprog:OK
> > #123/7 irq/irq_restore_missing_3_subprog:OK
> > #123/8 irq/irq_restore_missing_3_minus_2_subprog:OK
> > #123/9 irq/irq_balance:OK
> > #123/10 irq/irq_balance_n:OK
> > #123/11 irq/irq_balance_subprog:OK
> > #123/12 irq/irq_balance_n_subprog:OK
> > #123/13 irq/irq_global_subprog:OK
> > #123/14 irq/irq_restore_ooo:OK
> > #123/15 irq/irq_restore_ooo_3:OK
> > #123/16 irq/irq_restore_3_subprog:OK
> > #123/17 irq/irq_restore_4_subprog:OK
> > #123/18 irq/irq_restore_ooo_3_subprog:OK
> > #123/19 irq/irq_restore_invalid:OK
> > #123/20 irq/irq_save_invalid:OK
> > #123/21 irq/irq_restore_iter:OK
> > #123/22 irq/irq_save_iter:OK
> > #123/23 irq/irq_flag_overwrite:OK
> > #123/24 irq/irq_flag_overwrite_partial:OK
> > #123/25 irq/irq_sleepable_helper:OK
> > #123/26 irq/irq_sleepable_kfunc:OK
> > #123 irq:OK
> > Summary: 1/26 PASSED, 0 SKIPPED, 0 FAILED
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > ---
>
> The following error condition is not tested:
> "arg#%d doesn't point to an irq flag on stack".
> Also, I think a few tests are excessive.
> Otherwise looks good.
>
Will add.
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> [...]
>
> > diff --git a/tools/testing/selftests/bpf/prog_tests/irq.c b/tools/testing/selftests/bpf/prog_tests/irq.c
> > new file mode 100644
> > index 000000000000..496f4826ac37
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/irq.c
> > @@ -0,0 +1,9 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> > +#include <test_progs.h>
> > +#include <irq.skel.h>
> > +
> > +void test_irq(void)
> > +{
> > + RUN_TESTS(irq);
> > +}
>
> Nit: tools/testing/selftests/bpf/prog_tests/verifier.c
> could be used instead of a separate file.
>
> > diff --git a/tools/testing/selftests/bpf/progs/irq.c b/tools/testing/selftests/bpf/progs/irq.c
> > new file mode 100644
> > index 000000000000..5301b66fc752
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/irq.c
> > @@ -0,0 +1,393 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include "bpf_misc.h"
> > +
> > +SEC("?tc")
> > +__failure __msg("BPF_EXIT instruction cannot be used inside bpf_local_irq_save-ed region")
>
> Nit: I know this is not a fault of this series, but the error message
> is sort of confusing. BPF_EXIT is allowed for irq saved region,
> just it has to be an exit from a sub-program, not a whole program.
>
> > +int irq_restore_missing_1(struct __sk_buff *ctx)
> > +{
> > + unsigned long flags;
> > +
> > + bpf_local_irq_save(&flags);
> > + return 0;
> > +}
>
> [...]
>
> Nit: don't think this test adds much compared to irq_restore_missing_2.
>
> > +{
> > + unsigned long flags1;
> > + unsigned long flags2;
> > + unsigned long flags3;
> > +
> > + bpf_local_irq_save(&flags1);
> > + bpf_local_irq_save(&flags2);
> > + bpf_local_irq_save(&flags3);
> > + return 0;
> > +}
>
> [...]
>
> > +SEC("?tc")
> > +__success
> > +int irq_balance_n_subprog(struct __sk_buff *ctx)
>
> Nit: don't think this test adds much given irq_balance_n()
> and irq_balance_subprog().
My idea with both of these was to ensure when the state is copied in
and out on calls and when we're doing one or more than one
save/restore (which links prev_id into active_irq_id etc.) we don't
have problems, so they were definitely testing different scenarios.
But with the move into bpf_verifier_state they will indeed become
redundant, so I'm going to drop them in v2.
>
> > +{
> > + local_irq_balance_n();
> > + return 0;
> > +}
>
> [...]
>
