From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751985AbdFTJ2X (ORCPT <rfc822;w@1wt.eu>);
        Tue, 20 Jun 2017 05:28:23 -0400
Received: from mail-ot0-f180.google.com ([74.125.82.180]:34534 "EHLO
        mail-ot0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751018AbdFTJ2U (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 20 Jun 2017 05:28:20 -0400
MIME-Version: 1.0
In-Reply-To: <CAGXu5j+S7FOJEW+xp-3wsQOiLZtMVZjRDSDnPU=xQr3+Mjuxeg@mail.gmail.com>
References: <20170616042255.22653-1-orson.zhai@spreadtrum.com>
 <CAPPenn_E8=8tToEi6QtSq+STWYhTMztA=P=gvxUZBydUSQwz7Q@mail.gmail.com> <CAGXu5j+S7FOJEW+xp-3wsQOiLZtMVZjRDSDnPU=xQr3+Mjuxeg@mail.gmail.com>
From: Orson Zhai <orson.zhai@linaro.org>
Date: Tue, 20 Jun 2017 17:28:19 +0800
Message-ID: <CAPPenn8GXMHHUpe5yA1Cx1wg7qCK3m+7vn-bh2RaQEjnSLJpCA@mail.gmail.com>
Subject: Re: [PATCH] tools/testing/selftests/sysctl: Add pre-check to the
 value of writes_strict
To: Kees Cook <keescook@chromium.org>
Cc: Shuah Khan <Shuah@kernel.org>, linux-kselftest@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Sumit Semwal <sumit.semwal@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Kees,

Thanks for reviewing.

I will update in V1 soon.

Br,
Orson

On 20 June 2017 at 03:01, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Jun 16, 2017 at 2:28 AM, Orson Zhai <orson.zhai@linaro.org> wrote:
>> Sysctl test will fail in some items if the value of /proc/sys/kernel
>> /sysctrl_writes_strict is 0 as the default value in kernel older than v4.5.
>>
>> Make this test more robust and compatible with older kernels by checking and
>> update sysctrl_writes_strict value and restore it when test is done.
>>
>> Signed-off-by: Orson Zhai <orson.zhai@linaro.org>
>> ---
>>  tools/testing/selftests/sysctl/common_tests     | 14 ++++++++++++++
>>  tools/testing/selftests/sysctl/run_numerictests |  3 +++
>>  tools/testing/selftests/sysctl/run_stringtests  |  3 +++
>>  3 files changed, 20 insertions(+)
>>
>> diff --git a/tools/testing/selftests/sysctl/common_tests
>> b/tools/testing/selftests/sysctl/common_tests
>> index 17d534b1b7b4..f5c5c51d16f3 100644
>> --- a/tools/testing/selftests/sysctl/common_tests
>> +++ b/tools/testing/selftests/sysctl/common_tests
>> @@ -63,6 +63,20 @@ else
>>         echo "ok"
>>  fi
>>
>> +echo -n "Checking writes strict setting ... "
>> +WRITES_STRICT="${SYSCTL}/kernel/sysctl_writes_strict"
>> +if [ ! -e ${WRITES_STRICT} ]; then
>> +       echo "FAIL, but skip in case of old kernel" >&2
>> +else
>> +       val=$(cat ${WRITES_STRICT})
>
> Can you give this a more meaningful name instead of "val". Perhaps "old_strict"?
>
>> +       if [ "$val" = "1" ]; then
>> +               echo "ok"
>> +       else
>> +               echo "FAIL, strict value is 0 but force to 1 to continue" >&2
>> +               echo "1" > ${WRITES_STRICT}
>> +       fi
>> +fi
>> +
>>  # Now that we've validated the sanity of "set_test" and "set_orig",
>>  # we can use those functions to set starting states before running
>>  # specific behavioral tests.
>> diff --git a/tools/testing/selftests/sysctl/run_numerictests
>> b/tools/testing/selftests/sysctl/run_numerictests
>> index 8510f93f2d14..c0a98fd82c5c 100755
>> --- a/tools/testing/selftests/sysctl/run_numerictests
>> +++ b/tools/testing/selftests/sysctl/run_numerictests
>> @@ -7,4 +7,7 @@ TEST_STR=$(( $ORIG + 1 ))
>>
>>  . ./common_tests
>>
>> +if [ ! -z ${val} ]; then
>> +       echo ${val} > ${WRITES_STRICT}
>> +fi
>>  exit $rc
>> diff --git a/tools/testing/selftests/sysctl/run_stringtests
>> b/tools/testing/selftests/sysctl/run_stringtests
>> index 90a9293d520c..ae98d66a9ec6 100755
>> --- a/tools/testing/selftests/sysctl/run_stringtests
>> +++ b/tools/testing/selftests/sysctl/run_stringtests
>> @@ -74,4 +74,7 @@ else
>>         echo "ok"
>>  fi
>>
>> +if [ ! -z ${val} ]; then
>> +       echo ${val} > ${WRITES_STRICT}
>> +fi
>>  exit $rc
>
> And instead of open-coding these, can you instead add an "exit_test"
> function to common_tests, which will do this and the exit $rc?
>
> -Kees
>
> --
> Kees Cook
> Pixel Security