linux-kernel - Re: [PATCH v2 1/3] resource: Introduce a new helper resource_contains

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: <aGZN6EFMLp-pL6JB@surfacebook.localdomain>
Date: Thu, 3 Jul 2025 12:31:20 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Alison Schofield <alison.schofield@...el.com>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Li Ming <ming.li@...omail.com>, akpm@...ux-foundation.org,
	bhelgaas@...gle.com, ilpo.jarvinen@...ux.intel.com,
	dave@...olabs.net, jonathan.cameron@...wei.com,
	dave.jiang@...el.com, vishal.l.verma@...el.com, ira.weiny@...el.com,
	dan.j.williams@...el.com, shiju.jose@...wei.com,
	linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] resource: Introduce a new helper
 resource_contains_addr()

Wed, Jul 02, 2025 at 12:16:07PM -0700, Alison Schofield kirjoitti:
> On Wed, Jul 02, 2025 at 11:16:36AM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 02, 2025 at 03:20:06PM +0800, Li Ming wrote:
> > > In CXL subsystem, many functions need to check an address availability
> > > by checking if the resource range contains the address. Providing a new
> > > helper function resource_contains_addr() to check if the resource range
> > > contains the input address.
> > 
> > resources are about ranges and not addresses. At bare minimum naming #####
> > here. Also there is no symmetry with the intersection API. But I would argue
> > to use resource_contains() and just provide necessary parameter with both
> > start and end to be set at the same value.
> > 
> > 	struct resource r = DEFINE_RES...(addr);
> > 
> > 	if (resource_contains, res, &r)
> > 		...do stuff...

(1) ^^^

> Thanks for the review. Two alternative approaches were considered:
> 
> 1) A CXL-only helper with direct comparison:
> This duplicates range checking logic that's already common in the
> resource API. We'd end up with CXL specific when the resource API
> already provides the semantic framework for containment checks.

> 2) A CXL helper using temporary resource + resource_contains():
> While this maintains API consistency, it's unnecessarily heavyweight.

How is it? It's mostly matter of stack usage. Do I miss anything?

> Creating a temporary resource struct just to check if a single address
> falls within a range feels like overengineering.
> (This is your suggestion above.)

I don' think so. This won't create anything, it will declare the resource
data on a stack.

...

> This patch proposes, a new helper in ioport.h, provides a clean, reusable
> API that follows the existing resource_contains() pattern while being
> optimized for the single address case. I guess I can agree with the lack
> of symmetry with the existing API, but that's only if one thinks an API
> cannot be extended. The new helper actually complements resource_contains()
> nicely: one checks resource vs resource containment, the other checks
> address vs resource containment.

The point is _there is no address_ in the resources! There is a _range_.

> Revisited the name, I can see how extending an existing name, like 
> resource_contains() is a poor choice because then one might expect the
> pair to be resource_contains_resource() and resource_contains_addr()
> which it is not.
> 
> Do either of these fit better, or something else?
> - resource_includes_addr() 
> - resource_has_addr()

That's why (see above) naming sucks. Doing it local to CXL makes it clearer as
CXL knows the semantic of the range, struct resource doesn't and shouldn't.
(Yeah, I know that we have something there that relies on the range being an
address range, but those APIs provide more and TBH I would rather split them
to a separate header)

> The new helper would immediately be used 7 times in the CXL subsystem.
> What are you thinking the bar is for adding to ioport.h?

It's only a single subsystem, find 3+ more outside and we can reconsider.
Also for the symmetrical API,

-- 
With Best Regards,
Andy Shevchenko