[PATCH] nvme-pci: Make nvme_pci_npages_prp() __always_inline

[PATCH] nvme-pci: Make nvme_pci_npages_prp() __always_inline

[Kees Cook]

The only reason nvme_pci_npages_prp() could be used as a compile-time
known result in BUILD_BUG_ON() is because the compiler was always choosing
to inline the function. Under special circumstances (sanitizer coverage
functions disabled for __init functions on ARCH=um), the compiler decided
to stop inlining it:

   drivers/nvme/host/pci.c: In function 'nvme_init':
   include/linux/compiler_types.h:557:45: error: call to '__compiletime_assert_678' declared with attribute error: BUILD_BUG_ON failed: nvme_pci_npages_prp() > NVME_MAX_NR_ALLOCATIONS
     557 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |                                             ^
   include/linux/compiler_types.h:538:25: note: in definition of macro '__compiletime_assert'
     538 |                         prefix ## suffix();                             \
         |                         ^~~~~~
   include/linux/compiler_types.h:557:9: note: in expansion of macro '_compiletime_assert'
     557 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:50:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
      50 |         BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
         |         ^~~~~~~~~~~~~~~~
   drivers/nvme/host/pci.c:3804:9: note: in expansion of macro 'BUILD_BUG_ON'
    3804 |         BUILD_BUG_ON(nvme_pci_npages_prp() > NVME_MAX_NR_ALLOCATIONS);
         |         ^~~~~~~~~~~~

Force it to be __always_inline to make sure it is always available for
use with BUILD_BUG_ON().

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202505061846.12FMyRjj-lkp@intel.com/
Fixes: c372cdd1efdf ("nvme-pci: iod npages fits in s8")
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Keith Busch <kbusch@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: <linux-nvme@lists.infradead.org>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b178d52eac1b..9ab070a9f037 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -390,7 +390,7 @@ static bool nvme_dbbuf_update_and_check_event(u16 value, __le32 *dbbuf_db,
  * as it only leads to a small amount of wasted memory for the lifetime of
  * the I/O.
  */
-static int nvme_pci_npages_prp(void)
+static __always_inline int nvme_pci_npages_prp(void)
 {
 	unsigned max_bytes = (NVME_MAX_KB_SZ * 1024) + NVME_CTRL_PAGE_SIZE;
 	unsigned nprps = DIV_ROUND_UP(max_bytes, NVME_CTRL_PAGE_SIZE);
-- 
2.34.1

Re: [PATCH] nvme-pci: Make nvme_pci_npages_prp() __always_inline

[Christoph Hellwig]

On Tue, May 06, 2025 at 08:35:40PM -0700, Kees Cook wrote:
> The only reason nvme_pci_npages_prp() could be used as a compile-time
> known result in BUILD_BUG_ON() is because the compiler was always choosing
> to inline the function. Under special circumstances (sanitizer coverage
> functions disabled for __init functions on ARCH=um), the compiler decided
> to stop inlining it:

Can we place just fix um to still force inlining inline functions instead
of needing these workarounds?

Re: [PATCH] nvme-pci: Make nvme_pci_npages_prp() __always_inline

[Kees Cook]

On Wed, May 07, 2025 at 06:47:54AM +0200, Christoph Hellwig wrote:
> On Tue, May 06, 2025 at 08:35:40PM -0700, Kees Cook wrote:
> > The only reason nvme_pci_npages_prp() could be used as a compile-time
> > known result in BUILD_BUG_ON() is because the compiler was always choosing
> > to inline the function. Under special circumstances (sanitizer coverage
> > functions disabled for __init functions on ARCH=um), the compiler decided
> > to stop inlining it:
> 
> Can we place just fix um to still force inlining inline functions instead
> of needing these workarounds?

Oh, I don't have the history here. Is there something about UM and
forcing off inlining?

-- 
Kees Cook

Re: [PATCH] nvme-pci: Make nvme_pci_npages_prp() __always_inline

[Christoph Hellwig]

On Tue, May 06, 2025 at 10:55:31PM -0700, Kees Cook wrote:
> On Wed, May 07, 2025 at 06:47:54AM +0200, Christoph Hellwig wrote:
> > On Tue, May 06, 2025 at 08:35:40PM -0700, Kees Cook wrote:
> > > The only reason nvme_pci_npages_prp() could be used as a compile-time
> > > known result in BUILD_BUG_ON() is because the compiler was always choosing
> > > to inline the function. Under special circumstances (sanitizer coverage
> > > functions disabled for __init functions on ARCH=um), the compiler decided
> > > to stop inlining it:
> > 
> > Can we place just fix um to still force inlining inline functions instead
> > of needing these workarounds?
> 
> Oh, I don't have the history here. Is there something about UM and
> forcing off inlining?

Maybe I'm misunderstandng your report, but what causes the failure
to inline?

Re: [PATCH] nvme-pci: Make nvme_pci_npages_prp() __always_inline

[Kees Cook]

On Wed, May 07, 2025 at 08:59:13AM +0200, Christoph Hellwig wrote:
> On Tue, May 06, 2025 at 10:55:31PM -0700, Kees Cook wrote:
> > On Wed, May 07, 2025 at 06:47:54AM +0200, Christoph Hellwig wrote:
> > > On Tue, May 06, 2025 at 08:35:40PM -0700, Kees Cook wrote:
> > > > The only reason nvme_pci_npages_prp() could be used as a compile-time
> > > > known result in BUILD_BUG_ON() is because the compiler was always choosing
> > > > to inline the function. Under special circumstances (sanitizer coverage
> > > > functions disabled for __init functions on ARCH=um), the compiler decided
> > > > to stop inlining it:
> > > 
> > > Can we place just fix um to still force inlining inline functions instead
> > > of needing these workarounds?
> > 
> > Oh, I don't have the history here. Is there something about UM and
> > forcing off inlining?
> 
> Maybe I'm misunderstandng your report, but what causes the failure
> to inline?

I don't know precisely, but whatever internal heuristics the compiler
uses to change a function from "static" to "static inline" got disrupted
by the build options, and manifested with this failure. It's fully
reproducible on all architectures if I mark the function as "noinline".
:)

So, the solution for the "accidentally depending on a function to be
inlined by the compiler" is to mark it as _required_ to be inlined,
which given its singular use in BUILD_BUG_ON(), looks like the correct
solution.

I took your comment about ARCH=um to mean there was some kind of
long-standing "UM regularly fails to inline stuff; can we fix UM
instead?" But regardless, I think this patch is still correct given
that the compiler could, at any time, decide to make this function not
inline, since it's not marked that way at all (but its usage depends on
it being inline).

-Kees

-- 
Kees Cook

Re: [PATCH] nvme-pci: Make nvme_pci_npages_prp() __always_inline

[Keith Busch]

On Wed, May 07, 2025 at 08:53:06AM -0700, Kees Cook wrote:
> 
> So, the solution for the "accidentally depending on a function to be
> inlined by the compiler" is to mark it as _required_ to be inlined,
> which given its singular use in BUILD_BUG_ON(), looks like the correct
> solution.

That sounds fine to me. Or maybe this should be a #define instead. It
looks like this nvme function evolved from a very different purpose when
it wasn't an integer constant expression, and the comment above it is
outdated.

Re: [PATCH] nvme-pci: Make nvme_pci_npages_prp() __always_inline

[Christoph Hellwig]

Thank,

applied to nvme-6.15.