Re: [PATCH 3/5] PCI: Place resources to either edge of the window

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4813 bytes --]

On Wed, 23 Sep 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] pci: undefined behavior from 64-bit resource sizes on 32-bit systems
> - [Medium] pci: res->end + 1 overflow breaks natural alignment logic
> --
> 
> commit 65c3dea851ed27dd4bf3576e0a9387eaa7359798
> Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Subject: PCI: Place resources to either edge of the window
> 
> This commit updates the PCI resource assignment phase to use a window edge
> aware resource assignment algorithm. It alters the greedy assignment logic
> to place resources either to the left or right edge of the window to avoid
> fragmenting contiguous free space.
> 
> > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> > index eacce9e2486bf..1ab5d167ab5c6 100644
> > --- a/drivers/pci/setup-res.c
> > +++ b/drivers/pci/setup-res.c
> [ ... ]
> > @@ -262,16 +265,80 @@ resource_size_t pci_resource_alignment(const struct pci_dev *dev,
> >  	return resource_alignment(res);
> >  }
> >  
> > +static resource_size_t pci_max_natural_size(const struct resource *res,
> > +					    resource_size_t *max_align)
> > +{
> > +	resource_size_t size = resource_size(res);
> > +	resource_size_t powof2, natural_start;
> > +
> > +	*max_align = 1;
> > +	if (!size)
> > +		return 0;
> > +
> > +	powof2 = rounddown_pow_of_two(size);
> 
> [Severity: High]
> Does this code inadvertently truncate 64-bit resource sizes on 32-bit systems?
> 
> When a 32-bit kernel is built with a 64-bit phys_addr_t (like x86 PAE or
> ARM LPAE), passing a 64-bit size into rounddown_pow_of_two() casts it to an
> unsigned long. If the size is a multiple of 4GB (which is common for large
> GPU BARs), the lower 32 bits are silently truncated to 0, causing powof2 to
> be evaluated as 0.

This is a pre-existing issue. PCI core already uses __ffs() which is based 
on unsigned long (at least on x86).

There's also a serious discussion in x86 space about future of 32-bit 
support where keeping it wastes development effort:

https://lore.kernel.org/all/bd252483-fb0a-4818-bfaf-b9b6ebd187fe@intel.com/

The non-trivial cost here is avoiding the problematic generic helpers 
(custom coding them) or fixing ffs() and round*_pow_of_two() when input 
size > sizeof(unsigned long), for ability to run >=4GB BARs on a 32-bit 
platform. Yeah, modern GPUs have big BARs but running them on a 32-bit 
platform doesn't sound that great combination.


That being said, I think round*_pow_of_two() interface should be made
to return same type as the input because it simply doesn't make sense to 
convert the value to some other type. I briefly looked into that earlier 
but it requires auditting that all the callers can handle that (IIRC, 
there were even some build failures from making just the type change and
there could be logic flaws as well if something relies on type expansion
to unsigned long).

> > +	natural_start = ALIGN(res->start, powof2);
> > +	if (natural_start >= ALIGN_DOWN(res->end + 1, powof2)) {
> 
> [Severity: Medium]
> Could this boundary check overflow if the window ends exactly at ~0U?
> 
> If a 32-bit system maps PCI space just below the 4GB boundary, res->end
> could be 0xFFFFFFFF. In that scenario, res->end + 1 wraps to 0, which
> causes ALIGN_DOWN(0, powof2) to return 0. The check natural_start >= 0 then
> unconditionally evaluates to true for unsigned types, incorrectly forcing
> the algorithm to halve powof2 and potentially failing to pack memory windows
> tightly.

This will be fixed by subtracting 1 from the aligned result to underflow 
it back from 0 to ~0.

> > +		powof2 = max(powof2 / 2, 1U);
> > +		natural_start = ALIGN(res->start, powof2);
> > +	}
> > +
> > +	if (natural_start) {
> > +		*max_align <<= __ffs(natural_start);
> 
> [Severity: High]
> Does this trigger undefined behavior when passing a truncated 64-bit value?
> 
> Similar to rounddown_pow_of_two(), __ffs() takes an unsigned long. If a
> 64-bit natural_start is a multiple of 4GB, the cast truncates it to 0.
> Passing 0 to __ffs() invokes undefined behavior, which can lead to
> unpredictable results or kernel panics during boot (such as garbage shift
> amounts or arch-specific trap instructions on zero inputs).

Sashiko knows no shame with rather bold claims. I'm pretty sure the answer 
should have fit into its context window, no matter how small that was. ;-)

> > +	} else {
> > +		/*
> > +		 * Zero address has infinite alignment, return the largest
> > +		 * representable number even if it's not a power of two.
> > +		 */
> > +		*max_align = RESOURCE_SIZE_MAX;
> > +	}
> > +
> > +	return powof2;
> > +}
> 
> 

-- 
 i.