Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 19 Sep 2019 14:17:11 -0700
From:   Kees Cook <keescook@chromium.org>
To:     shuah <shuah@kernel.org>
Cc:     Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Anders Roxell <anders.roxell@linaro.org>,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH] selftests/kselftest/runner.sh: Add 45 second timeout per
 test
Message-ID: <201909191411.3FA57CBCF3@keescook>
References: <201909191102.97FA56072@keescook>
 <20190919185525.GD21254@piout.net>
 <4844c68f-603d-14f2-f976-5bd255268c0d@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4844c68f-603d-14f2-f976-5bd255268c0d@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Sep 19, 2019 at 02:09:37PM -0600, shuah wrote:
> On 9/19/19 12:55 PM, Alexandre Belloni wrote:
> > On 19/09/2019 11:06:44-0700, Kees Cook wrote:
> > > Commit a745f7af3cbd ("selftests/harness: Add 30 second timeout per
> > > test") solves the problem of kselftest_harness.h-using binary tests
> > > possibly hanging forever. However, scripts and other binaries can still
> > > hang forever. This adds a global timeout to each test script run.
> > > 
> 
> Timeout is good, but really tests should not hang. So we have to somehow
> indicate that the test needs to be fixed.

Totally agreed, which is why I changed the reporting to call out a
"TIMEOUT" instead of just having it enter the general failure noise.

> This timeout is a band-aid and not real solution for the problem. This
> arbitrary value doesn't take into account that the test(s) in that
> particular directory (TARGET) could be running normally and working
> through all the tests.

Even something that looks like it's making progress may still be hung or
won't finish in a reasonable amount of time.

> We need some way to differentiate the two cases.

I don't think it's unreasonable to declare that no test should take
longer than some default amount of time that can be tweaked via a
"settings" file. It gives the framework the option of easily removing
tests that take "too long", etc. If the "timeout=..." value was made
mandatory for each test directory, then the framework could actually
filter based on expected worst-case run time.

> > > To make this configurable (e.g. as needed in the "rtc" test case),
> > > include a new per-test-directory "settings" file (similar to "config")
> > > that can contain kselftest-specific settings. The first recognized field
> > > is "timeout".
> > > 
> > 
> > Seems good to me. I was also wondering whether this is actually
> > reasonable to have tests running for so long. I wanted to discuss that
> > at LPC but I missed the session.
> > 
> 
> There is the individual test times and overall kselftest run time. We
> have lots of tests now and it does take long.

This patch seeks to implement a "timeout for a single test from
kselftest's perspective". Some "individual" tests have many subtests
(e.g. anything built with kselftest_harness.h) giving us the whole
subtest issue. I think my solution here is a good middle ground: we
specify the max run time for each executed test binary/script.

It's not clear to me if a v2 is needed? Is this patch fine as-is?

Thanks!

-- 
Kees Cook