Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
To:     David Vernet <void@manifault.com>, Petr Mladek <pmladek@suse.com>
Cc:     live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        jpoimboe@redhat.com, jikos@kernel.org, mbenes@suse.cz,
        corbet@lwn.net, kernel-team@fb.com
References: <20220203233205.1554034-1-void@manifault.com>
 <20220204205625.2628328-1-void@manifault.com>
 <20220209130233.GB8279@pathway.suse.cz>
 <20220209154934.5w5k5tqcqldbfjt3@dev0025.ash9.facebook.com>
 <YgUt4+XGLOB4+0H3@alley>
 <20220210162215.oupcqlcglfpeqkbp@dev0025.ash9.facebook.com>
From:   Joe Lawrence <joe.lawrence@redhat.com>
Subject: Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be
 configured
Message-ID: <17674ab3-897d-3760-d86a-f89b5c0f1257@redhat.com>
Date:   Thu, 10 Feb 2022 15:58:49 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <20220210162215.oupcqlcglfpeqkbp@dev0025.ash9.facebook.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

Sorry for not catching this in v2. I think Petr's approach is pretty
reasonable, a few comments below.

On 2/10/22 11:22 AM, David Vernet wrote:
> 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

nit: I don't think the script is entirely consistent w/local variables, but:

local 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)

nit: likewise, local err and result variables.

>> +
>> +	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"

Unexpected failure: If the caller doesn't want this to fail but it does,
would it be helpful to report the original $err, too?  IIUC, the
original code captured that in $result.

>> +	fi
>> +

Unexpected success: Inversely, if the caller expects failure, but it
actually succeeds, I think this version only reports the new value, right?

In the case of the 2nd "set_ftrace_enabled 0" in test-ftrace.sh, that
turns out to be OK since it follows up with verifying that livepatch
redirection is still in effect.

Maybe this is too paranoid?  Just thought I'd mentioned it.

Thanks,

--
Joe

>> +	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@manifault.com>
>