[PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Song Liu]

CONFIG_KPROBES_ON_FTRACE is required for test-kprobe. Skip test-kprobe
when CONFIG_KPROBES_ON_FTRACE is not set.

Signed-off-by: Song Liu <song@kernel.org>
---
 tools/testing/selftests/livepatch/test-kprobe.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
index 115065156016..fd823dd5dd7f 100755
--- a/tools/testing/selftests/livepatch/test-kprobe.sh
+++ b/tools/testing/selftests/livepatch/test-kprobe.sh
@@ -5,6 +5,8 @@
 
 . $(dirname $0)/functions.sh
 
+zgrep KPROBES_ON_FTRACE /proc/config.gz || skip "test-kprobe requires CONFIG_KPROBES_ON_FTRACE"
+
 MOD_LIVEPATCH=test_klp_livepatch
 MOD_KPROBE=test_klp_kprobe
 
-- 
2.47.1

Re: [PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Joe Lawrence]

On 3/17/25 12:51, Song Liu wrote:
> CONFIG_KPROBES_ON_FTRACE is required for test-kprobe. Skip test-kprobe
> when CONFIG_KPROBES_ON_FTRACE is not set.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  tools/testing/selftests/livepatch/test-kprobe.sh | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
> index 115065156016..fd823dd5dd7f 100755
> --- a/tools/testing/selftests/livepatch/test-kprobe.sh
> +++ b/tools/testing/selftests/livepatch/test-kprobe.sh
> @@ -5,6 +5,8 @@
>  
>  . $(dirname $0)/functions.sh
>  
> +zgrep KPROBES_ON_FTRACE /proc/config.gz || skip "test-kprobe requires CONFIG_KPROBES_ON_FTRACE"
> +

Hi Song,

This in turn depends on CONFIG_IKCONFIG_PROC for /proc/config.gz (not
set for RHEL distro kernels).

Is there a dynamic way to figure out CONFIG_KPROBES_ON_FTRACE support?
Without looking into it very long, maybe test_klp_kprobe.c's call to
register_kprobe() could fail with -ENOTSUPP and the test script could
gracefully skip the test?

Regards,

-- 
Joe

Re: [PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Joe Lawrence]

On 3/17/25 14:59, Joe Lawrence wrote:
> On 3/17/25 12:51, Song Liu wrote:
>> CONFIG_KPROBES_ON_FTRACE is required for test-kprobe. Skip test-kprobe
>> when CONFIG_KPROBES_ON_FTRACE is not set.
>>
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>>  tools/testing/selftests/livepatch/test-kprobe.sh | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
>> index 115065156016..fd823dd5dd7f 100755
>> --- a/tools/testing/selftests/livepatch/test-kprobe.sh
>> +++ b/tools/testing/selftests/livepatch/test-kprobe.sh
>> @@ -5,6 +5,8 @@
>>  
>>  . $(dirname $0)/functions.sh
>>  
>> +zgrep KPROBES_ON_FTRACE /proc/config.gz || skip "test-kprobe requires CONFIG_KPROBES_ON_FTRACE"
>> +
> 
> Hi Song,
> 
> This in turn depends on CONFIG_IKCONFIG_PROC for /proc/config.gz (not
> set for RHEL distro kernels).
> 
> Is there a dynamic way to figure out CONFIG_KPROBES_ON_FTRACE support?
> Without looking into it very long, maybe test_klp_kprobe.c's call to
> register_kprobe() could fail with -ENOTSUPP and the test script could
> gracefully skip the test?
> 

Ugh, n/m, looking at the code now I see it already does return
-EOPNOTSUPP, but insmod throws out the code (if it even gets that far)
and so it only reports success or failure.  Graceful handling will have
to be more clever than that.

-- 
Joe

Re: [PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Song Liu]

On Mon, Mar 17, 2025 at 11:59 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On 3/17/25 12:51, Song Liu wrote:
> > CONFIG_KPROBES_ON_FTRACE is required for test-kprobe. Skip test-kprobe
> > when CONFIG_KPROBES_ON_FTRACE is not set.
> >
> > Signed-off-by: Song Liu <song@kernel.org>
> > ---
> >  tools/testing/selftests/livepatch/test-kprobe.sh | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
> > index 115065156016..fd823dd5dd7f 100755
> > --- a/tools/testing/selftests/livepatch/test-kprobe.sh
> > +++ b/tools/testing/selftests/livepatch/test-kprobe.sh
> > @@ -5,6 +5,8 @@
> >
> >  . $(dirname $0)/functions.sh
> >
> > +zgrep KPROBES_ON_FTRACE /proc/config.gz || skip "test-kprobe requires CONFIG_KPROBES_ON_FTRACE"
> > +
>
> Hi Song,
>
> This in turn depends on CONFIG_IKCONFIG_PROC for /proc/config.gz (not
> set for RHEL distro kernels).

I was actually worrying about this when testing it.

> Is there a dynamic way to figure out CONFIG_KPROBES_ON_FTRACE support?

How about we grep kprobe_ftrace_ops from /proc/kallsyms?

Thanks,
Song

> Without looking into it very long, maybe test_klp_kprobe.c's call to
> register_kprobe() could fail with -ENOTSUPP and the test script could
> gracefully skip the test?
>
> Regards,
>
> --
> Joe
>

Re: [PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Miroslav Benes]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

Hi,

On Mon, 17 Mar 2025, Song Liu wrote:

> On Mon, Mar 17, 2025 at 11:59 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> >
> > On 3/17/25 12:51, Song Liu wrote:
> > > CONFIG_KPROBES_ON_FTRACE is required for test-kprobe. Skip test-kprobe
> > > when CONFIG_KPROBES_ON_FTRACE is not set.
> > >
> > > Signed-off-by: Song Liu <song@kernel.org>
> > > ---
> > >  tools/testing/selftests/livepatch/test-kprobe.sh | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
> > > index 115065156016..fd823dd5dd7f 100755
> > > --- a/tools/testing/selftests/livepatch/test-kprobe.sh
> > > +++ b/tools/testing/selftests/livepatch/test-kprobe.sh
> > > @@ -5,6 +5,8 @@
> > >
> > >  . $(dirname $0)/functions.sh
> > >
> > > +zgrep KPROBES_ON_FTRACE /proc/config.gz || skip "test-kprobe requires CONFIG_KPROBES_ON_FTRACE"
> > > +
> >
> > Hi Song,
> >
> > This in turn depends on CONFIG_IKCONFIG_PROC for /proc/config.gz (not
> > set for RHEL distro kernels).
> 
> I was actually worrying about this when testing it.
> 
> > Is there a dynamic way to figure out CONFIG_KPROBES_ON_FTRACE support?
> 
> How about we grep kprobe_ftrace_ops from /proc/kallsyms?

which relies on having KALLSYMS_ALL enabled but since CONFIG_LIVEPATCH 
depends on it, we are good. So I would say yes, it is a better option. 
Please, add a comment around.

Thank you,
Miroslav

Re: [PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Joe Lawrence]

On Tue, Mar 18, 2025 at 11:14:55AM +0100, Miroslav Benes wrote:
> Hi,
> 
> On Mon, 17 Mar 2025, Song Liu wrote:
> 
> > On Mon, Mar 17, 2025 at 11:59 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > >
> > > On 3/17/25 12:51, Song Liu wrote:
> > > > CONFIG_KPROBES_ON_FTRACE is required for test-kprobe. Skip test-kprobe
> > > > when CONFIG_KPROBES_ON_FTRACE is not set.
> > > >
> > > > Signed-off-by: Song Liu <song@kernel.org>
> > > > ---
> > > >  tools/testing/selftests/livepatch/test-kprobe.sh | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
> > > > index 115065156016..fd823dd5dd7f 100755
> > > > --- a/tools/testing/selftests/livepatch/test-kprobe.sh
> > > > +++ b/tools/testing/selftests/livepatch/test-kprobe.sh
> > > > @@ -5,6 +5,8 @@
> > > >
> > > >  . $(dirname $0)/functions.sh
> > > >
> > > > +zgrep KPROBES_ON_FTRACE /proc/config.gz || skip "test-kprobe requires CONFIG_KPROBES_ON_FTRACE"
> > > > +
> > >
> > > Hi Song,
> > >
> > > This in turn depends on CONFIG_IKCONFIG_PROC for /proc/config.gz (not
> > > set for RHEL distro kernels).
> > 
> > I was actually worrying about this when testing it.
> > 
> > > Is there a dynamic way to figure out CONFIG_KPROBES_ON_FTRACE support?
> > 
> > How about we grep kprobe_ftrace_ops from /proc/kallsyms?
> 
> which relies on having KALLSYMS_ALL enabled but since CONFIG_LIVEPATCH 
> depends on it, we are good. So I would say yes, it is a better option. 
> Please, add a comment around.
> 

Kallsyms is a good workaround as kprobe_ftrace_ops should be stable (I
hope :)

Since Song probably noticed this when upgrading and the new kprobes test
unexpectedly failed, I'd add a Fixes tag to help out backporters:

  Fixes: 62597edf6340 ("selftests: livepatch: test livepatching a kprobed function")

but IMHO not worth rushing as important through the merge window.


Also, while poking at this today with virtme-ng, my initial attempt had
build a fairly minimal kernel without CONFIG_DYNAMIC_DEBUG.  We can also
check for that via the sysfs interface to avoid confusing path-not-found
msgs, like the following ...

-- Joe

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

From c71f1966cfb7311bb1dde90c9f1be40704a253b6 Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence@redhat.com>
Date: Tue, 18 Mar 2025 09:26:55 -0400
Subject: [PATCH] selftests/livepatch: check for CONFIG_DYNAMIC_DEBUG
Content-type: text/plain

The livepatch kselftests set and restore dynamic debug to verify
subsystem behavior through expected printk messages.  The tests should
first check that this facility is available, otherwise just skip
running.

Fixes: a2818ee4dce5 ("selftests/livepatch: introduce tests")
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 tools/testing/selftests/livepatch/functions.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index 15601402dee6..92bc083225a6 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -58,6 +58,9 @@ function die() {
 }
 
 function push_config() {
+	if [[ ! -e "$SYSFS_DEBUG_DIR/dynamic_debug" ]]; then
+		skip "test requires CONFIG_DYNAMIC_DEBUG"
+	fi
 	DYNAMIC_DEBUG=$(grep '^kernel/livepatch' "$SYSFS_DEBUG_DIR/dynamic_debug/control" | \
 			awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}')
 	FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
-- 
2.42.0

Re: [PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Song Liu]

On Tue, Mar 18, 2025 at 6:45 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>
> On Tue, Mar 18, 2025 at 11:14:55AM +0100, Miroslav Benes wrote:
> > Hi,
> >
> > On Mon, 17 Mar 2025, Song Liu wrote:
> >
> > > On Mon, Mar 17, 2025 at 11:59 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
> > > >
> > > > On 3/17/25 12:51, Song Liu wrote:
> > > > > CONFIG_KPROBES_ON_FTRACE is required for test-kprobe. Skip test-kprobe
> > > > > when CONFIG_KPROBES_ON_FTRACE is not set.
> > > > >
> > > > > Signed-off-by: Song Liu <song@kernel.org>
> > > > > ---
> > > > >  tools/testing/selftests/livepatch/test-kprobe.sh | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
> > > > > index 115065156016..fd823dd5dd7f 100755
> > > > > --- a/tools/testing/selftests/livepatch/test-kprobe.sh
> > > > > +++ b/tools/testing/selftests/livepatch/test-kprobe.sh
> > > > > @@ -5,6 +5,8 @@
> > > > >
> > > > >  . $(dirname $0)/functions.sh
> > > > >
> > > > > +zgrep KPROBES_ON_FTRACE /proc/config.gz || skip "test-kprobe requires CONFIG_KPROBES_ON_FTRACE"
> > > > > +
> > > >
> > > > Hi Song,
> > > >
> > > > This in turn depends on CONFIG_IKCONFIG_PROC for /proc/config.gz (not
> > > > set for RHEL distro kernels).
> > >
> > > I was actually worrying about this when testing it.
> > >
> > > > Is there a dynamic way to figure out CONFIG_KPROBES_ON_FTRACE support?
> > >
> > > How about we grep kprobe_ftrace_ops from /proc/kallsyms?
> >
> > which relies on having KALLSYMS_ALL enabled but since CONFIG_LIVEPATCH
> > depends on it, we are good. So I would say yes, it is a better option.
> > Please, add a comment around.
> >
>
> Kallsyms is a good workaround as kprobe_ftrace_ops should be stable (I
> hope :)
>
> Since Song probably noticed this when upgrading and the new kprobes test
> unexpectedly failed, I'd add a Fixes tag to help out backporters:
>
>   Fixes: 62597edf6340 ("selftests: livepatch: test livepatching a kprobed function")
>
> but IMHO not worth rushing as important through the merge window.
>
>
> Also, while poking at this today with virtme-ng, my initial attempt had
> build a fairly minimal kernel without CONFIG_DYNAMIC_DEBUG.  We can also
> check for that via the sysfs interface to avoid confusing path-not-found
> msgs, like the following ...

We already have CONFIG_DYNAMIC_DEBUG=y in
tools/testing/selftests/livepatch, so I think we don't need to check that again.

We cannot do the same for CONFIG_KPROBES_ON_FTRACE, because
we still want to run other tests on systems that do not support
CONFIG_KPROBES_ON_FTRACE.

Thanks,
Song

Re: [PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Joe Lawrence]

On 3/18/25 12:25, Song Liu wrote:
> On Tue, Mar 18, 2025 at 6:45 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>>
>> On Tue, Mar 18, 2025 at 11:14:55AM +0100, Miroslav Benes wrote:
>>> Hi,
>>>
>>> On Mon, 17 Mar 2025, Song Liu wrote:
>>>
>>>> On Mon, Mar 17, 2025 at 11:59 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
>>>>>
>>>>> On 3/17/25 12:51, Song Liu wrote:
>>>>>> CONFIG_KPROBES_ON_FTRACE is required for test-kprobe. Skip test-kprobe
>>>>>> when CONFIG_KPROBES_ON_FTRACE is not set.
>>>>>>
>>>>>> Signed-off-by: Song Liu <song@kernel.org>
>>>>>> ---
>>>>>>  tools/testing/selftests/livepatch/test-kprobe.sh | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
>>>>>> index 115065156016..fd823dd5dd7f 100755
>>>>>> --- a/tools/testing/selftests/livepatch/test-kprobe.sh
>>>>>> +++ b/tools/testing/selftests/livepatch/test-kprobe.sh
>>>>>> @@ -5,6 +5,8 @@
>>>>>>
>>>>>>  . $(dirname $0)/functions.sh
>>>>>>
>>>>>> +zgrep KPROBES_ON_FTRACE /proc/config.gz || skip "test-kprobe requires CONFIG_KPROBES_ON_FTRACE"
>>>>>> +
>>>>>
>>>>> Hi Song,
>>>>>
>>>>> This in turn depends on CONFIG_IKCONFIG_PROC for /proc/config.gz (not
>>>>> set for RHEL distro kernels).
>>>>
>>>> I was actually worrying about this when testing it.
>>>>
>>>>> Is there a dynamic way to figure out CONFIG_KPROBES_ON_FTRACE support?
>>>>
>>>> How about we grep kprobe_ftrace_ops from /proc/kallsyms?
>>>
>>> which relies on having KALLSYMS_ALL enabled but since CONFIG_LIVEPATCH
>>> depends on it, we are good. So I would say yes, it is a better option.
>>> Please, add a comment around.
>>>
>>
>> Kallsyms is a good workaround as kprobe_ftrace_ops should be stable (I
>> hope :)
>>
>> Since Song probably noticed this when upgrading and the new kprobes test
>> unexpectedly failed, I'd add a Fixes tag to help out backporters:
>>
>>   Fixes: 62597edf6340 ("selftests: livepatch: test livepatching a kprobed function")
>>
>> but IMHO not worth rushing as important through the merge window.
>>
>>
>> Also, while poking at this today with virtme-ng, my initial attempt had
>> build a fairly minimal kernel without CONFIG_DYNAMIC_DEBUG.  We can also
>> check for that via the sysfs interface to avoid confusing path-not-found
>> msgs, like the following ...
> 
> We already have CONFIG_DYNAMIC_DEBUG=y in
> tools/testing/selftests/livepatch, so I think we don't need to check that again.
> 

Ah right.  Do you know what script/file reads or sources that file?
(It's such a generic term that I get a bazillion grep hits.)  When I run
`make -C tools/testing/selftests/livepatch run_tests` it just builds and
runs the tests regardless of the actual config.

-- 
Joe

Re: [PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Song Liu]

On Tue, Mar 18, 2025 at 10:18 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
[...]
> >> Also, while poking at this today with virtme-ng, my initial attempt had
> >> build a fairly minimal kernel without CONFIG_DYNAMIC_DEBUG.  We can also
> >> check for that via the sysfs interface to avoid confusing path-not-found
> >> msgs, like the following ...
> >
> > We already have CONFIG_DYNAMIC_DEBUG=y in
> > tools/testing/selftests/livepatch, so I think we don't need to check that again.
> >
>
> Ah right.  Do you know what script/file reads or sources that file?
> (It's such a generic term that I get a bazillion grep hits.)  When I run
> `make -C tools/testing/selftests/livepatch run_tests` it just builds and
> runs the tests regardless of the actual config.

I am not aware of any script reading the config file. It appears to be a
convention that many subsystems follow. The subsystems may have
some CI that reads these config files. Some subsystems use more
complex logic around these config files. For example, bpf selftests
have 6 config files and 4 DENYLIST.

Thanks,
Song

Re: [PATCH] selftest/livepatch: Only run test-kprobe with CONFIG_KPROBES_ON_FTRACE

[Song Liu]

On Tue, Mar 18, 2025 at 6:45 AM Joe Lawrence <joe.lawrence@redhat.com> wrote:
[...]
> Kallsyms is a good workaround as kprobe_ftrace_ops should be stable (I
> hope :)
>
> Since Song probably noticed this when upgrading and the new kprobes test
> unexpectedly failed, I'd add a Fixes tag to help out backporters:
>
>   Fixes: 62597edf6340 ("selftests: livepatch: test livepatching a kprobed function")
>
> but IMHO not worth rushing as important through the merge window.

I actually think this fix is simple enough that I would not worry about it
breaking anything in 6.15. But if other folks agree this is too late for 6.15,
I don't mind waiting until 6.16.

Thanks,
Song