linux-kernel - Re: [PATCH 2/3] PCI: Do not coalesce host bridge resource structs in place

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: <aPZ_4qDfKdX3F_r3@smile.fi.intel.com>
Date: Mon, 20 Oct 2025 21:30:58 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: linux-pci@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Kai-Heng Feng <kaihengf@...dia.com>, Rob Herring <robh@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH 2/3] PCI: Do not coalesce host bridge resource structs in
 place

On Mon, Oct 20, 2025 at 09:15:08PM +0300, Ilpo Järvinen wrote:
> On Mon, 20 Oct 2025, Andy Shevchenko wrote:
> > On Mon, Oct 20, 2025 at 08:21:50PM +0300, Ilpo Järvinen wrote:
> > > On Wed, 15 Oct 2025, Andy Shevchenko wrote:
> > > > On Fri, Oct 10, 2025 at 05:42:30PM +0300, Ilpo Järvinen wrote:

...

> > > > > +/**
> > > > > + * resource_mergeable - Test if resources are contiguous and can be merged
> > > > > + * @r1: first resource
> > > > > + * @r2: second resource
> > > > > + *
> > > > > + * Tests @r1 is followed by @r2 contiguously and share the metadata.
> > > > 
> > > > This needs an additional explanation about name equivalence that's not only by
> > > > pointers, but by a content.
> > > 
> > > Okay. The point was to check names are the same, the pointer check was 
> > > just an optimization as these resources are expected to carry the same 
> > > name even on the pointer level.
> > > 
> > > > > + * Return: %true if resources are mergeable non-destructively.
> > > > > + */
> > > > > +static bool resource_mergeable(struct resource *r1, struct resource *r2)
> > > > > +{
> > > > > +	if ((r1->flags != r2->flags) ||
> > > > > +	    (r1->desc != r2->desc) ||
> > > > > +	    (r1->parent != r2->parent) ||
> > > > > +	    (r1->end + 1 != r2->start))
> > > > > +		return false;
> > > > 
> > > > > +	if (r1->name == r2->name)
> > > > > +		return true;
> > > > > +
> > > > > +	if (r1->name && r2->name && !strcmp(r1->name, r2->name))
> > > > > +		return true;
> > > > > +
> > > > > +	return false;
> > > > 
> > > > Hmm... Can we keep the logic more straight as in returning false cases as soon
> > > > as possible?
> > > > 
> > > > I think of something like this:
> > > > 
> > > > 	if (r1->name && r2->name)
> > > > 		return strcmp(r1->name, r2->name) == 0;
> > > > 
> > > > 	return r1->name == r2->name;
> > > 
> > > But the point the order above was to avoid strcmp() when the pointer 
> > > itself is same which I think is quite common case. I don't think strcmp() 
> > > itself checks whether the pointer is the same.
> > 
> > On the second thought I think comparing by the content is quite a behavioural
> > change here.
> 
> Compared to what?
> 
> This code was previously only used for merging contiguous "System RAM" 
> resources (AFAICT, I don't have way to check what the names in all those
> resources truly were but in any case, the check was even stricter earlier, 
> comparing pointer equality only so definitely the names were not different 
> before this).
> 
> > Perhaps we may start without doing that first? Theoretically it
> > might be the case when the content of names is different, but resources are
> > the same.
> 
> Resources are NOT same, they're two contiguous memory regions and may 
> originate from different source, and thus have different names.
> 
> Not caring about the names will lose one of them from /proc/iomem.
> 
> > The case when name is the same (by content, but pointers) with the
> > idea of having different resources sounds to me quite an awkward case. TL;
> > DR: What are the cases that we have in practice now?
> 
> In the original thread [1], PCI side resource coalescing did break the 
> resources by merging without caring what the resource internals were. That 
> problem was found after trying to fix another problem, thus it might not 
> happen in practice except after fixing the other problem with root bus 
> resources.
> 
> In the common case when merging PCI root bus resources, the resources 
> typically have the same name - this happens all the time (e.g. io port 
> ranges are split to many small ranges which form a contiguous region 
> when coalesced). But that's not always the case, why do you think these 
> two names should be merged losing some information:
> 
>      ee080000-ee08ffff : pci@...90000
>        ...
>      ee090000-ee090bff : ee090000.pci pci@...90000
> 
> ?

I don't think it's a good idea (after reading the nice elaboration from you).
It seems I misunderstood the use case(s). That's why I asked for some elaboration
about the (new?) requirement to test the content of the names and not only pointer
equivalency.

> (Also, the careless change in the underlying resource by the code this 
> series tries to fix would have likely broken also devres release of the 
> mangled resource, which admittedly, is not related to name at all).
> 
> [1] https://lore.kernel.org/linux-pci/CAMuHMdVgCHU80mRm1Vwo6GFgNAtQcf50yHBz_oAk4TrtjcMpYg@mail.gmail.com/


-- 
With Best Regards,
Andy Shevchenko