From: Kees Cook kees@kernel.org
[ Upstream commit 00e58ff924b3a684b076f9512fe2753be87b50e1 ]
In preparation for the future commit ("bitops: Add __attribute_const__ to generic ffs()-family implementations"), which allows GCC's value range tracker to see past ffs(), GCC 8 on ARM thinks that it might be possible that "ffs(rq) - 8" used here:
v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8);
could wrap below 0, leading to a very large value, which would be out of range for the FIELD_PREP() usage:
drivers/pci/pci.c: In function 'pcie_set_readrq': include/linux/compiler_types.h:572:38: error: call to '__compiletime_assert_471' declared with attribute error: FIELD_PREP: value too large for the field ... drivers/pci/pci.c:5896:6: note: in expansion of macro 'FIELD_PREP' v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8); ^~~~~~~~~~
If the result of the ffs() is bounds checked before being used in FIELD_PREP(), the value tracker seems happy again. :)
Reported-by: Linux Kernel Functional Testing lkft@linaro.org Closes: https://lore.kernel.org/linux-pci/CA+G9fYuysVr6qT8bjF6f08WLyCJRG7aXAeSd2F7=z... Acked-by: Bjorn Helgaas bhelgaas@google.com Acked-by: Arnd Bergmann arnd@arndb.de Link: https://lore.kernel.org/r/20250905052836.work.425-kees@kernel.org Signed-off-by: Kees Cook kees@kernel.org Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
Based on my comprehensive analysis, here is my determination:
## Backport Status: **YES**
### Extensive Analysis:
#### Nature of the Fix: This commit adds a defensive check in `pcie_set_readrq()` to prevent a potential bit underflow when computing `ffs(rq) - 8`. Specifically, it: 1. Stores the result of `ffs(rq)` in a new variable `firstbit` 2. Adds a check: `if (firstbit < 8) return -EINVAL;` 3. Uses `firstbit - 8` instead of `ffs(rq) - 8` in the FIELD_PREP macro
#### Root Cause & Context: This is **not a runtime bug fix** - it's a **build fix** triggered by another optimization commit. The bitops commit (50675b8f5bd4e "bitops: Add __attribute_const__ to generic ffs()-family implementations") adds `__attribute_const__` to ffs() functions, enabling GCC's value range tracker to perform better static analysis. However, GCC-8's conservative analysis on ARM, RISCV, and MIPS architectures incorrectly determines that `ffs(rq) - 8` could underflow, causing a **compilation error** (not a warning):
``` drivers/pci/pci.c: In function 'pcie_set_readrq': include/linux/compiler_types.h:572:38: error: call to '__compiletime_assert_471' declared with attribute error: FIELD_PREP: value too large for the field ```
#### Why the Compiler is Wrong (but we still need to fix it): Examining the code flow in `pcie_set_readrq()` (drivers/pci/pci.c:5931-5968): 1. Initial validation: `if (rq < 128 || rq > 4096 || !is_power_of_2(rq))` ensures rq ≥ 128 2. Performance mode clamping: `if (mps < rq) rq = mps;` where mps comes from `pcie_get_mps()` 3. `pcie_get_mps()` returns `128 << FIELD_GET(...)`, which is always ≥ 128 (verified at line 5976-5984) 4. Therefore, `ffs(rq) >= ffs(128) = 8`, so underflow is impossible
However, since commit f67577118d115 (2013), `pcie_get_mps()` never returns an error, always returning valid values ≥ 128. The compiler cannot prove this through interprocedural analysis.
#### Critical Dependency: This commit is **tightly coupled** with the bitops commit. Evidence: - Both commits are signed off by Sasha Levin (autosel backports) - They appear consecutively in the git history (50675b8f5bd4e → 5385aceb86f2f) - The commit message explicitly states: "In preparation for the future commit" - Without this fix, **builds will fail** on GCC-8 ARM/RISCV/MIPS after bitops changes
#### Risk Assessment: **Minimal Risk:** - Small, localized change (6 lines added in one function) - Adds defensive validation that cannot break existing functionality - For all valid inputs (rq ≥ 128), the check passes through - Only rejects values that would have caused incorrect behavior anyway - No performance impact - No changes to critical kernel subsystems beyond PCI
#### Backporting Justification: 1. **Mandatory dependency**: Required if bitops commit is backported (which it is - commit 50675b8f5bd4e) 2. **Build fix**: Prevents compilation failures on supported compiler configurations 3. **Low risk**: Defensive check with no behavior change for valid inputs 4. **Follows stable rules**: Small, contained fix with clear purpose 5. **Multiple architectures affected**: ARM, RISCV, MIPS with GCC-8 6. **Upstream acceptance**: Acked by PCI maintainer (Bjorn Helgaas) and Arnd Bergmann
#### Supporting Evidence: - Reported by: Linux Kernel Functional Testing (LKFT) - Affects: GCC-8 on arm, riscv, mips architectures - First seen: Linux 6.17.0-rc3-next-20250828 - Bisected to: bitops __attribute_const__ commit - Acked-by: Bjorn Helgaas, Arnd Bergmann
### Conclusion: **YES - This commit should be backported** as it's a necessary build fix that must accompany the bitops optimization commit. Without it, stable kernels with the bitops changes will fail to build on GCC-8 ARM/RISCV/MIPS configurations, breaking supported build environments. The fix is minimal, defensive, and poses negligible regression risk.
drivers/pci/pci.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b0f4d98036cdd..005b92e6585e9 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5932,6 +5932,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq) { u16 v; int ret; + unsigned int firstbit; struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
if (rq < 128 || rq > 4096 || !is_power_of_2(rq)) @@ -5949,7 +5950,10 @@ int pcie_set_readrq(struct pci_dev *dev, int rq) rq = mps; }
- v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8); + firstbit = ffs(rq); + if (firstbit < 8) + return -EINVAL; + v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, firstbit - 8);
if (bridge->no_inc_mrrs) { int max_mrrs = pcie_get_readrq(dev);