On Thu, Jun 08, 2023 at 08:52:36PM -0700, Yonghong Song wrote:
On 6/8/23 5:11 PM, Krister Johansen wrote:
In certain situations a program with subprograms may have a NULL extable entry. This should not happen, and when it does, it turns a single trap into multiple. Add a test case for further debugging and to prevent regressions. N.b: without any other patches this can panic or oops a kernel.
It would be great if you can add the panic call stack in the commit message.
Please also mention that three identical bpf programs in the test significantly increased the 'oops' chance. Just one program may not be able to trigger the issue.
Apologies, I mistakenly put the oops in the cover letter when you did tell me to put it in the commit message for this patch. That is fixed now.
Signed-off-by: Krister Johansen kjlx@templeofstupid.com
.../bpf/prog_tests/subprogs_extable.c | 31 +++++++++++++ .../bpf/progs/test_subprogs_extable.c | 46 +++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/subprogs_extable.c create mode 100644 tools/testing/selftests/bpf/progs/test_subprogs_extable.c
diff --git a/tools/testing/selftests/bpf/prog_tests/subprogs_extable.c b/tools/testing/selftests/bpf/prog_tests/subprogs_extable.c new file mode 100644 index 000000000000..2201988274a4 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/subprogs_extable.c @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h> +#include "test_subprogs_extable.skel.h"
+void test_subprogs_extable(void) +{
- const int READ_SZ = 456;
There is no need to use uppercase for READ_SZ. Just do const int read_sz = 456; is sufficient.
- struct test_subprogs_extable *skel;
- int err;
- skel = test_subprogs_extable__open();
- if (!ASSERT_OK_PTR(skel, "skel_open"))
return;- err = test_subprogs_extable__load(skel);
- if (!ASSERT_OK(err, "skel_load"))
goto cleanup;You can combine the above open and load with a single one test_subprogs_extable__open_and_load().
- err = test_subprogs_extable__attach(skel);
- if (!ASSERT_OK(err, "skel_attach"))
goto cleanup;- /* trigger tracepoint */
- ASSERT_OK(trigger_module_test_read(READ_SZ), "trigger_read");
I think we should at least ensure that the program is triggered. For example, add a global variable 'triggered' in the program and triggered will be set to 1 in the program if the program is running. Here check skel->bss->triggered must be 1.
Thanks for the additional feedback on cleaning this up. I've incorporated the requested changes and will send out a v4 shortly.
-K