When using relaxed tail alignment for the bridge window,
pbus_size_mem() also tries to minimize min_align, which can under
certain scenarios end up increasing min_align from that found by
calculate_mem_align().
Ensure min_align is not increased by the relaxed tail alignment.
Eventually, it would be better to add calculate_relaxed_head_align()
similar to calculate_mem_align() which finds out what alignment can be
used for the head without introducing any gaps into the bridge window
to give flexibility on head address too. But that looks relatively
complex algorithm so it requires much more testing than fixing the
immediate problem causing a regression.
Fixes: 67f9085596ee ("PCI: Allow relaxed bridge window tail sizing for optional resources")
Reported-by: Rio <rio@r26.me>
Tested-by: Rio <rio@r26.me>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: <stable@vger.kernel.org>
---
drivers/pci/setup-bus.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 07c3d021a47e..f90d49cd07da 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1169,6 +1169,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
resource_size_t children_add_size = 0;
resource_size_t children_add_align = 0;
resource_size_t add_align = 0;
+ resource_size_t relaxed_align;
if (!b_res)
return -ENOSPC;
@@ -1246,8 +1247,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
if (bus->self && size0 &&
!pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
size0, min_align)) {
- min_align = 1ULL << (max_order + __ffs(SZ_1M));
- min_align = max(min_align, win_align);
+ relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
+ relaxed_align = max(relaxed_align, win_align);
+ min_align = min(min_align, relaxed_align);
size0 = calculate_memsize(size, min_size, 0, 0, resource_size(b_res), win_align);
pci_info(bus->self, "bridge window %pR to %pR requires relaxed alignment rules\n",
b_res, &bus->busn_res);
@@ -1261,8 +1263,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
if (bus->self && size1 &&
!pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
size1, add_align)) {
- min_align = 1ULL << (max_order + __ffs(SZ_1M));
- min_align = max(min_align, win_align);
+ relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
+ relaxed_align = max(min_align, win_align);
+ min_align = min(min_align, relaxed_align);
size1 = calculate_memsize(size, min_size, add_size, children_add_size,
resource_size(b_res), win_align);
pci_info(bus->self,
--
2.39.5
…
> Ensure min_align is not increased by the relaxed tail alignment.
…
…
+++ b/drivers/pci/setup-bus.c
…
@@ -1261,8 +1263,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
if (bus->self && size1 &&
!pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
size1, add_align)) {
- min_align = 1ULL << (max_order + __ffs(SZ_1M));
- min_align = max(min_align, win_align);
+ relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
+ relaxed_align = max(min_align, win_align);
…
I wonder why a variable content would be overwritten here
without using the previous value.
https://cwe.mitre.org/data/definitions/563.html
Regards,
Markus
On Thu, 21 Aug 2025, Markus Elfring wrote:
> …
> > Ensure min_align is not increased by the relaxed tail alignment.
> …
>
>
> …
> +++ b/drivers/pci/setup-bus.c
> …
> @@ -1261,8 +1263,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> if (bus->self && size1 &&
> !pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
> size1, add_align)) {
> - min_align = 1ULL << (max_order + __ffs(SZ_1M));
> - min_align = max(min_align, win_align);
> + relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
> + relaxed_align = max(min_align, win_align);
> …
>
> I wonder why a variable content would be overwritten here
> without using the previous value.
> https://cwe.mitre.org/data/definitions/563.html
Hi Markus,
This looks a very good catch. I think it too should have been:
relaxed_align = max(relaxed_align, win_align);
...like in the other case.
--
i.
>> …
>> +++ b/drivers/pci/setup-bus.c
>> …
>> @@ -1261,8 +1263,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>> if (bus->self && size1 &&
>> !pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
>> size1, add_align)) {
>> - min_align = 1ULL << (max_order + __ffs(SZ_1M));
>> - min_align = max(min_align, win_align);
>> + relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
>> + relaxed_align = max(min_align, win_align);
>> …
>>
>> I wonder why a variable content would be overwritten here
>> without using the previous value.
>> https://cwe.mitre.org/data/definitions/563.html
…> This looks a very good catch. I think it too should have been:
>
> relaxed_align = max(relaxed_align, win_align);
>
> ...like in the other case.
Did any known source code analysis tools point such a questionable implementation detail out
for further development considerations?
Regards,
Markus
On Mon, Jun 30, 2025 at 05:26:39PM +0300, Ilpo Järvinen wrote:
> When using relaxed tail alignment for the bridge window,
> pbus_size_mem() also tries to minimize min_align, which can under
> certain scenarios end up increasing min_align from that found by
> calculate_mem_align().
>
> Ensure min_align is not increased by the relaxed tail alignment.
>
> Eventually, it would be better to add calculate_relaxed_head_align()
> similar to calculate_mem_align() which finds out what alignment can be
> used for the head without introducing any gaps into the bridge window
> to give flexibility on head address too. But that looks relatively
> complex algorithm so it requires much more testing than fixing the
> immediate problem causing a regression.
>
> Fixes: 67f9085596ee ("PCI: Allow relaxed bridge window tail sizing for optional resources")
> Reported-by: Rio <rio@r26.me>
Was there a regression report URL we could include here?
> Tested-by: Rio <rio@r26.me>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: <stable@vger.kernel.org>
> ---
> drivers/pci/setup-bus.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 07c3d021a47e..f90d49cd07da 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1169,6 +1169,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> resource_size_t children_add_size = 0;
> resource_size_t children_add_align = 0;
> resource_size_t add_align = 0;
> + resource_size_t relaxed_align;
>
> if (!b_res)
> return -ENOSPC;
> @@ -1246,8 +1247,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> if (bus->self && size0 &&
> !pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
> size0, min_align)) {
> - min_align = 1ULL << (max_order + __ffs(SZ_1M));
> - min_align = max(min_align, win_align);
> + relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
> + relaxed_align = max(relaxed_align, win_align);
> + min_align = min(min_align, relaxed_align);
> size0 = calculate_memsize(size, min_size, 0, 0, resource_size(b_res), win_align);
> pci_info(bus->self, "bridge window %pR to %pR requires relaxed alignment rules\n",
> b_res, &bus->busn_res);
> @@ -1261,8 +1263,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> if (bus->self && size1 &&
> !pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
> size1, add_align)) {
> - min_align = 1ULL << (max_order + __ffs(SZ_1M));
> - min_align = max(min_align, win_align);
> + relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
> + relaxed_align = max(min_align, win_align);
> + min_align = min(min_align, relaxed_align);
> size1 = calculate_memsize(size, min_size, add_size, children_add_size,
> resource_size(b_res), win_align);
> pci_info(bus->self,
> --
> 2.39.5
>
On Thu, 21 Aug 2025, Bjorn Helgaas wrote:
> On Mon, Jun 30, 2025 at 05:26:39PM +0300, Ilpo Järvinen wrote:
> > When using relaxed tail alignment for the bridge window,
> > pbus_size_mem() also tries to minimize min_align, which can under
> > certain scenarios end up increasing min_align from that found by
> > calculate_mem_align().
> >
> > Ensure min_align is not increased by the relaxed tail alignment.
> >
> > Eventually, it would be better to add calculate_relaxed_head_align()
> > similar to calculate_mem_align() which finds out what alignment can be
> > used for the head without introducing any gaps into the bridge window
> > to give flexibility on head address too. But that looks relatively
> > complex algorithm so it requires much more testing than fixing the
> > immediate problem causing a regression.
> >
> > Fixes: 67f9085596ee ("PCI: Allow relaxed bridge window tail sizing for optional resources")
> > Reported-by: Rio <rio@r26.me>
>
> Was there a regression report URL we could include here?
There's the Lore thread only:
https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/
(It's so far back that if there was something else, I've forgotten them
by now but looking at the exchanges in the thread, it doesn't look like
bugzilla entry or so made out of it.)
--
i.
> > Tested-by: Rio <rio@r26.me>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> > drivers/pci/setup-bus.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 07c3d021a47e..f90d49cd07da 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1169,6 +1169,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> > resource_size_t children_add_size = 0;
> > resource_size_t children_add_align = 0;
> > resource_size_t add_align = 0;
> > + resource_size_t relaxed_align;
> >
> > if (!b_res)
> > return -ENOSPC;
> > @@ -1246,8 +1247,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> > if (bus->self && size0 &&
> > !pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
> > size0, min_align)) {
> > - min_align = 1ULL << (max_order + __ffs(SZ_1M));
> > - min_align = max(min_align, win_align);
> > + relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
> > + relaxed_align = max(relaxed_align, win_align);
> > + min_align = min(min_align, relaxed_align);
> > size0 = calculate_memsize(size, min_size, 0, 0, resource_size(b_res), win_align);
> > pci_info(bus->self, "bridge window %pR to %pR requires relaxed alignment rules\n",
> > b_res, &bus->busn_res);
> > @@ -1261,8 +1263,9 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> > if (bus->self && size1 &&
> > !pbus_upstream_space_available(bus, mask | IORESOURCE_PREFETCH, type,
> > size1, add_align)) {
> > - min_align = 1ULL << (max_order + __ffs(SZ_1M));
> > - min_align = max(min_align, win_align);
> > + relaxed_align = 1ULL << (max_order + __ffs(SZ_1M));
> > + relaxed_align = max(min_align, win_align);
> > + min_align = min(min_align, relaxed_align);
> > size1 = calculate_memsize(size, min_size, add_size, children_add_size,
> > resource_size(b_res), win_align);
> > pci_info(bus->self,
> > --
> > 2.39.5
> >
>
On Thu, 21 Aug 2025, Ilpo Järvinen wrote:
> On Thu, 21 Aug 2025, Bjorn Helgaas wrote:
>
> > On Mon, Jun 30, 2025 at 05:26:39PM +0300, Ilpo Järvinen wrote:
> > > When using relaxed tail alignment for the bridge window,
> > > pbus_size_mem() also tries to minimize min_align, which can under
> > > certain scenarios end up increasing min_align from that found by
> > > calculate_mem_align().
> > >
> > > Ensure min_align is not increased by the relaxed tail alignment.
> > >
> > > Eventually, it would be better to add calculate_relaxed_head_align()
> > > similar to calculate_mem_align() which finds out what alignment can be
> > > used for the head without introducing any gaps into the bridge window
> > > to give flexibility on head address too. But that looks relatively
> > > complex algorithm so it requires much more testing than fixing the
> > > immediate problem causing a regression.
> > >
> > > Fixes: 67f9085596ee ("PCI: Allow relaxed bridge window tail sizing for optional resources")
> > > Reported-by: Rio <rio@r26.me>
> >
> > Was there a regression report URL we could include here?
>
> There's the Lore thread only:
>
> https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/
>
> (It's so far back that if there was something else, I've forgotten them
> by now but looking at the exchanges in the thread, it doesn't look like
> bugzilla entry or so made out of it.)
Making it "official" tag in case that's easier for you to handle
automatically...
Link: https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/
--
i.
On Thu, Aug 21, 2025 at 06:24:12PM +0300, Ilpo Järvinen wrote:
> On Thu, 21 Aug 2025, Ilpo Järvinen wrote:
> > On Thu, 21 Aug 2025, Bjorn Helgaas wrote:
> > > On Mon, Jun 30, 2025 at 05:26:39PM +0300, Ilpo Järvinen wrote:
> > > > When using relaxed tail alignment for the bridge window,
> > > > pbus_size_mem() also tries to minimize min_align, which can under
> > > > certain scenarios end up increasing min_align from that found by
> > > > calculate_mem_align().
> > > >
> > > > Ensure min_align is not increased by the relaxed tail alignment.
> > > >
> > > > Eventually, it would be better to add calculate_relaxed_head_align()
> > > > similar to calculate_mem_align() which finds out what alignment can be
> > > > used for the head without introducing any gaps into the bridge window
> > > > to give flexibility on head address too. But that looks relatively
> > > > complex algorithm so it requires much more testing than fixing the
> > > > immediate problem causing a regression.
> > > >
> > > > Fixes: 67f9085596ee ("PCI: Allow relaxed bridge window tail sizing for optional resources")
> > > > Reported-by: Rio <rio@r26.me>
> > >
> > > Was there a regression report URL we could include here?
> >
> > There's the Lore thread only:
> >
> > https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/
The email thread is fine and contains good information about how the
reporter tripped over it.
> > (It's so far back that if there was something else, I've forgotten them
> > by now but looking at the exchanges in the thread, it doesn't look like
> > bugzilla entry or so made out of it.)
>
> Making it "official" tag in case that's easier for you to handle
> automatically...
>
> Link: https://lore.kernel.org/all/o2bL8MtD_40-lf8GlslTw-AZpUPzm8nmfCnJKvS8RQ3NOzOW1uq1dVCEfRpUjJ2i7G2WjfQhk2IWZ7oGp-7G-jXN4qOdtnyOcjRR0PZWK5I=@r26.me/
Thanks for all of these, I added them to the commit logs.
Bjorn
© 2016 - 2025 Red Hat, Inc.