runtest/fs: Don't read files in /dev indiscriminately
diff mbox series

Message ID 20180511173156.30197-1-punit.agrawal@arm.com
State Rejected
Delegated to: Cyril Hrubis
Headers show
Series
  • runtest/fs: Don't read files in /dev indiscriminately
Related show

Commit Message

Punit Agrawal May 11, 2018, 5:31 p.m. UTC
read_all_dev attempts to read 1024 bytes from all devices in /dev. As
nodes in /dev represent devices, any access can have side-effects -
sometimes fatally so, e.g., accessing /dev/port on Juno R2 and AMD
Seattle lead to synchronous external abort or SError (system error)
interrupt depending on the access pattern.

There isn't much the kernel can do about the aborts other than panic
the system.

The side-effects problem is also highlighted by the recent exclusion
added for watchdog devices. See commit 4a41aa6b48c134e ("runtest/fs:
filter /dev/watchdog* for read_all_dev by default").

It would be better to replace indiscriminate reading of /dev files
with tests targeting specific files in /dev which have defined known
behaviour, e.g., /dev/null, /dev/urandom, etc.

In the meanwhile, drop the indiscriminate reading of files in /dev.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
Cc: xuyang.jy@cn.fujitsu.com
Cc: naresh.kamboju@linaro.org
Cc: rpalethorpe@suse.com
Cc: chrubis@suse.cz
Cc: james.morse@arm.com
---
Hi,

The test leads to panic during nightly ltp runs on internal
systems. Looking at the crash report from Naresh[0], it looks likely
that he's facing the same problem.

Please consider including in upcoming release.

Thanks,
Punit

[0] http://lists.linux.it/pipermail/ltp/2018-May/007954.html
---
 runtest/fs | 1 -
 1 file changed, 1 deletion(-)

Comments

Richard Palethorpe May 14, 2018, 7:28 a.m. UTC | #1
Hello,

Punit Agrawal writes:

> read_all_dev attempts to read 1024 bytes from all devices in /dev. As
> nodes in /dev represent devices, any access can have side-effects -
> sometimes fatally so, e.g., accessing /dev/port on Juno R2 and AMD
> Seattle lead to synchronous external abort or SError (system error)
> interrupt depending on the access pattern.
>
> There isn't much the kernel can do about the aborts other than panic
> the system.
>
> The side-effects problem is also highlighted by the recent exclusion
> added for watchdog devices. See commit 4a41aa6b48c134e ("runtest/fs:
> filter /dev/watchdog* for read_all_dev by default").
>
> It would be better to replace indiscriminate reading of /dev files
> with tests targeting specific files in /dev which have defined known
> behaviour, e.g., /dev/null, /dev/urandom, etc.
>
> In the meanwhile, drop the indiscriminate reading of files in /dev.

Another solution might be to add an option which drops privileges before
running the test on /dev. An unprivileged user should not have access to files
like /dev/port, but will for /dev/null and /dev/urandom.

>
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> Cc: xuyang.jy@cn.fujitsu.com
> Cc: naresh.kamboju@linaro.org
> Cc: rpalethorpe@suse.com
> Cc: chrubis@suse.cz
> Cc: james.morse@arm.com
> ---
> Hi,
>
> The test leads to panic during nightly ltp runs on internal
> systems. Looking at the crash report from Naresh[0], it looks likely
> that he's facing the same problem.
>
> Please consider including in upcoming release.
>
> Thanks,
> Punit
>
> [0] http://lists.linux.it/pipermail/ltp/2018-May/007954.html
> ---
>  runtest/fs | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/runtest/fs b/runtest/fs
> index 42a9bfcbf..c7ed64fbf 100644
> --- a/runtest/fs
> +++ b/runtest/fs
> @@ -69,7 +69,6 @@ fs_di fs_di -d $TMPDIR
>  # Was not sure why it should reside in runtest/crashme and won´t get tested ever
>  proc01 proc01 -m 128
>
> -read_all_dev read_all -d /dev -e '/dev/watchdog?(0)' -q -r 10
>  read_all_proc read_all -d /proc -q -r 10
>  read_all_sys read_all -d /sys -q -r 10


--
Thank you,
Richard.
Cyril Hrubis May 14, 2018, 8:53 a.m. UTC | #2
Hi!
> > It would be better to replace indiscriminate reading of /dev files
> > with tests targeting specific files in /dev which have defined known
> > behaviour, e.g., /dev/null, /dev/urandom, etc.
> >
> > In the meanwhile, drop the indiscriminate reading of files in /dev.
> 
> Another solution might be to add an option which drops privileges before
> running the test on /dev. An unprivileged user should not have access to files
> like /dev/port, but will for /dev/null and /dev/urandom.

That sounds like a better solution to me, can we get that in before the
release?
Richard Palethorpe May 14, 2018, 9:04 a.m. UTC | #3
Hello,

Cyril Hrubis writes:

> Hi!
>> > It would be better to replace indiscriminate reading of /dev files
>> > with tests targeting specific files in /dev which have defined known
>> > behaviour, e.g., /dev/null, /dev/urandom, etc.
>> >
>> > In the meanwhile, drop the indiscriminate reading of files in /dev.
>> 
>> Another solution might be to add an option which drops privileges before
>> running the test on /dev. An unprivileged user should not have access to files
>> like /dev/port, but will for /dev/null and /dev/urandom.
>
> That sounds like a better solution to me, can we get that in before the
> release?

Yes, I can give this priority.
Punit Agrawal May 14, 2018, 10:07 a.m. UTC | #4
Hi Richard,

Richard Palethorpe <rpalethorpe@suse.de> writes:

> Hello,
>
> Punit Agrawal writes:
>
>> read_all_dev attempts to read 1024 bytes from all devices in /dev. As
>> nodes in /dev represent devices, any access can have side-effects -
>> sometimes fatally so, e.g., accessing /dev/port on Juno R2 and AMD
>> Seattle lead to synchronous external abort or SError (system error)
>> interrupt depending on the access pattern.
>>
>> There isn't much the kernel can do about the aborts other than panic
>> the system.
>>
>> The side-effects problem is also highlighted by the recent exclusion
>> added for watchdog devices. See commit 4a41aa6b48c134e ("runtest/fs:
>> filter /dev/watchdog* for read_all_dev by default").
>>
>> It would be better to replace indiscriminate reading of /dev files
>> with tests targeting specific files in /dev which have defined known
>> behaviour, e.g., /dev/null, /dev/urandom, etc.
>>
>> In the meanwhile, drop the indiscriminate reading of files in /dev.
>
> Another solution might be to add an option which drops privileges before
> running the test on /dev. An unprivileged user should not have access to files
> like /dev/port, but will for /dev/null and /dev/urandom.

The following files have unprivileged read permission in /dev -

vfio/vfio
net/tun
ptmx
tty
urandom
random
full
zero
null

which seems a reasonable set (this'll likely vary by system).

So dropping privileges should keep things ticking.

Thanks,
Punit


>
>>
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>> Cc: xuyang.jy@cn.fujitsu.com
>> Cc: naresh.kamboju@linaro.org
>> Cc: rpalethorpe@suse.com
>> Cc: chrubis@suse.cz
>> Cc: james.morse@arm.com
>> ---
>> Hi,
>>
>> The test leads to panic during nightly ltp runs on internal
>> systems. Looking at the crash report from Naresh[0], it looks likely
>> that he's facing the same problem.
>>
>> Please consider including in upcoming release.
>>
>> Thanks,
>> Punit
>>
>> [0] http://lists.linux.it/pipermail/ltp/2018-May/007954.html
>> ---
>>  runtest/fs | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/runtest/fs b/runtest/fs
>> index 42a9bfcbf..c7ed64fbf 100644
>> --- a/runtest/fs
>> +++ b/runtest/fs
>> @@ -69,7 +69,6 @@ fs_di fs_di -d $TMPDIR
>>  # Was not sure why it should reside in runtest/crashme and won´t get tested ever
>>  proc01 proc01 -m 128
>>
>> -read_all_dev read_all -d /dev -e '/dev/watchdog?(0)' -q -r 10
>>  read_all_proc read_all -d /proc -q -r 10
>>  read_all_sys read_all -d /sys -q -r 10
>
>
> --
> Thank you,
> Richard.

Patch
diff mbox series

diff --git a/runtest/fs b/runtest/fs
index 42a9bfcbf..c7ed64fbf 100644
--- a/runtest/fs
+++ b/runtest/fs
@@ -69,7 +69,6 @@  fs_di fs_di -d $TMPDIR
 # Was not sure why it should reside in runtest/crashme and won´t get tested ever
 proc01 proc01 -m 128
 
-read_all_dev read_all -d /dev -e '/dev/watchdog?(0)' -q -r 10
 read_all_proc read_all -d /proc -q -r 10
 read_all_sys read_all -d /sys -q -r 10