linux-kernel - Re: [PATCH] selftests: livepatch: test if ftrace can trace a livepatched function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9edd671a-be5e-41c9-a8dc-b1fd1d5e3375@gmail.com>
Date: Fri, 10 Jan 2025 14:13:24 -0300
From: Filipe Xavier <felipeaggger@...il.com>
To: Joe Lawrence <joe.lawrence@...hat.com>
Cc: Marcos Paulo de Souza <mpdesouza@...e.com>,
 Josh Poimboeuf <jpoimboe@...nel.org>, Jiri Kosina <jikos@...nel.org>,
 Miroslav Benes <mbenes@...e.cz>, Petr Mladek <pmladek@...e.com>,
 Shuah Khan <shuah@...nel.org>, live-patching@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 Felipe Xavier <felipe_life@...e.com>
Subject: Re: [PATCH] selftests: livepatch: test if ftrace can trace a
 livepatched function

Em 07/01/2025 13:23, Joe Lawrence escreveu:

> On Thu, Jan 02, 2025 at 03:42:10PM -0300, Filipe Xavier wrote:
>> This new test makes sure that ftrace can trace a
>> function that was introduced by a livepatch.
>>
> Hi Filipe,
>
> Thanks for adding a test!
>
> Aside: another similar test could verify that the original function, in
> this case cmdline_proc_show(), can still be traced despite it being
> livepatched.  That may be non-intuitive but it demonstrates how the
> ftrace handler works.

Thanks for the review Joe!

I have fixed all points mentioned below,

and have a patch ready to submit.

Do you believe that this other similar test could be sent later,

or is it required in this patch?

>   
>> Signed-off-by: Filipe Xavier <felipeaggger@...il.com>
>> ---
>>   tools/testing/selftests/livepatch/test-ftrace.sh | 37 ++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh
>> index fe14f248913acbec46fb6c0fec38a2fc84209d39..5f0d5308c88669e84210393ce7b8aa138b694ebd 100755
>> --- a/tools/testing/selftests/livepatch/test-ftrace.sh
>> +++ b/tools/testing/selftests/livepatch/test-ftrace.sh
>> @@ -61,4 +61,41 @@ livepatch: '$MOD_LIVEPATCH': unpatching complete
>>   % rmmod $MOD_LIVEPATCH"
>>   
>>   
>> +# - verify livepatch can load
>> +# - check traces if have a patched function
> nit: wording?  "check if traces have a patched function" ?
>
>> +# - unload livepatch and reset trace
>> +
>> +start_test "livepatch trace patched function and check that the live patch remains in effect"
> nit: wording?  "trace livepatched function and check ..." ?
>
>> +
>> +TRACE_FILE="$SYSFS_DEBUG_DIR/tracing/trace"
>> +FUNCTION_NAME="livepatch_cmdline_proc_show"
>> +
>> +load_lp $MOD_LIVEPATCH
>> +
>> +echo $FUNCTION_NAME > $SYSFS_DEBUG_DIR/tracing/set_ftrace_filter
>> +echo "function" > $SYSFS_DEBUG_DIR/tracing/current_tracer
>> +echo "" > $TRACE_FILE
> A few suggestions:
>
> - The tracing is also dependent on the 'tracing_on' file, so if it
>    happens to be turned off, the test will fail.
>
> - See functions.sh :: push_config() and pop_config() for an example of
>    saving the existing values rather than turning them all off at the end
>    of the test.
>
> - Nitpick: shellcheck suggests wrapping filenames in double quotations,
>    applicable in several places.
>
>> +
>> +if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then
>> +	echo -e "FAIL\n\n"
>> +	die "livepatch kselftest(s) failed"
>> +fi
>> +
>> +grep -q $FUNCTION_NAME $TRACE_FILE
>> +FOUND=$?
>> +
>> +disable_lp $MOD_LIVEPATCH
>> +unload_lp $MOD_LIVEPATCH
>> +
>> +# Reset tracing
>> +echo "nop" > $SYSFS_DEBUG_DIR/tracing/current_tracer
>> +echo "" > $SYSFS_DEBUG_DIR/tracing/set_ftrace_filter
>> +echo "" > $TRACE_FILE
>> +
>> +if [ "$FOUND" -eq 1 ]; then
>> +	echo -e "FAIL\n\n"
>> +	die "livepatch kselftest(s) failed"
>> +fi
>> +
>> +
>>   exit 0
>>
>> ---
>> base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
>> change-id: 20250101-ftrace-selftest-livepatch-161fb77dbed8
>>
>> Best regards,
>> -- 
>> Filipe Xavier <felipeaggger@...il.com>
>>
> Thanks,
> --
> Joe
>