[PATCH 0/5] Fix segfaults related to missing BTF support

[PATCH 0/5] Fix segfaults related to missing BTF support

[Arnaldo Carvalho de Melo]

Hi,

	While looking for reports to fix before release 1.28 I got to
Matthias reports about segfaults in systems where BTF isn't present, so
I introduced a regression test and infrastructure to allow testing
handling such a system, please take a look.

Thanks,

- Arnaldo

Arnaldo Carvalho de Melo (5):
  core: Add method to get the vmlinux BTF filename, allow overriding it via env var
  tests default_vmlinux_btf: Introduce test for using BTF by default
  pahole: Honour exclusive BTF loading
  tests default_vmlinux_btf: Cover the no args segfault too
  core, libctf: Check if constructor arguments are NULL before using them

 dwarves.c                    | 33 ++++++++++++++++++++++++++++++---
 dwarves.h                    |  2 ++
 libctf.c                     |  3 +++
 man-pages/pahole.1           |  4 ++++
 pahole.c                     | 11 +++++++++--
 tests/default_vmlinux_btf.sh | 32 ++++++++++++++++++++++++++++++++
 6 files changed, 80 insertions(+), 5 deletions(-)
 create mode 100755 tests/default_vmlinux_btf.sh

-- 
2.47.0

[PATCH 1/5] core: Add method to get the vmlinux BTF filename, allow overriding it via env var

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

In systems where BTF isn't available there were reports that the
simplest pahole call, without any args, segfaults.

To have a proper test before fixing this problem, allow overriding the
/sys/kernel/btf/vmlinux filename, so that even in systems with BTF we an
point it to a invalid location, making pahole think that there is no BTF
available and thus fallback to something that currently segfaults.

Using it:

  $ pahole list_head
  struct list_head {
  	struct list_head *         next;                 /*     0     8 */
  	struct list_head *         prev;                 /*     8     8 */

  	/* size: 16, cachelines: 1, members: 2 */
  	/* last cacheline: 16 bytes */
  };
  $
  $ PAHOLE_VMLINUX_BTF_FILENAME=foobar pahole list_head
  Segmentation fault (core dumped)
  $

Reported-by: Matthias Schwarzott <zzam@gentoo.org>
Cc: Alan Maguire <alan.maguire@oracle.com>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Song Liu <song@kernel.org>
Cc: Yonghong Song <yonghong.song@linux.dev>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 dwarves.c          | 17 +++++++++++++++--
 dwarves.h          |  2 ++
 man-pages/pahole.1 |  4 ++++
 pahole.c           |  2 +-
 4 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/dwarves.c b/dwarves.c
index 14ba4f0c6dd7f90f..5b51b55191177e21 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -2807,6 +2807,19 @@ static int filename__sprintf_build_id(const char *pathname, char *sbuild_id)
 static int vmlinux_path__nr_entries;
 static char **vmlinux_path;
 
+const char *vmlinux_path__btf_filename(void)
+{
+	static const char *vmlinux_btf;
+
+	if (vmlinux_btf == NULL) {
+		vmlinux_btf = getenv("PAHOLE_VMLINUX_BTF_FILENAME");
+		if (vmlinux_btf == NULL)
+			vmlinux_btf = "/sys/kernel/btf/vmlinux";
+	}
+
+	return vmlinux_btf;
+}
+
 static void vmlinux_path__exit(void)
 {
 	while (--vmlinux_path__nr_entries >= 0)
@@ -2933,7 +2946,7 @@ static int cus__load_running_kernel(struct cus *cus, struct conf_load *conf)
 	int err = 0;
 
 	if ((!conf || conf->format_path == NULL || strncmp(conf->format_path, "btf", 3) == 0) &&
-	    access("/sys/kernel/btf/vmlinux", R_OK) == 0) {
+	    access(vmlinux_path__btf_filename(), R_OK) == 0) {
 		int loader = debugging_formats__loader("btf");
 		if (loader == -1)
 			goto try_elf;
@@ -2941,7 +2954,7 @@ static int cus__load_running_kernel(struct cus *cus, struct conf_load *conf)
 		if (conf && conf->conf_fprintf)
 			conf->conf_fprintf->has_alignment_info = debug_fmt_table[loader]->has_alignment_info;
 
-		if (debug_fmt_table[loader]->load_file(cus, conf, "/sys/kernel/btf/vmlinux") == 0)
+		if (debug_fmt_table[loader]->load_file(cus, conf, vmlinux_path__btf_filename()) == 0)
 			return 0;
 	}
 try_elf:
diff --git a/dwarves.h b/dwarves.h
index 49988f1cf9d45be8..1cb0d629f5265523 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -1628,6 +1628,8 @@ void dwarves__resolve_cacheline_size(const struct conf_load *conf, uint16_t user
 
 const char *dwarf_tag_name(const uint32_t tag);
 
+const char *vmlinux_path__btf_filename(void);
+
 const char *vmlinux_path__find_running_kernel(void);
 
 struct argp_state;
diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
index bd28259b65429acc..39e7b465c64e7e2c 100644
--- a/man-pages/pahole.1
+++ b/man-pages/pahole.1
@@ -75,6 +75,10 @@ struct list_head {
 };
 $
 .fi
+It is possible to override the /sys/kernel/btf/vmlinux file location by setting
+the PAHOLE_VMLINUX_BTF_FILENAME environment variable. This may be useful for
+testing, scripting when using a different BTF for vmlinux. Used in the pahole
+regression tests.
 
 If BTF is not present and no file is passed, then a vmlinux that matches the
 build-id for the running kernel will be looked up in the usual places,
diff --git a/pahole.c b/pahole.c
index 55d04cf82a3da683..b94cb1a979a6923d 100644
--- a/pahole.c
+++ b/pahole.c
@@ -3754,7 +3754,7 @@ try_sole_arg_as_class_names:
 		if (filename &&
 		    strstarts(filename, "/sys/kernel/btf/") &&
 		    strstr(filename, "/vmlinux") == NULL) {
-			base_btf_file = "/sys/kernel/btf/vmlinux";
+			base_btf_file = vmlinux_path__btf_filename();
 			conf_load.base_btf = btf__parse(base_btf_file, NULL);
 			if (libbpf_get_error(conf_load.base_btf)) {
 				fprintf(stderr, "Failed to parse base BTF '%s': %ld\n",
-- 
2.47.0

Re: [PATCH 4/5] tests default_vmlinux_btf: Cover the no args segfault too

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 04:45:42PM +0000, Alan Maguire wrote:
> On 18/11/2024 20:41, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> >   root@x1:/home/acme/git/pahole# pahole --running_kernel_vmlinux
> >   pahole: couldn't find a vmlinux that matches the running kernel
> >   HINT: Maybe you're inside a container or missing a debuginfo package?
> >   root@x1:/home/acme/git/pahole# tests/default_vmlinux_btf.sh
> >   Default BTF on a system without BTF: FAILED
> >   root@x1:/home/acme/git/pahole# pahole
> >   Segmentation fault (core dumped)
> >   root@x1:/home/acme/git/pahole#
> > 
> > Now to fix this one as well.
> > 
> > Reported-by: Matthias Schwarzott <zzam@gentoo.org>
> > Cc: Alan Maguire <alan.maguire@oracle.com>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Eduard Zingerman <eddyz87@gmail.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Song Liu <song@kernel.org>
> > Cc: Yonghong Song <yonghong.song@linux.dev>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

Thanks, added.

- Arnaldo

Re: [PATCH 1/5] core: Add method to get the vmlinux BTF filename, allow overriding it via env var

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 10:13:32AM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Nov 19, 2024 at 12:57:44PM +0000, Alan Maguire wrote:
> > On 18/11/2024 20:41, Arnaldo Carvalho de Melo wrote:
> > > In systems where BTF isn't available there were reports that the
> > > simplest pahole call, without any args, segfaults.
> 
> > > To have a proper test before fixing this problem, allow overriding the
> > > /sys/kernel/btf/vmlinux filename, so that even in systems with BTF we an
> > > point it to a invalid location, making pahole think that there is no BTF
> > > available and thus fallback to something that currently segfaults.
> 
> <SNIP>
> 
> > Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> 
> Thanks!

Ah, thanks, added to the cset,

- Arnaldo
  
> > Small thing - would it be worth dropping the _FILENAME suffix of the
> > envvar to just have PAHOLE_VMLINUX_BTF? One other thing below..
> 
> See below ends up being related to the other point you made.
>  
> > > +++ b/pahole.c
> > > @@ -3754,7 +3754,7 @@ try_sole_arg_as_class_names:
> > >  		if (filename &&
> > >  		    strstarts(filename, "/sys/kernel/btf/") &&
> > >  		    strstr(filename, "/vmlinux") == NULL) {
> > > -			base_btf_file = "/sys/kernel/btf/vmlinux";
> > > +			base_btf_file = vmlinux_path__btf_filename();
> > 
> > so in this case, we are specifying a module in /sys/kernel/btf and
> > implicitly we use /sys/kernel/btf/vmlinux as the base. I wonder if
> > there's much value for the envvar override here; I guess it might allow
> > us to test using a mismatched base vmlinux with split BTF?
> 
> Right, that would be possible with this patch as-is, which may be
> useful, we may want to have PAHOLE_VMLINUX_BTF_DIR as a counterpart to
> PAHOLE_VMLINUX_BTF_FILENAME, that would allow us to use some different
> directory with both vmlinux and modules.
> 
> Having both allows more flexibility, picking some arbitrary vmlinux
> plust some arbitrary directory with modules, etc. So I'd keep the patch
> as is and at some point add the PAHOLE_VMLINUX_BTF_DIR knob.
> 
> - Arnaldo
>  
> > >  			conf_load.base_btf = btf__parse(base_btf_file, NULL);
> > >  			if (libbpf_get_error(conf_load.base_btf)) {
> > >  				fprintf(stderr, "Failed to parse base BTF '%s': %ld\n",

Re: [PATCH 2/5] tests default_vmlinux_btf: Introduce test for using BTF by default

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 12:59:58PM +0000, Alan Maguire wrote:
> On 18/11/2024 20:41, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > On a system without any debugging info or when one specifies BTF as the
> > only BTF info desired but no BTF is available (or invalidated using
> > PAHOLE_VMLINUX_BTF_FILENAME to an invalid/non-existent file), we're
> > getting a segfault:
> > 
> >   root@x1:/home/acme/git/pahole# export PAHOLE_VMLINUX_BTF_FILENAME=non-existent
> >   root@x1:/home/acme/git/pahole# pahole --running_kernel_vmlinux
> >   pahole: couldn't find a vmlinux that matches the running kernel
> >   HINT: Maybe you're inside a container or missing a debuginfo package?
> >   root@x1:/home/acme/git/pahole# pahole
> >   Segmentation fault (core dumped)
> >   root@x1:/home/acme/git/pahole#
> > 
> > So add a test that checks for that before we fix it:
> > 
> >   root@x1:/home/acme/git/pahole# tests/default_vmlinux_btf.sh
> >   Default BTF on a system without BTF: FAILED
> >   root@x1:/home/acme/git/pahole#
> > 
> > Reported-by: Matthias Schwarzott <zzam@gentoo.org>
> > Cc: Alan Maguire <alan.maguire@oracle.com>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Eduard Zingerman <eddyz87@gmail.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Song Liu <song@kernel.org>
> > Cc: Yonghong Song <yonghong.song@linux.dev>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

Thanks, added to the cset,

- Arnaldo
 
> > ---
> >  tests/default_vmlinux_btf.sh | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >  create mode 100755 tests/default_vmlinux_btf.sh
> > 
> > diff --git a/tests/default_vmlinux_btf.sh b/tests/default_vmlinux_btf.sh
> > new file mode 100755
> > index 0000000000000000..a9effa2d6d37e0ee
> > --- /dev/null
> > +++ b/tests/default_vmlinux_btf.sh
> > @@ -0,0 +1,21 @@
> > +#!/bin/bash
> > +
> > +echo -n "Default BTF on a system without BTF: "
> > +
> > +ulimit -c 0
> > +
> > +# To suppress the "Segmentation fault core dumped" message in bash we
> > +# pipe it to some other command, if it segfaults it will not produce any
> > +# lines and thus we can infer from the number of lines that the segfault
> > +# took place, tricky, but couldn't find any other way to check this
> > +# while suppressing the core dumped message. -acme
> > +
> > +nr_lines=$(PAHOLE_VMLINUX_BTF_FILENAME=foobar pahole -F btf list_head 2>&1 | wc -l)
> > +
> > +if [ $nr_lines -eq 0 ] ; then
> > +	echo "FAILED"
> > +	exit 1
> > +fi
> > +
> > +echo "Ok"
> > +exit 0

Re: [PATCH 4/5] tests default_vmlinux_btf: Cover the no args segfault too

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 04:45:42PM +0000, Alan Maguire wrote:
> On 18/11/2024 20:41, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> >   root@x1:/home/acme/git/pahole# pahole --running_kernel_vmlinux
> >   pahole: couldn't find a vmlinux that matches the running kernel
> >   HINT: Maybe you're inside a container or missing a debuginfo package?
> >   root@x1:/home/acme/git/pahole# tests/default_vmlinux_btf.sh
> >   Default BTF on a system without BTF: FAILED
> >   root@x1:/home/acme/git/pahole# pahole
> >   Segmentation fault (core dumped)
> >   root@x1:/home/acme/git/pahole#
> > 
> > Now to fix this one as well.
> > 
> > Reported-by: Matthias Schwarzott <zzam@gentoo.org>
> > Cc: Alan Maguire <alan.maguire@oracle.com>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Eduard Zingerman <eddyz87@gmail.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Song Liu <song@kernel.org>
> > Cc: Yonghong Song <yonghong.song@linux.dev>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

Thanks, added to the cset,

- Arnaldo

Re: [PATCH 5/5] core, libctf: Check if constructor arguments are NULL before using them

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 05:02:39PM +0000, Alan Maguire wrote:
> On 18/11/2024 20:41, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> >   # pahole
> >   Segmentation fault (core dumped)
> >   #
> > 
> > Now we have:
> > 
> >   # ls -la bla
> >   ls: cannot access 'bla': No such file or directory
> >   # export PAHOLE_VMLINUX_BTF_FILENAME=bla
> >   # pahole
> >   pahole: couldn't find any debug information on this system.
> >   # pahole list_head
> >   pahole: couldn't find any debug information on this system.
> >   # pahole -F btf list_head
> >   pahole: couldn't find any btf debug information on this system.
> >   # pahole -F dwarf list_head
> >   pahole: couldn't find any dwarf debug information on this system.
> >   # tests/default_vmlinux_btf.sh
> >   Default BTF on a system without BTF: Ok
> >   #
> > 
> > Reported-by: Matthias Schwarzott <zzam@gentoo.org>
> > Cc: Alan Maguire <alan.maguire@oracle.com>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Eduard Zingerman <eddyz87@gmail.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Song Liu <song@kernel.org>
> > Cc: Yonghong Song <yonghong.song@linux.dev>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> Tested-by: Alan Maguire <alan.maguire@oracle.com>

Thanks, added to the cset,

- Arnaldo

Re: [PATCH 1/5] core: Add method to get the vmlinux BTF filename, allow overriding it via env var

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 10:24:14AM -0800, Eduard Zingerman wrote:
> On Mon, 2024-11-18 at 17:41 -0300, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > In systems where BTF isn't available there were reports that the
> > simplest pahole call, without any args, segfaults.
> > 
> > To have a proper test before fixing this problem, allow overriding the
> > /sys/kernel/btf/vmlinux filename, so that even in systems with BTF we an
> > point it to a invalid location, making pahole think that there is no BTF
> > available and thus fallback to something that currently segfaults.
> > 
> > Using it:
> > 
> >   $ pahole list_head
> >   struct list_head {
> >   	struct list_head *         next;                 /*     0     8 */
> >   	struct list_head *         prev;                 /*     8     8 */
> > 
> >   	/* size: 16, cachelines: 1, members: 2 */
> >   	/* last cacheline: 16 bytes */
> >   };
> >   $
> >   $ PAHOLE_VMLINUX_BTF_FILENAME=foobar pahole list_head
> >   Segmentation fault (core dumped)
> >   $
> > 
> > Reported-by: Matthias Schwarzott <zzam@gentoo.org>
> > Cc: Alan Maguire <alan.maguire@oracle.com>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Eduard Zingerman <eddyz87@gmail.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Song Liu <song@kernel.org>
> > Cc: Yonghong Song <yonghong.song@linux.dev>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> 
> Lgtm, tried setting PAHOLE_VMLINUX_BTF_FILENAME to unrelated selftests
> with some BTF, pahole works as expected (prints structures defined in
> the selftest).
> 
> Tested-by: Eduard Zingerman <eddyz87@gmail.com>

Thanks a lot, added to the cset!

- Arnaldo
 
> [...]

Re: [PATCH 2/5] tests default_vmlinux_btf: Introduce test for using BTF by default

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 10:35:35AM -0800, Eduard Zingerman wrote:
> On Mon, 2024-11-18 at 17:41 -0300, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > On a system without any debugging info or when one specifies BTF as the
> > only BTF info desired but no BTF is available (or invalidated using
> > PAHOLE_VMLINUX_BTF_FILENAME to an invalid/non-existent file), we're
> > getting a segfault:
> > 
> >   root@x1:/home/acme/git/pahole# export PAHOLE_VMLINUX_BTF_FILENAME=non-existent
> >   root@x1:/home/acme/git/pahole# pahole --running_kernel_vmlinux
> >   pahole: couldn't find a vmlinux that matches the running kernel
> >   HINT: Maybe you're inside a container or missing a debuginfo package?
> >   root@x1:/home/acme/git/pahole# pahole
> >   Segmentation fault (core dumped)
> >   root@x1:/home/acme/git/pahole#
> > 
> > So add a test that checks for that before we fix it:
> > 
> >   root@x1:/home/acme/git/pahole# tests/default_vmlinux_btf.sh
> >   Default BTF on a system without BTF: FAILED
> >   root@x1:/home/acme/git/pahole#
> > 
> > Reported-by: Matthias Schwarzott <zzam@gentoo.org>
> > Cc: Alan Maguire <alan.maguire@oracle.com>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Eduard Zingerman <eddyz87@gmail.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Song Liu <song@kernel.org>
> > Cc: Yonghong Song <yonghong.song@linux.dev>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > ---
> 
> Lgtm.
> Would it make sense to move this patch to the end of the series?
> In case someone does a bisect and runs the tests to find some regression.

Humm?

So you think it should be introduced only when it passes? I.e. when the
problem is fixed?

My practice so far has been to reproduce the problem manually, write a
test, show that it detects the problem, fix, then show that the
regression test shows that the problem is not present anymore.

I see your point about a bisection when running in the cset that
introduces the test case and in all before the fix is added will fail,
confusing the bisector or not allowing it to be automated :-\

So, yeah, probably, for automated bisection we should move it to after
the fix, when finishing the devel cycle, which is now, will do.

If someone disagrees, please holer.

- Arnaldo

Re: [PATCH 2/5] tests default_vmlinux_btf: Introduce test for using BTF by default

[Eduard Zingerman]

On Tue, 2024-11-19 at 16:49 -0300, Arnaldo Carvalho de Melo wrote:

[...]

> > Lgtm.
> > Would it make sense to move this patch to the end of the series?
> > In case someone does a bisect and runs the tests to find some regression.
> 
> Humm?
> 
> So you think it should be introduced only when it passes? I.e. when the
> problem is fixed?

Right.

> My practice so far has been to reproduce the problem manually, write a
> test, show that it detects the problem, fix, then show that the
> regression test shows that the problem is not present anymore.

Yes, the downside would be that anyone trying out the fix would
need to do some rebase to try the test w/o fix.

> I see your point about a bisection when running in the cset that
> introduces the test case and in all before the fix is added will fail,
> confusing the bisector or not allowing it to be automated :-\
> 
> So, yeah, probably, for automated bisection we should move it to after
> the fix, when finishing the devel cycle, which is now, will do.

Fwiw, that's what folks enforce for bpf selftests ¯\_(ツ)_/¯.

Re: [PATCH 2/5] tests default_vmlinux_btf: Introduce test for using BTF by default

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 11:54:12AM -0800, Eduard Zingerman wrote:
> On Tue, 2024-11-19 at 16:49 -0300, Arnaldo Carvalho de Melo wrote:
 
> [...]
 
> > > Lgtm.
> > > Would it make sense to move this patch to the end of the series?
> > > In case someone does a bisect and runs the tests to find some regression.
> 
> > Humm?
 
> > So you think it should be introduced only when it passes? I.e. when the
> > problem is fixed?
 
> Right.
 
> > My practice so far has been to reproduce the problem manually, write a
> > test, show that it detects the problem, fix, then show that the
> > regression test shows that the problem is not present anymore.
 
> Yes, the downside would be that anyone trying out the fix would
> need to do some rebase to try the test w/o fix.
 
> > I see your point about a bisection when running in the cset that
> > introduces the test case and in all before the fix is added will fail,
> > confusing the bisector or not allowing it to be automated :-\

> > So, yeah, probably, for automated bisection we should move it to after
> > the fix, when finishing the devel cycle, which is now, will do.
 
> Fwiw, that's what folks enforce for bpf selftests ¯\_(ツ)_/¯.

Makes sense, automated bisection trumps old practices :-)

- Arnaldo

Re: [PATCH 2/5] tests default_vmlinux_btf: Introduce test for using BTF by default

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 11:54:12AM -0800, Eduard Zingerman wrote:
> On Tue, 2024-11-19 at 16:49 -0300, Arnaldo Carvalho de Melo wrote:
> 
> [...]
> 
> > > Lgtm.

And I'm taking the lgtm as an Acked-by, maybe a Reviewed-by?

- Arnaldo

> > > Would it make sense to move this patch to the end of the series?
> > > In case someone does a bisect and runs the tests to find some regression.
> > 
> > Humm?
> > 
> > So you think it should be introduced only when it passes? I.e. when the
> > problem is fixed?
> 
> Right.
> 
> > My practice so far has been to reproduce the problem manually, write a
> > test, show that it detects the problem, fix, then show that the
> > regression test shows that the problem is not present anymore.
> 
> Yes, the downside would be that anyone trying out the fix would
> need to do some rebase to try the test w/o fix.
> 
> > I see your point about a bisection when running in the cset that
> > introduces the test case and in all before the fix is added will fail,
> > confusing the bisector or not allowing it to be automated :-\
> > 
> > So, yeah, probably, for automated bisection we should move it to after
> > the fix, when finishing the devel cycle, which is now, will do.
> 
> Fwiw, that's what folks enforce for bpf selftests ¯\_(ツ)_/¯.

Re: [PATCH 2/5] tests default_vmlinux_btf: Introduce test for using BTF by default

[Eduard Zingerman]

On Tue, 2024-11-19 at 17:12 -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Nov 19, 2024 at 11:54:12AM -0800, Eduard Zingerman wrote:
> > On Tue, 2024-11-19 at 16:49 -0300, Arnaldo Carvalho de Melo wrote:
> > 
> > [...]
> > 
> > > > Lgtm.
> 
> And I'm taking the lgtm as an Acked-by, maybe a Reviewed-by?

Sorry for confusion, let's go with Acked-by.

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

Re: [PATCH 2/5] tests default_vmlinux_btf: Introduce test for using BTF by default

[Arnaldo Carvalho de Melo]

On Tue, Nov 19, 2024 at 12:13:56PM -0800, Eduard Zingerman wrote:
> On Tue, 2024-11-19 at 17:12 -0300, Arnaldo Carvalho de Melo wrote:
> > On Tue, Nov 19, 2024 at 11:54:12AM -0800, Eduard Zingerman wrote:
> > > On Tue, 2024-11-19 at 16:49 -0300, Arnaldo Carvalho de Melo wrote:
> > > 
> > > [...]
> > > 
> > > > > Lgtm.
> > 
> > And I'm taking the lgtm as an Acked-by, maybe a Reviewed-by?
> 
> Sorry for confusion, let's go with Acked-by.
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Thats what I tentatively had added, and already pushed to the next
branch, thanks!

- Arnaldo