[LTP] [PATCH] read_all_sys: skip debugfs

[LTP] [PATCH] 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>
---
 runtest/fs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/runtest/fs b/runtest/fs
index 07d6e2b67964..fba19cae03e8 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 -e /sys/power/wakeup_count -e '/sys/kernel/debug/*'
 
 #Run the File System Race Condition Check tests as well
 fs_racer fs_racer.sh -t 5
-- 
1.8.3.1

[LTP] [PATCH] read_all_sys: skip debugfs

[Cyril Hrubis]

Hi!
> 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>
> ---
>  runtest/fs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/runtest/fs b/runtest/fs
> index 07d6e2b67964..fba19cae03e8 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 -e /sys/power/wakeup_count -e '/sys/kernel/debug/*'

Do we even support multiple parameters for the same option?

I looked that the library code and I would say that we will replace the
value with each iteration which means that we will effectively use the
last one.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] read_all_sys: skip debugfs

[Jan Stancek]

----- Original Message -----
> Hi!
> > 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>
> > ---
> >  runtest/fs | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/runtest/fs b/runtest/fs
> > index 07d6e2b67964..fba19cae03e8 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 -e /sys/power/wakeup_count -e
> > '/sys/kernel/debug/*'
> 
> Do we even support multiple parameters for the same option?
> 
> I looked that the library code and I would say that we will replace the
> value with each iteration which means that we will effectively use the
> last one.

That is good point. Do we actually need to store blacklist as cmdline parameters?

[LTP] [PATCH] read_all_sys: skip debugfs

[Cyril Hrubis]

Hi!
> >  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 -e /sys/power/wakeup_count -e '/sys/kernel/debug/*'
> 
> Do we even support multiple parameters for the same option?
> 
> I looked that the library code and I would say that we will replace the
> value with each iteration which means that we will effectively use the
> last one.

Maybe we should even catch the case where user passed one option twice with:

diff --git a/lib/tst_test.c b/lib/tst_test.c
index c7b46d0d5..e1c841585 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -468,6 +468,9 @@ static void parse_topt(unsigned int topts_len, int opt, char *optarg)
        if (i >= topts_len)
                tst_brk(TBROK, "Invalid option '%c' (should not happen)", opt);
 
+       if (*(toptions[i].arg))
+               tst_brk(TBROK, "Option already set!");
+
        *(toptions[i].arg) = optarg ? optarg : "";
 }


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] read_all_sys: skip debugfs

[Cyril Hrubis]

Hi!
> > Do we even support multiple parameters for the same option?
> > 
> > I looked that the library code and I would say that we will replace the
> > value with each iteration which means that we will effectively use the
> > last one.
> 
> That is good point. Do we actually need to store blacklist as cmdline parameters?

I guess that we can put the well known ones to the source as well.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] read_all_sys: skip debugfs

[Jan Stancek]

----- Original Message -----
> Hi!
> > >  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 -e /sys/power/wakeup_count -e
> > > '/sys/kernel/debug/*'
> > 
> > Do we even support multiple parameters for the same option?
> > 
> > I looked that the library code and I would say that we will replace the
> > value with each iteration which means that we will effectively use the
> > last one.
> 
> Maybe we should even catch the case where user passed one option twice with:

Or WARN that option might be overwritten. Anyway, it sounds like good idea.

> 
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index c7b46d0d5..e1c841585 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -468,6 +468,9 @@ static void parse_topt(unsigned int topts_len, int opt,
> char *optarg)
>         if (i >= topts_len)
>                 tst_brk(TBROK, "Invalid option '%c' (should not happen)",
>                 opt);
>  
> +       if (*(toptions[i].arg))
> +               tst_brk(TBROK, "Option already set!");
> +
>         *(toptions[i].arg) = optarg ? optarg : "";
>  }
> 
> 
> --
> Cyril Hrubis
> chrubis@suse.cz
>