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  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]
Date:   Fri, 11 Jun 2021 13:57:37 +0000
From:   Maximilian Heyne <mheyne@...zon.de>
To:     SeongJae Park <sj38.park@...il.com>
CC:     <akpm@...ux-foundation.org>, SeongJae Park <sjpark@...zon.de>,
        <Jonathan.Cameron@...wei.com>, <acme@...nel.org>,
        <alexander.shishkin@...ux.intel.com>, <amit@...nel.org>,
        <benh@...nel.crashing.org>, <brendanhiggins@...gle.com>,
        <corbet@....net>, <david@...hat.com>, <dwmw@...zon.com>,
        <elver@...gle.com>, <fan.du@...el.com>, <foersleo@...zon.de>,
        <greg@...ah.com>, <gthelen@...gle.com>,
        <guoju.fgj@...baba-inc.com>, <mgorman@...e.de>,
        <minchan@...nel.org>, <mingo@...hat.com>, <namhyung@...nel.org>,
        <peterz@...radead.org>, <riel@...riel.com>, <rientjes@...gle.com>,
        <rostedt@...dmis.org>, <rppt@...nel.org>, <shakeelb@...gle.com>,
        <shuah@...nel.org>, <snu@...zon.de>, <vbabka@...e.cz>,
        <vdavydov.dev@...il.com>, <zgf574564920@...il.com>,
        <linux-damon@...zon.com>, <linux-mm@...ck.org>,
        <linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v29 12/13] mm/damon: Add user space selftests

On Thu, 20 May 2021 07:56:28 +0000 SeongJae Park <sj38.park@...il.com> wrote:

> From: SeongJae Park <sjpark@...zon.de>
> 
> This commit adds a simple user space tests for DAMON.  The tests are
> using kselftest framework.
> 
> Signed-off-by: SeongJae Park <sjpark@...zon.de>
> ---
>  tools/testing/selftests/damon/Makefile        |  7 ++
>  .../selftests/damon/_chk_dependency.sh        | 28 ++++++
>  .../testing/selftests/damon/debugfs_attrs.sh  | 98 +++++++++++++++++++
>  3 files changed, 133 insertions(+)
>  create mode 100644 tools/testing/selftests/damon/Makefile
>  create mode 100644 tools/testing/selftests/damon/_chk_dependency.sh
>  create mode 100755 tools/testing/selftests/damon/debugfs_attrs.sh
> 
> diff --git a/tools/testing/selftests/damon/Makefile b/tools/testing/selftests/damon/Makefile
> new file mode 100644
> index 000000000000..8a3f2cd9fec0
> --- /dev/null
> +++ b/tools/testing/selftests/damon/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Makefile for damon selftests
> +
> +TEST_FILES = _chk_dependency.sh
> +TEST_PROGS = debugfs_attrs.sh
> +
> +include ../lib.mk
> diff --git a/tools/testing/selftests/damon/_chk_dependency.sh b/tools/testing/selftests/damon/_chk_dependency.sh
> new file mode 100644
> index 000000000000..e090836c2bf7
> --- /dev/null
> +++ b/tools/testing/selftests/damon/_chk_dependency.sh
> @@ -0,0 +1,28 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# Kselftest framework requirement - SKIP code is 4.
> +ksft_skip=4
> +
> +DBGFS=/sys/kernel/debug/damon
> +
> +if [ $EUID -ne 0 ];
> +then
> +	echo "Run as root"
> +	exit $ksft_skip
> +fi
> +
> +if [ ! -d $DBGFS ]
> +then
> +	echo "$DBGFS not found"
> +	exit $ksft_skip
> +fi
> +
> +for f in attrs target_ids monitor_on
> +do
> +	if [ ! -f "$DBGFS/$f" ]
> +	then
> +		echo "$f not found"
> +		exit 1
> +	fi
> +done
> diff --git a/tools/testing/selftests/damon/debugfs_attrs.sh b/tools/testing/selftests/damon/debugfs_attrs.sh
> new file mode 100755
> index 000000000000..4a8ab4910ee4
> --- /dev/null
> +++ b/tools/testing/selftests/damon/debugfs_attrs.sh
> @@ -0,0 +1,98 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +source ./_chk_dependency.sh
> +
> +# Test attrs file
> +file="$DBGFS/attrs"
> +
> +ORIG_CONTENT=$(cat $file)

Missing quotes around $file. Can you run shellcheck on this code and fix all
reportings, please?

> +
> +echo 1 2 3 4 5 > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file write failed"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo 1 2 3 4 > $file
> +if [ $? -eq 0 ]
> +then
> +	echo "$file write success (should failed)"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +CONTENT=$(cat $file)
> +if [ "$CONTENT" != "1 2 3 4 5" ]
> +then
> +	echo "$file not written"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi

I'd add test cases for the contents written to the attrs, like checking that
input min_nr_regions is actually smaller than the input max_nr_regions values.

> +
> +echo $ORIG_CONTENT > $file
> +
> +# Test target_ids file
> +file="$DBGFS/target_ids"
> +
> +ORIG_CONTENT=$(cat $file)
> +
> +echo "1 2 3 4" > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file write fail"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo "1 2 abc 4" > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file write fail"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi

I've seen this construct more than once. Any chance to refactor this code? Or is
this selftest not expected to grow in the future?

> +
> +CONTENT=$(cat $file)
> +if [ "$CONTENT" != "1 2" ]
> +then
> +	echo "$file not written"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo abc 2 3 > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file wrong value write fail"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +if [ ! -z "$(cat $file)" ]
> +then
> +	echo "$file not cleared"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file init fail"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +if [ ! -z "$(cat $file)" ]
> +then
> +	echo "$file not initialized"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo $ORIG_CONTENT > $file
> +
> +echo "PASS"
> -- 
> 2.17.1
> 
> 
> 
> 



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



Powered by blists - more mailing lists