linux-kernel - Re: [PATCH] Resource: fix region

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]
Message-ID: <66c69470ae357_7605294ab@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date: Wed, 21 Aug 2024 18:29:20 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: "Huang, Ying" <ying.huang@...el.com>, Dan Williams
	<dan.j.williams@...el.com>
CC: Andrew Morton <akpm@...ux-foundation.org>, <linux-mm@...ck.org>,
	<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>, Davidlohr Bueso
	<dave@...olabs.net>, Jonathan Cameron <jonathan.cameron@...wei.com>, "Dave
 Jiang" <dave.jiang@...el.com>, Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
	Alistair Popple <apopple@...dia.com>, Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Baoquan He <bhe@...hat.com>
Subject: Re: [PATCH] Resource: fix region_intersects() for CXL memory

Huang, Ying wrote:
> Dan Williams <dan.j.williams@...el.com> writes:
> > Huang Ying wrote:
[..]
> >>  kernel/resource.c | 44 +++++++++++++++++++++++++++++++++++++-------
> >>  1 file changed, 37 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/kernel/resource.c b/kernel/resource.c
> >> index 14777afb0a99..c97a5add9394 100644
> >> --- a/kernel/resource.c
> >> +++ b/kernel/resource.c
[..]
> >> +		/*
> >> +		 * Continue to search in descendant resources.  Unless
> >> +		 * the matched descendant resources cover the whole
> >> +		 * overlapped range, increase 'other', because it
> >> +		 * overlaps with 'p' at least.
> >> +		 */
> >> +		other++;
> >
> > This results in REGION_MIXED whenever the target of the search is found
> > as a descendant of @parent which I believe is unwanted.
> 
> This is not the behavior of this patch.  There's a "other--" later in
> this patch.
> 
> +		ostart = max(res.start, p->start);
> +		oend = min(res.end, p->end);
> +		for_each_resource(p, dp, false) {
> +			if (!resource_overlaps(dp, &res))
> +				continue;
> +			is_type = (((dp->flags & flags) == flags) &&
> +				   ((desc == IORES_DESC_NONE) ||
> +				    (desc == dp->desc)));
> +			if (is_type) {
> +				type++;
> +				if (dp->start > ostart)
> +					break;
> +				if (dp->end >= oend) {
> +					other--;     <====================== HERE!

Yes, I missed that.

> +					break;
> +				}
> +				ostart = dp->end + 1;
> +			}
> +		}
>  	}
>  
>  	if (type == 0)
> 
> 
> That is, if the overlapped range is covered by matched (is_type == true)
> descendant resources completely, other will not increase.
> 
> So, for resource tree as follows
> 
> 490000000-52fffffff : CXL Window 0
>   490000000-50fffffff : region0
>     490000000-50fffffff : dax0.0
>       490000000-50fffffff : System RAM (kmem)
>   510000000-52fffffff
>     510000000-52fffffff : dax0.1
> 
> region_intersects(, 0x490000000, PAGE_SIZE, IORESOURCE_SYSTEM_RAM,
> IORES_DESC_NONE) => REGION_INTERSECTS
> region_intersects(, 0x50f000000, 0x2000000, IORESOURCE_SYSTEM_RAM,
> IORES_DESC_NONE) => REGION_MIXED
> 
> Even for
> 
> 490000000-52fffffff : CXL Window 0
>   490000000-50fffffff : region0
>     490000000-50fffffff : dax0.0
>       490000000-50fffffff : System RAM (kmem)
> 
> region_intersects(, 0x50f000000, 0x2000000, IORESOURCE_SYSTEM_RAM,
> IORES_DESC_NONE) => REGION_MIXED
> 
> This isn't perfect, but it looks OK for me.  Because for
> 
> 490000000-50fffffff : System RAM
> 510000000-52fffffff : CXL Window 0
> 
> region_intersects(, 0x50f000000, 0x2000000, IORESOURCE_SYSTEM_RAM,
> IORES_DESC_NONE) => REGION_MIXED

That explanation makes sense and matches my expectation.

> However, I admit that the original code is hard to be understood,
> whether is something like below better?

I like that this proposal defers incrementing @other rather than
decrement after the fact.

> 
> 	for (p = parent->child; p ; p = p->sibling) {
> 		if (!resource_overlaps(p, &res))
> 			continue;
> 		is_type = (((p->flags & flags) == flags) &&
> 			   ((desc == IORES_DESC_NONE) || (desc == p->desc)));
> 		if (is_type) {
> 			type++;
> 			continue;
> 		}
> 		/*
> 		 * Continue to search in descendant resources.  Unless
> 		 * the matched descendant resources cover the whole
> 		 * overlapped range, increase 'other', because it
> 		 * overlaps with 'p' at least.
> 		 */
> 		covered = false;

I would call @covered, @single_descendant. Since @covered is ambiguous.

> 		ostart = max(res.start, p->start);
> 		oend = min(res.end, p->end);
> 		for_each_resource(p, dp, false) {
> 			if (!resource_overlaps(dp, &res))
> 				continue;
> 			is_type = (((dp->flags & flags) == flags) &&
> 				   ((desc == IORES_DESC_NONE) ||
> 				    (desc == dp->desc)));
> 			if (is_type) {
> 				type++;
> 				if (dp->start > ostart)

...this should have a comment:

/* partial descendant overlap indicates overlap with a descendant hole */

> 					break;
> 				if (dp->end >= oend) {
> 					covered = true;
> 					break;

...then per above this because easier to read as:

	single_descendant = true;

> 				}
> 				ostart = dp->end + 1;
> 			}
> 		}
> 		if (!covered)
> 			other++;
> 	}
> 
> > The semantics of region_intersects() has always been within a single
> > sibling level to date. So, I don't think @other should be incremented
> > until @is_type is non-zero. It follows that if @is_type is set and
> > !resource_contains(p, &res) then there is no point in descending because
> > it is known at that there are no descendants to worry about.
> 
> Sorry, I don't understand your words here.  Can you show your idea with
> some examples or pseudo code?

I think your proposed updates address my concern.