linux-ext4 - Re: [PATCH linux-kselftest/test v2] ext4: add kunit test for decoding extended timestamps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Oct 2019 09:19:02 -0400
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     Brendan Higgins <brendanhiggins@...gle.com>
Cc:     Iurii Zaikin <yzaikin@...gle.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>, linux-ext4@...r.kernel.org,
        adilger.kernel@...ger.ca, kunit-dev@...glegroups.com
Subject: Re: [PATCH linux-kselftest/test v2] ext4: add kunit test for
 decoding extended timestamps

On Fri, Oct 11, 2019 at 03:05:43AM -0700, Brendan Higgins wrote:
> That's an interesting point. Should we try to establish a pattern for
> how tests should be configured? My *very long term* goal is to
> eventually have tests able to be built and run without any kind of
> kernel of any kind, but I don't think that having a single config for
> all tests in a subsystem gets in the way of that, so I don't think I
> have a strong preference in terms of what I want to do.
> 
> Nevertheless, I think establishing patterns is good. Do we want to try
> to follow Ted's preference as a general rule from now on?

As I suggested on another thread (started on kunit-dev, but Brendan
has cc'ed in linux-kselftest), I think it might really work well if
"make kunit" runs all of the kunit tests automatically.  As we add
more kunit tests, finding all of the CONFIG options so they can be
added to the kunitconfig file is going to be hard, so kunit.py really
needs an --allconfig which does this automatically.

Along these lines, perhaps we should state that as a general rule the
CONFIG option for Kunit tests should only depend on KUINIT, and use
select to enable other dependencies.  i.e., for the ext4 kunit tests,
it should look like this:

config EXT4_KUNIT_TESTS
	bool "KUnit test for ext4 inode"
	select EXT4_FS
	depends on KUNIT
...

In the current patch, we use "depends on EXT4_FS", which meant that
when I first added "CONFIG_EXT4_KUNIT_TESTS=y" to the kunitconfig
file, I got the following confusing error message:

% ./tools/testing/kunit/kunit.py  run
Regenerating .config ...
ERROR:root:Provided Kconfig is not contained in validated .config!

Using "select EXT4_FS" makes it much easier to enable the ext4 kunit
tests in kunitconfig.  At the moment requiring that we two lines to
kunitconfig to enable ext4 isn't _that_ bad:

CONFIG_EXT4_FS=y
CONFIG_EXT4_KUNIT_TESTS=y

but over time, if many subsystems start adding unit tests, the
overhead of managing the kunitconfig file is going to get unwieldy.
Hence my suggestion that we just make all Kunit CONFIG options depend
only on CONFIG_KUNIT.

> I agree with Iurii. I don't think that this example alone warrants
> adding support for being able to read test data in from a separate
> file (I would also like some clarification here on what is meant by
> reading in from a separate file). I can imagine some scenarios where
> that might make sense, but I think it would be better to get more
> examples before trying to support that use case.

So what I was thinking might happen is that for some of the largest
unit tests before I would transition to deciding that xfstests was the
better way to go, I *might* have a small, 100k ext4 file system which
would checked into the kernel sources as fs/ext4/kunit_test.img, and
there would be a makefile rule that would turn that into
fs/ext4/kunit_test_img.c file that might look something like:

const ext4_kunit_test_img[] = {
      0xde, ...

But I'm not sure I actually want to go down that path.  It would
certainly better from a test design perspective to create test mocks
at a higher layer, such as ext4_iget() and ext4_read_block_bitmap().

The problem is that quite a bit of code in ext4 would have to be
*extensively* refactored in order to allow for easy test mocking,
since we have calls to sb_bread, ext4_bread(), submit_bh(), etc.,
sprinkled alongside the code logic that we would want to test.

So using a small test image and making the cut line be at the buffer
cache layer is going to be much, *much* simpler at least in the short
term.  So the big question is how much of an investment (or technical
debt paydown) do I want to do right away, versus taking a shortcut to
get better unit test coverage more quickly, and then do further tech
debt reduction later?

   	       	     	    	       - Ted