After commit cbc654d18d37 ("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. :)
Fixes: cbc654d18d37 ("bitops: Add __attribute_const__ to generic ffs()-family implementations") Reported-by: Linux Kernel Functional Testing lkft@linaro.org Closes: https://lore.kernel.org/linux-pci/CA+G9fYuysVr6qT8bjF6f08WLyCJRG7aXAeSd2F7=z... Signed-off-by: Kees Cook kees@kernel.org --- Cc: Bjorn Helgaas bhelgaas@google.com Cc: Anders Roxell anders.roxell@linaro.org Cc: Naresh Kamboju naresh.kamboju@linaro.org Cc: lkft-triage@lists.linaro.org Cc: Linux Regressions regressions@lists.linux.dev Cc: Arnd Bergmann arnd@arndb.de Cc: Dan Carpenter dan.carpenter@linaro.org Cc: Ben Copeland benjamin.copeland@linaro.org Cc: lkft-triage@lists.linaro.org Cc: linux-pci@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- 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 b0f4d98036cd..005b92e6585e 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);
On Fri, 5 Sept 2025 at 07:28, Kees Cook kees@kernel.org wrote:
After commit cbc654d18d37 ("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. :)
Fixes: cbc654d18d37 ("bitops: Add __attribute_const__ to generic ffs()-family implementations") Reported-by: Linux Kernel Functional Testing lkft@linaro.org Closes: https://lore.kernel.org/linux-pci/CA+G9fYuysVr6qT8bjF6f08WLyCJRG7aXAeSd2F7=z... Signed-off-by: Kees Cook kees@kernel.org
Cc: Bjorn Helgaas bhelgaas@google.com Cc: Anders Roxell anders.roxell@linaro.org Cc: Naresh Kamboju naresh.kamboju@linaro.org Cc: lkft-triage@lists.linaro.org Cc: Linux Regressions regressions@lists.linux.dev Cc: Arnd Bergmann arnd@arndb.de Cc: Dan Carpenter dan.carpenter@linaro.org Cc: Ben Copeland benjamin.copeland@linaro.org Cc: lkft-triage@lists.linaro.org Cc: linux-pci@vger.kernel.org Cc: linux-kernel@vger.kernel.org
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 b0f4d98036cd..005b92e6585e 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);
Hi Kees,
Thank you for looking into this.
These warnings are not a one time thing. the later versions of gcc can figure it out that firstbit is at least 8 based on the "rq < 128" (i guess), so we're adding bogus code. maybe we should just disable the check for gcc-8.
Maybe something like this:
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index 5355f8f806a9..4716025c98c7 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -65,9 +65,20 @@ BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ _pfx "mask is not constant"); \ BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \ - BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ - ~((_mask) >> __bf_shf(_mask)) & \ - (0 + (_val)) : 0, \ + /* Value validation disabled for gcc < 9 due to __attribute_const__ issues. + */ \ + BUILD_BUG_ON_MSG(__GNUC__ >= 9 && __builtin_constant_p(_val) ? \ + ~((_mask) >> __bf_shf(_mask)) & \ + (0 + (_val)) : 0, \ _pfx "value too large for the field"); \ BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \ __bf_cast_unsigned(_reg, ~0ull), \
I found similar patterns with ffs and FIELD_PREP here drivers/dma/uniphier-xdmac.c row 156 and 165 drivers/gpu/drm/i915/display/intel_cursor_regs.h row 17
Cheers, Anders
On Fri, Sep 5, 2025, at 10:16, Anders Roxell wrote:
On Fri, 5 Sept 2025 at 07:28, Kees Cook kees@kernel.org wrote:
@@ -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);Hi Kees,
Thank you for looking into this.
These warnings are not a one time thing. the later versions of gcc can figure it out that firstbit is at least 8 based on the "rq < 128" (i guess), so we're adding bogus code. maybe we should just disable the check for gcc-8.
Out of the three failures I saw, two also happened with gcc-9, but gcc-10 looks clean so far.
\
(0 + (_val)) : 0, \ _pfx "value too large for the field"); \ BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \ __bf_cast_unsigned(_reg, ~0ull), \I found similar patterns with ffs and FIELD_PREP here drivers/dma/uniphier-xdmac.c row 156 and 165 drivers/gpu/drm/i915/display/intel_cursor_regs.h row 17
I did not come across build failures for these.
Arnd
On Fri, Sep 5, 2025, at 07:28, Kees Cook wrote:
After commit cbc654d18d37 ("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. :)
Fixes: cbc654d18d37 ("bitops: Add __attribute_const__ to generic ffs()-family implementations") Reported-by: Linux Kernel Functional Testing lkft@linaro.org Closes: https://lore.kernel.org/linux-pci/CA+G9fYuysVr6qT8bjF6f08WLyCJRG7aXAeSd2F7=z... Signed-off-by: Kees Cook kees@kernel.org
Acked-by: Arnd Bergmann arnd@arndb.de
This looks good to me individually, however I have now tried to do more randconfig tests with the __attribute_const change and gcc-8.5.0, and so far found two other files with the same issue:
In file included from <command-line>: In function 'mtk_dai_etdm_out_configure.constprop', inlined from 'mtk_dai_etdm_configure' at sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:2168:3: include/linux/compiler_types.h:575:38: error: call to '__compiletime_assert_416' declared with attribute error: FIELD_PREP: value too large for the field sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:2065:10: note: in expansion of macro 'FIELD_PREP' val |= FIELD_PREP(ETDM_OUT_CON4_FS_MASK, get_etdm_fs_timing(rate)); sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:1971:10: note: in expansion of macro 'FIELD_PREP' val |= FIELD_PREP(ETDM_IN_CON3_FS_MASK, get_etdm_fs_timing(rate));
drivers/mmc/host/meson-gx-mmc.c: In function 'meson_mmc_start_cmd': drivers/mmc/host/meson-gx-mmc.c:811:14: note: in expansion of macro 'FIELD_PREP' cmd_cfg |= FIELD_PREP(CMD_CFG_TIMEOUT_MASK,
This is fairly rare, but over time there are likely going to be others like them. I see three possible ways forward here:
a) fix them individually as we run into them, hoping for the best b) skip that one compile time check on older compilers, like Anders' new patch c) try to come up with a more robust FIELD_PREP() implementation
I already tried hacking on FIELD_PREP(), but my feeling is that it is already getting out of hand with its complexity, and needs to become simpler instead. The root cause for the warning here is apparently the way it evaluates the macro arguments multiple times, and that causes both extra compile-time complexity and extra code paths where part of the invocation goes through an inline function and another path that does not. It may be possible to change this in a way that moves the BUILD_BUG_ON() portions into an __always_inline function, and only uses the macro to deal with the type differences.
Arnd
-
Anders Roxell -
Arnd Bergmann -
Kees Cook