[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Jan Stancek]

library doesn't support multiple parameters for same option.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 runtest/fs                              |  2 +-
 testcases/kernel/fs/read_all/read_all.c | 28 ++++++++++++++++++++++------
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/runtest/fs b/runtest/fs
index 07d6e2b67964..46318575653e 100644
--- a/runtest/fs
+++ b/runtest/fs
@@ -71,7 +71,7 @@ proc01 proc01 -m 128
 
 read_all_dev read_all -d /dev -p -q -r 10
 read_all_proc read_all -d /proc -q -r 10
-read_all_sys read_all -d /sys -q -r 10 -e /sys/power/wakeup_count
+read_all_sys read_all -d /sys -q -r 10
 
 #Run the File System Race Condition Check tests as well
 fs_racer fs_racer.sh -t 5
diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index 4edccff03a4f..46f88af2270c 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -71,7 +71,6 @@ enum dent_action {
 static char *verbose;
 static char *quiet;
 static char *root_dir;
-static char *exclude;
 static char *str_reads;
 static int reads = 1;
 static char *str_worker_count;
@@ -81,6 +80,11 @@ static long max_workers = 15;
 static struct worker *workers;
 static char *drop_privs;
 
+static char *blacklist[] = {
+	NULL, /* reserved for -e parameter */
+	"/sys/power/wakeup_count",
+};
+
 static struct tst_option options[] = {
 	{"v", &verbose,
 	 "-v       Print information about successful reads."},
@@ -88,7 +92,7 @@ static struct tst_option options[] = {
 	 "-q       Don't print file read or open errors."},
 	{"d:", &root_dir,
 	 "-d path  Path to the directory to read from, defaults to /sys."},
-	{"e:", &exclude,
+	{"e:", &blacklist[0],
 	 "-e pattern Ignore files which match an 'extended' pattern, see fnmatch(3)."},
 	{"r:", &str_reads,
 	 "-r count The number of times to schedule a file for reading."},
@@ -182,17 +186,29 @@ static void sanitize_str(char *buf, ssize_t count)
 		strcpy(buf + MAX_DISPLAY, "...");
 }
 
+static int is_blacklisted(const char *path)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
+		if (blacklist[i] && !fnmatch(blacklist[i], path, FNM_EXTMATCH)) {
+			if (verbose)
+				tst_res(TINFO, "Ignoring %s", path);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 static void read_test(const char *path)
 {
 	char buf[BUFFER_SIZE];
 	int fd;
 	ssize_t count;
 
-	if (exclude && !fnmatch(exclude, path, FNM_EXTMATCH)) {
-		if (verbose)
-			tst_res(TINFO, "Ignoring %s", path);
+	if (is_blacklisted(path))
 		return;
-	}
 
 	if (verbose)
 		tst_res(TINFO, "%s(%s)", __func__, path);
-- 
1.8.3.1

[LTP] [PATCH v2 2/2] read_all_sys: skip debugfs

[Jan Stancek]

debugfs is meant for debugging, it exposes also device registers
and can pretty much do anything:
  https://lore.kernel.org/linux-arm-kernel/1507592549.3785589.1570404050459.JavaMail.zimbra@redhat.com/
  https://lore.kernel.org/stable/2029139028.10333037.1572874551626.JavaMail.zimbra@redhat.com/

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/fs/read_all/read_all.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index 46f88af2270c..68fc7d031f80 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -83,6 +83,7 @@ static char *drop_privs;
 static char *blacklist[] = {
 	NULL, /* reserved for -e parameter */
 	"/sys/power/wakeup_count",
+	"/sys/kernel/debug/*",
 };
 
 static struct tst_option options[] = {
-- 
1.8.3.1

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Cyril Hrubis]

Hi!

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Jan Stancek]

----- Original Message -----
> Hi!
> 
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> 

OK to push both, or is this only for first patch?

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Cyril Hrubis]

Hi!
> OK to push both, or is this only for first patch?

Both of them, sorry for not being clear.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Jan Stancek]

----- Original Message -----
> Hi!
> > OK to push both, or is this only for first patch?
> 
> Both of them, sorry for not being clear.

Thanks, both pushed.

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Richard Palethorpe]

Hello,

Jan Stancek <jstancek@redhat.com> writes:

> library doesn't support multiple parameters for same option.
>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  runtest/fs                              |  2 +-
>  testcases/kernel/fs/read_all/read_all.c | 28 ++++++++++++++++++++++------
>  2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/runtest/fs b/runtest/fs
> index 07d6e2b67964..46318575653e 100644
> --- a/runtest/fs
> +++ b/runtest/fs
> @@ -71,7 +71,7 @@ proc01 proc01 -m 128
>
>  read_all_dev read_all -d /dev -p -q -r 10
>  read_all_proc read_all -d /proc -q -r 10
> -read_all_sys read_all -d /sys -q -r 10 -e /sys/power/wakeup_count
> +read_all_sys read_all -d /sys -q -r 10
>
>  #Run the File System Race Condition Check tests as well
>  fs_racer fs_racer.sh -t 5
> diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
> index 4edccff03a4f..46f88af2270c 100644
> --- a/testcases/kernel/fs/read_all/read_all.c
> +++ b/testcases/kernel/fs/read_all/read_all.c
> @@ -71,7 +71,6 @@ enum dent_action {
>  static char *verbose;
>  static char *quiet;
>  static char *root_dir;
> -static char *exclude;
>  static char *str_reads;
>  static int reads = 1;
>  static char *str_worker_count;
> @@ -81,6 +80,11 @@ static long max_workers = 15;
>  static struct worker *workers;
>  static char *drop_privs;
>
> +static char *blacklist[] = {
> +	NULL, /* reserved for -e parameter */
> +	"/sys/power/wakeup_count",
> +};

The problem with this is that it is only required if we are running as a
privileged user. If -p is specified then it would be a bug if nobody can
read from any of these files.

So I guess we could disable the builtin blacklist if drop_privs (switch
to nobody) is specified and run this test twice on /sys with and without
-p.

> +
>  static struct tst_option options[] = {
>  	{"v", &verbose,
>  	 "-v       Print information about successful reads."},
> @@ -88,7 +92,7 @@ static struct tst_option options[] = {
>  	 "-q       Don't print file read or open errors."},
>  	{"d:", &root_dir,
>  	 "-d path  Path to the directory to read from, defaults to /sys."},
> -	{"e:", &exclude,
> +	{"e:", &blacklist[0],
>  	 "-e pattern Ignore files which match an 'extended' pattern, see fnmatch(3)."},
>  	{"r:", &str_reads,
>  	 "-r count The number of times to schedule a file for reading."},
> @@ -182,17 +186,29 @@ static void sanitize_str(char *buf, ssize_t count)
>  		strcpy(buf + MAX_DISPLAY, "...");
>  }
>
> +static int is_blacklisted(const char *path)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
> +		if (blacklist[i] && !fnmatch(blacklist[i], path, FNM_EXTMATCH)) {
> +			if (verbose)
> +				tst_res(TINFO, "Ignoring %s", path);
> +			return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void read_test(const char *path)
>  {
>  	char buf[BUFFER_SIZE];
>  	int fd;
>  	ssize_t count;
>
> -	if (exclude && !fnmatch(exclude, path, FNM_EXTMATCH)) {
> -		if (verbose)
> -			tst_res(TINFO, "Ignoring %s", path);
> +	if (is_blacklisted(path))
>  		return;
> -	}
>
>  	if (verbose)
>  		tst_res(TINFO, "%s(%s)", __func__, path);
> --
> 1.8.3.1


--
Thank you,
Richard.

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Cyril Hrubis]

Hi!
> > +static char *blacklist[] = {
> > +	NULL, /* reserved for -e parameter */
> > +	"/sys/power/wakeup_count",
> > +};
> 
> The problem with this is that it is only required if we are running as a
> privileged user. If -p is specified then it would be a bug if nobody can
> read from any of these files.

Good point.

> So I guess we could disable the builtin blacklist if drop_privs (switch
> to nobody) is specified and run this test twice on /sys with and without

Sounds reasonable, will you send a patch?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Richard Palethorpe]

hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > +static char *blacklist[] = {
>> > +	NULL, /* reserved for -e parameter */
>> > +	"/sys/power/wakeup_count",
>> > +};
>> 
>> The problem with this is that it is only required if we are running as a
>> privileged user. If -p is specified then it would be a bug if nobody can
>> read from any of these files.
>
> Good point.
>
>> So I guess we could disable the builtin blacklist if drop_privs (switch
>> to nobody) is specified and run this test twice on /sys with and without
>
> Sounds reasonable, will you send a patch?

Yeah, sure.

-- 
Thank you,
Richard.

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Jan Stancek]

----- Original Message -----
> > +static char *blacklist[] = {
> > +	NULL, /* reserved for -e parameter */
> > +	"/sys/power/wakeup_count",
> > +};
> 
> The problem with this is that it is only required if we are running as a
> privileged user. If -p is specified then it would be a bug if nobody can
> read from any of these files.
> 
> So I guess we could disable the builtin blacklist if drop_privs (switch
> to nobody) is specified

Good point. I just saw your reply that you plan to send a patch, thank you.

> and run this test twice on /sys with and without
> -p.
 
greg-kh wasn't very happy to hear about privileged runs in the other thread.
He was suggesting whitelist approach, but I don't know how we would keep it
up to date, deal with different configs, etc.

[LTP] [PATCH v2 1/2] read_all: move blacklist to source

[Richard Palethorpe]

Hello,

Jan Stancek <jstancek@redhat.com> writes:

> ----- Original Message -----
>> > +static char *blacklist[] = {
>> > +	NULL, /* reserved for -e parameter */
>> > +	"/sys/power/wakeup_count",
>> > +};
>>
>> The problem with this is that it is only required if we are running as a
>> privileged user. If -p is specified then it would be a bug if nobody can
>> read from any of these files.
>>
>> So I guess we could disable the builtin blacklist if drop_privs (switch
>> to nobody) is specified
>
> Good point. I just saw your reply that you plan to send a patch, thank
> you.

Thanks!

>
>> and run this test twice on /sys with and without
>> -p.
>
> greg-kh wasn't very happy to hear about privileged runs in the other thread.
> He was suggesting whitelist approach, but I don't know how we would keep it
> up to date, deal with different configs, etc.

--
Thank you,
Richard.