linux-kernel - Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220210162215.oupcqlcglfpeqkbp@dev0025.ash9.facebook.com>
Date:   Thu, 10 Feb 2022 08:22:15 -0800
From:   David Vernet <void@...ifault.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        jpoimboe@...hat.com, jikos@...nel.org, mbenes@...e.cz,
        joe.lawrence@...hat.com, corbet@....net, kernel-team@...com
Subject: Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be
 configured

On Thu, Feb 10, 2022 at 04:23:15PM +0100, Petr Mladek wrote:
> OK, what about the following change:
> 
>    1. Store only the value (number) in $result
>    2. Add optional --fail parameter

I'm fine with this approach. I agree that it's probably less confusing than
"$2" == "$1". I don't think the elif check I proposed is strictly necessary
either, and in any case the way you've rewired the patch address that.

> 
> Hmm, the 1st step is not needed after several iterations here ;-)
> Anyway, it should be easier to maintain it in the long term when the
> sysctl output might change. We should probably do it in a separate patch.
>
>
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 846c7ed71556..7b624d0fd7c0 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -75,9 +75,25 @@ function set_dynamic_debug() {
>  }
>  
>  function set_ftrace_enabled() {
> -	result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
> -		 sysctl kernel.ftrace_enabled 2>&1)
> -	echo "livepatch: $result" > /dev/kmsg
> +	can_fail=0

Can you make this variable a local?

> +	if [[ "$1" == "--fail" ]] ; then
> +		can_fail=1
> +		shift
> +	fi
> +
> +	err=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1)
> +	result=$(sysctl --values kernel.ftrace_enabled)
> +
> +	if [[ "$result" != "$1" ]] ; then
> +		if [[ $can_fail -eq 1 ]] ; then
> +			echo "livepatch: $err" > /dev/kmsg
> +			return
> +		fi
> +
> +		skip "failed to set kernel.ftrace_enabled = $1"
> +	fi
> +
> +	echo "livepatch: kernel.ftrace_enabled = $result" > /dev/kmsg
>  }
>  
>  function cleanup() {
> diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh
> index 552e165512f4..825540a5194d 100755
> --- a/tools/testing/selftests/livepatch/test-ftrace.sh
> +++ b/tools/testing/selftests/livepatch/test-ftrace.sh
> @@ -25,7 +25,8 @@ if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]]
>  	die "livepatch kselftest(s) failed"
>  fi
>  
> -set_ftrace_enabled 0
> +# Check that ftrace could not get disabled when a livepatch is enabled
> +set_ftrace_enabled --fail 0
>  if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then
>  	echo -e "FAIL\n\n"
>  	die "livepatch kselftest(s) failed"

Patch looks great otherwise and works fine after testing on my end. Do you
want me to send it out as a v3? Or would you prefer to just apply it as is?
Assuming it's the latter, then:

Reviewed-by: David Vernet <void@...ifault.com>

Thanks,
David