linux-kernel - Re: [PATCH v4] resource: add a simple test for walk_iomem_res

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: Wed, 5 Jun 2024 15:52:26 -0700
From: Chia-I Wu <olvaffe@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Dan Williams <dan.j.williams@...el.com>, 
	AKASHI Takahiro <takahiro.akashi@...aro.org>, Dave Jiang <dave.jiang@...el.com>, 
	Alison Schofield <alison.schofield@...el.com>, Baoquan He <bhe@...hat.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] resource: add a simple test for walk_iomem_res_desc()

On Wed, Jun 5, 2024 at 3:10 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Wed, Jun 05, 2024 at 02:28:26PM -0700, Chia-I Wu wrote:
> > This mainly tests that find_next_iomem_res() does not miss resources.
>
> ...
>
> > v2: update subject, use DEFINE_RES_NAMED and hardcoded offsets
> > v3: really hardcode offsets, with 4KB intervals since 0x1000 is easier
> >     to read than 0x400
>
> Right, but...
>
> > v4: use RESOURCE_SIZE_MAX, split allocate_resource and KUNIT_ASSERT_EQ,
> >     and other cosmetic changes
>
> ...
>
> > +     ret = allocate_resource(&iomem_resource, &root, 0x100000,
> > +                     0, RESOURCE_SIZE_MAX, 0x100000, NULL, NULL);
>
> Just double check that limits.h is included.
>
> ...
>
> > +     /* build the resource tree */
> > +     res[0] = DEFINE_RES_NAMED(root.start + 0x0000, 0x1000, "SYSRAM 1",
> > +                     IORESOURCE_SYSTEM_RAM);
> > +     res[1] = DEFINE_RES_NAMED(root.start + 0x1000, 0x1000, "OTHER", 0);
> > +
> > +     res[2] = DEFINE_RES_NAMED(root.start + 0x3000, 0x1000, "NESTED", 0);
> > +     res[3] = DEFINE_RES_NAMED(root.start + 0x3800, 0x0400, "SYSRAM 2",
> > +                     IORESOURCE_SYSTEM_RAM);
>
> ...here is overlap with the previous resource.
>
> And here is the gap to the next one, in case we make that overlapping gone.
>
> > +     res[4] = DEFINE_RES_NAMED(root.start + 0x4000, 0x1000, "SYSRAM 3",
> > +                     IORESOURCE_SYSTEM_RAM);
>
> It wasn't the case in previous data. Please, elaborate what's going on here?
The test data is chosen to be

  first interval: a matching resource (res[0])
  second interval: a non-matching resource (res[1])
  third interval: a hole
  fourth interval: a matching resource (res[3]) nested in a
non-matching resource (res[2])
  fifth interval: a matching resource (res[4])

The idea hasn't changed between revisions.

res[3] went from a half of res[2] to a quarter of res[2] in v4.  I
guess it causes confusion if it is not viewed as a nested resource.

>
> ...
>
> And rather sending one version per 12h, take your time and think more about
> test data. What are we testing? Are the testing data correct? Shouldn't we also
> have negative test cases?
The current choice of test data covers the most common patterns.  Do
you have other patterns you want to cover?  I am new to the resource
code and that's why I am largely reactive to review feedback.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>