[LTP] [RFC PATCH] tst_taint: TCONF when kernel is alreay tainted

[LTP] [RFC PATCH] tst_taint: TCONF when kernel is alreay tainted

[Li Wang]

As the tst_taint_init comments described, If the tainted-flags are already set
by the kernel, there is no reason to continue and TCONF is generated. But in
the function achieve, it uses TBROK.

  cmdline="cve-2017-17053"
  tst_test.c:1096: INFO: Timeout per run is 0h 10m 00s
  tst_taint.c:88: BROK: Kernel is already tainted: 536871424

Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Chang Yin <cyin@redhat.com>
Cc: Michael Moese <mmoese@suse.de>
---

Notes:
    Hi Cyril & Michael,
    
    I'm not sure if that's a good choice to skip whole test if the kernel already
    tainted, so maybe we could also perform it but not do a strict tainted-flags
    check after testing? The reason I think is it probably have chance to find
    new issue even with a tainted kernel.
    
    Any suggestion?

 lib/tst_taint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/tst_taint.c b/lib/tst_taint.c
index a5dbf77d2..f7cafb96f 100644
--- a/lib/tst_taint.c
+++ b/lib/tst_taint.c
@@ -85,7 +85,7 @@ void tst_taint_init(unsigned int mask)
 
 	taint = tst_taint_read();
 	if ((taint & mask) != 0)
-		tst_brk(TBROK, "Kernel is already tainted: %u", taint);
+		tst_brk(TCONF, "Kernel is already tainted: %u", taint);
 }
 
 
-- 
2.20.1

[LTP] [RFC PATCH] tst_taint: TCONF when kernel is alreay tainted

[Li Wang]

On Wed, Sep 18, 2019 at 1:35 PM Li Wang <liwang@redhat.com> wrote:

> As the tst_taint_init comments described, If the tainted-flags are already
> set
> by the kernel, there is no reason to continue and TCONF is generated. But
> in
> the function achieve, it uses TBROK.
>
>   cmdline="cve-2017-17053"
>   tst_test.c:1096: INFO: Timeout per run is 0h 10m 00s
>   tst_taint.c:88: BROK: Kernel is already tainted: 536871424
>
> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: Chang Yin <cyin@redhat.com>
> Cc: Michael Moese <mmoese@suse.de>
> ---
>
> Notes:
>     Hi Cyril & Michael,
>
>     I'm not sure if that's a good choice to skip whole test if the kernel
> already
>     tainted, so maybe we could also perform it but not do a strict
> tainted-flags
>     check after testing? The reason I think is it probably have chance to
> find
>     new issue even with a tainted kernel.
>

Something maybe change like below, which way is better?

--- a/lib/tst_taint.c
+++ b/lib/tst_taint.c
@@ -7,6 +7,7 @@
 #define TAINT_FILE "/proc/sys/kernel/tainted"

 static unsigned int taint_mask = -1;
+static unsigned int taint_check = 1;

 static unsigned int tst_taint_read(void)
 {
@@ -84,8 +85,10 @@ void tst_taint_init(unsigned int mask)
        taint_mask = mask;

        taint = tst_taint_read();
-       if ((taint & mask) != 0)
-               tst_brk(TCONF, "Kernel is already tainted: %u", taint);
+       if ((taint & mask) != 0) {
+               tst_res(TINFO, "Kernel is already tainted: %u", taint);
+               taint_check = 0;
+       }
 }


@@ -98,5 +101,5 @@ unsigned int tst_taint_check(void)

        taint = tst_taint_read();

-       return (taint & taint_mask);
+       return (taint & taint_mask & taint_check);
 }
-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190918/2a2f7a28/attachment.htm>

[LTP] [RFC PATCH] tst_taint: TCONF when kernel is alreay tainted

[Cyril Hrubis]

Hi!
> As the tst_taint_init comments described, If the tainted-flags are already set
> by the kernel, there is no reason to continue and TCONF is generated. But in
> the function achieve, it uses TBROK.
> 
>   cmdline="cve-2017-17053"
>   tst_test.c:1096: INFO: Timeout per run is 0h 10m 00s
>   tst_taint.c:88: BROK: Kernel is already tainted: 536871424

There is a reason for generating TBROK, we do not want the test to be
skipped silently in this case. If kernel is tainted something went wrong
anyways and we are looking only for a specific flags.

> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: Chang Yin <cyin@redhat.com>
> Cc: Michael Moese <mmoese@suse.de>
> ---
> 
> Notes:
>     Hi Cyril & Michael,
>     
>     I'm not sure if that's a good choice to skip whole test if the kernel already
>     tainted, so maybe we could also perform it but not do a strict tainted-flags
>     check after testing? The reason I think is it probably have chance to find
>     new issue even with a tainted kernel.
>     
>     Any suggestion?
> 
>  lib/tst_taint.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/tst_taint.c b/lib/tst_taint.c
> index a5dbf77d2..f7cafb96f 100644
> --- a/lib/tst_taint.c
> +++ b/lib/tst_taint.c
> @@ -85,7 +85,7 @@ void tst_taint_init(unsigned int mask)
>  
>  	taint = tst_taint_read();
>  	if ((taint & mask) != 0)
> -		tst_brk(TBROK, "Kernel is already tainted: %u", taint);
> +		tst_brk(TCONF, "Kernel is already tainted: %u", taint);
>  }
>  
>  
> -- 
> 2.20.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH] tst_taint: TCONF when kernel is alreay tainted

[Li Wang]

On Thu, Sep 19, 2019 at 6:02 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > As the tst_taint_init comments described, If the tainted-flags are
> already set
> > by the kernel, there is no reason to continue and TCONF is generated.
> But in
> > the function achieve, it uses TBROK.
> >
> >   cmdline="cve-2017-17053"
> >   tst_test.c:1096: INFO: Timeout per run is 0h 10m 00s
> >   tst_taint.c:88: BROK: Kernel is already tainted: 536871424
>
> There is a reason for generating TBROK, we do not want the test to be
> skipped silently in this case. If kernel is tainted something went wrong
> anyways and we are looking only for a specific flags.
>

That's true. The TBROK can highlight the tainted kernel as a good reminder
to the tester.

But in a real testing situation, LTP sometimes not being run in the first,
after we go here cve-2017-17053, the kernel very probably has already been
tainted and reported by other tests. So this new TBROK will drive tester to
double-check if something wrong as new.

Imagine that, if there are many test cases invoke a tainted checking(e.g.
TST_TAINT_W) in LTP, then all of them will be skipped and report TBORK on
such tainted-kernel, that seems not to be friendly to the tester to review
the result.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20190920/7ebf5063/attachment.htm>

[LTP] [RFC PATCH] tst_taint: TCONF when kernel is alreay tainted

[Jan Stancek]

----- Original Message -----
> Hi!
> > As the tst_taint_init comments described, If the tainted-flags are already
> > set
> > by the kernel, there is no reason to continue and TCONF is generated. But
> > in
> > the function achieve, it uses TBROK.
> > 
> >   cmdline="cve-2017-17053"
> >   tst_test.c:1096: INFO: Timeout per run is 0h 10m 00s
> >   tst_taint.c:88: BROK: Kernel is already tainted: 536871424
> 
> There is a reason for generating TBROK, we do not want the test to be
> skipped silently in this case.

It can still run and maybe trigger worse problem. IMO if test wants
to report taint flags it should only report _new_ taint flags.

We could add a dummy test to end of runtest file, which would check
selected taint flags and report WARN/FAIL, so they are guaranteed
to appear on report.

Regards,
Jan

[LTP] [RFC PATCH] tst_taint: TCONF when kernel is alreay tainted

[Cyril Hrubis]

Hi!
> > > As the tst_taint_init comments described, If the tainted-flags are already
> > > set
> > > by the kernel, there is no reason to continue and TCONF is generated. But
> > > in
> > > the function achieve, it uses TBROK.
> > > 
> > >   cmdline="cve-2017-17053"
> > >   tst_test.c:1096: INFO: Timeout per run is 0h 10m 00s
> > >   tst_taint.c:88: BROK: Kernel is already tainted: 536871424
> > 
> > There is a reason for generating TBROK, we do not want the test to be
> > skipped silently in this case.
> 
> It can still run and maybe trigger worse problem. IMO if test wants
> to report taint flags it should only report _new_ taint flags.

IMHO this is more complex than this, we look for a specific subset of
taint flags. One is the warn flag, that is not well defined and may be
harmless and the second is the died flag, which is set on OOPS or BUG. I
I would say that in case of the died flag any subseqent testing is
pointless since the machine is in inconsistent state and has to be
rebooted anyways. Hence we may weaken the check for a warning flag, i.e.
ignore it if it was set at the start of the test, but certainly not for
the died flag, which would solve this problem as well.

> We could add a dummy test to end of runtest file, which would check
> selected taint flags and report WARN/FAIL, so they are guaranteed
> to appear on report.

Actually the new testrunner runltp-ng checks the taint flags after each
test and reboots the machine if needed, which is, as far as I can tell,
the right place for such checks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [RFC PATCH] tst_taint: TCONF when kernel is alreay tainted

[Jan Stancek]

----- Original Message -----
> Hi!
> > > > As the tst_taint_init comments described, If the tainted-flags are
> > > > already
> > > > set
> > > > by the kernel, there is no reason to continue and TCONF is generated.
> > > > But
> > > > in
> > > > the function achieve, it uses TBROK.
> > > > 
> > > >   cmdline="cve-2017-17053"
> > > >   tst_test.c:1096: INFO: Timeout per run is 0h 10m 00s
> > > >   tst_taint.c:88: BROK: Kernel is already tainted: 536871424
> > > 
> > > There is a reason for generating TBROK, we do not want the test to be
> > > skipped silently in this case.
> > 
> > It can still run and maybe trigger worse problem. IMO if test wants
> > to report taint flags it should only report _new_ taint flags.
> 
> IMHO this is more complex than this, we look for a specific subset of
> taint flags. One is the warn flag, that is not well defined and may be
> harmless and the second is the died flag, which is set on OOPS or BUG. I
> I would say that in case of the died flag any subseqent testing is
> pointless since the machine is in inconsistent state and has to be
> rebooted anyways. Hence we may weaken the check for a warning flag, i.e.
> ignore it if it was set at the start of the test, but certainly not for
> the died flag, which would solve this problem as well.

OK, I'd vote for getting rid of warn flag then or ignore it on test start.
Upstream devel trees tend to trigger lot of random warnings during boot,
e.g. a random driver warning from device that isn't even used by LTP.

> 
> > We could add a dummy test to end of runtest file, which would check
> > selected taint flags and report WARN/FAIL, so they are guaranteed
> > to appear on report.
> 
> Actually the new testrunner runltp-ng checks the taint flags after each
> test and reboots the machine if needed, which is, as far as I can tell,
> the right place for such checks.

It is right place, but I think it (reboot) should be configurable behavior.
Some harnesses might want to handle it themselves or just stop testing.
For example on 1TB box that takes 45min to boot, I'd probably turn it off.