Re: [PATCH bpf-next v1 7/7] selftests/bpf: Add IRQ save/restore tests

[Eduard Zingerman <eddyz87@gmail.com>]

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@gmail.com>
> ---

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.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> 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().

> +{
> +	local_irq_balance_n();
> +	return 0;
> +}

[...]