Received-SPF: pass (google.com: domain of linux-kernel+bounces-56500-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 7 Feb 2024 04:53:35 -0800
From: Kees Cook <keescook@chromium.org>
To: Yuanhe Shu <xiangzao@linux.alibaba.com>
Cc: tony.luck@intel.com, gpiccoli@igalia.com, shuah@kernel.org,
	corbet@lwn.net, xlpang@linux.alibaba.com,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-hardening@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH 3/3] tools/testing: adjust pstore backend related selftest
Message-ID: <202402070452.24B3200@keescook>
References: <20240207021921.206425-1-xiangzao@linux.alibaba.com>
 <20240207021921.206425-4-xiangzao@linux.alibaba.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240207021921.206425-4-xiangzao@linux.alibaba.com>

On Wed, Feb 07, 2024 at 10:19:21AM +0800, Yuanhe Shu wrote:
> Pstore now supports multiple backends, the module parameter
> pstore.backend varies from 'registered backend' to 'backends that are
> allowed to register'. Adjust selftests to match the change.
> 
> Signed-off-by: Yuanhe Shu <xiangzao@linux.alibaba.com>
> ---
>  tools/testing/selftests/pstore/common_tests   |  8 +--
>  .../selftests/pstore/pstore_post_reboot_tests | 65 ++++++++++---------
>  tools/testing/selftests/pstore/pstore_tests   |  2 +-
>  3 files changed, 38 insertions(+), 37 deletions(-)
> 
> diff --git a/tools/testing/selftests/pstore/common_tests b/tools/testing/selftests/pstore/common_tests
> index 4509f0cc9c91..497e6fc3215f 100755
> --- a/tools/testing/selftests/pstore/common_tests
> +++ b/tools/testing/selftests/pstore/common_tests
> @@ -27,9 +27,9 @@ show_result() { # result_value
>  }
>  
>  check_files_exist() { # type of pstorefs file
> -    if [ -e ${1}-${backend}-0 ]; then
> +    if [ -e ${1}-${2}-0 ]; then
>  	prlog "ok"
> -	for f in `ls ${1}-${backend}-*`; do
> +	for f in `ls ${1}-${2}-*`; do
>              prlog -e "\t${f}"
>  	done
>      else
> @@ -74,9 +74,9 @@ prlog "=== Pstore unit tests (`basename $0`) ==="
>  prlog "UUID="$UUID
>  
>  prlog -n "Checking pstore backend is registered ... "
> -backend=`cat /sys/module/pstore/parameters/backend`
> +backends=$(dmesg | sed -n 's/.*pstore: Registered \(.*\) as persistent store backend.*/\1/p')
>  show_result $?
> -prlog -e "\tbackend=${backend}"
> +prlog -e "\tbackends="$backends

Missing trailing "? Also, doesn't this end up printing multiple lines?
Perhaps, like LSM stacking, we need a /sys/module entry for the list of
backends, comma separated?

>  prlog -e "\tcmdline=`cat /proc/cmdline`"
>  if [ $rc -ne 0 ]; then
>      exit 1
> diff --git a/tools/testing/selftests/pstore/pstore_post_reboot_tests b/tools/testing/selftests/pstore/pstore_post_reboot_tests
> index d6da5e86efbf..9e40ccb9c918 100755
> --- a/tools/testing/selftests/pstore/pstore_post_reboot_tests
> +++ b/tools/testing/selftests/pstore/pstore_post_reboot_tests
> @@ -36,45 +36,46 @@ else
>  fi
>  
>  cd ${mount_point}
> +for backend in ${backends}; do
> +    prlog -n "Checking ${backend}-dmesg files exist in pstore filesystem ... "
> +    check_files_exist dmesg ${backend}
>  
> -prlog -n "Checking dmesg files exist in pstore filesystem ... "
> -check_files_exist dmesg
> +    prlog -n "Checking ${backend}-console files exist in pstore filesystem ... "
> +    check_files_exist console ${backend}
>  
> -prlog -n "Checking console files exist in pstore filesystem ... "
> -check_files_exist console
> +    prlog -n "Checking ${backend}-pmsg files exist in pstore filesystem ... "
> +    check_files_exist pmsg ${backend}
>  
> -prlog -n "Checking pmsg files exist in pstore filesystem ... "
> -check_files_exist pmsg
> +    prlog -n "Checking ${backend}-dmesg files contain oops end marker"
> +    grep_end_trace() {
> +        grep -q "\---\[ end trace" $1
> +    }
> +    files=`ls dmesg-${backend}-*`
> +    operate_files $? "$files" grep_end_trace
>  
> -prlog -n "Checking dmesg files contain oops end marker"
> -grep_end_trace() {
> -    grep -q "\---\[ end trace" $1
> -}
> -files=`ls dmesg-${backend}-*`
> -operate_files $? "$files" grep_end_trace
> +    prlog -n "Checking ${backend}-console file contains oops end marker ... "
> +    grep -q "\---\[ end trace" console-${backend}-0
> +    show_result $?
>  
> -prlog -n "Checking console file contains oops end marker ... "
> -grep -q "\---\[ end trace" console-${backend}-0
> -show_result $?
> -
> -prlog -n "Checking pmsg file properly keeps the content written before crash ... "
> -prev_uuid=`cat $TOP_DIR/prev_uuid`
> -if [ $? -eq 0 ]; then
> -    nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
> -    if [ $nr_matched -eq 1 ]; then
> -	grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
> -	show_result $?
> +    prlog -n "Checking ${backend}-pmsg file properly keeps the content written before crash ... "
> +    prev_uuid=`cat $TOP_DIR/prev_uuid`
> +    if [ $? -eq 0 ]; then
> +        nr_matched=`grep -c "$TEST_STRING_PATTERN" pmsg-${backend}-0`
> +        if [ $nr_matched -eq 1 ]; then
> +	    grep -q "$TEST_STRING_PATTERN"$prev_uuid pmsg-${backend}-0
> +	    show_result $?
> +        else
> +            prlog "FAIL"
> +            rc=1
> +        fi
>      else
> -	prlog "FAIL"
> -	rc=1
> +        prlog "FAIL"
> +        rc=1
>      fi
> -else
> -    prlog "FAIL"
> -    rc=1
> -fi
>  
> -prlog -n "Removing all files in pstore filesystem "
> -files=`ls *-${backend}-*`
> -operate_files $? "$files" rm
> +    prlog -n "Removing all ${backend} files in pstore filesystem "
> +    files=`ls *-${backend}-*`
> +    operate_files $? "$files" rm
> +done
>  
>  exit $rc
> diff --git a/tools/testing/selftests/pstore/pstore_tests b/tools/testing/selftests/pstore/pstore_tests
> index 2aa9a3852a84..f4665a8c77dc 100755
> --- a/tools/testing/selftests/pstore/pstore_tests
> +++ b/tools/testing/selftests/pstore/pstore_tests
> @@ -10,7 +10,7 @@
>  . ./common_tests
>  
>  prlog -n "Checking pstore console is registered ... "
> -dmesg | grep -Eq "console \[(pstore|${backend})"
> +dmesg | grep -Eq "console \[(pstore console)"
>  show_result $?
>  
>  prlog -n "Checking /dev/pmsg0 exists ... "
> -- 
> 2.39.3
> 

Otherwise seems ok

-- 
Kees Cook