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: <9d56e776-731a-7e25-60f0-44485cfbf12c@linux.intel.com>
Date: Mon, 20 Oct 2025 21:15:08 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Andy Shevchenko <andriy.shevchenko@...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, 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

?

(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/

-- 
 i.