[PATCH] nvme: remove bogus check in nvme_pr_read_keys()

[PATCH] nvme: remove bogus check in nvme_pr_read_keys()

[Dan Carpenter]

This check for if (rse_len > U32_MAX) is confusing because if
rse_len is > INT_MAX, that will trigger a WARN() in kvzalloc().
Fortunately, the caller blkdev_pr_read_keys(), puts a limit on num_keys.
The number of keys can't be more than PR_KEYS_MAX (65536) and the
condition is impossible.

Delete the confusing, dead code.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/nvme/host/pr.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/nvme/host/pr.c b/drivers/nvme/host/pr.c
index fe7dbe264815..abab2746f0f8 100644
--- a/drivers/nvme/host/pr.c
+++ b/drivers/nvme/host/pr.c
@@ -239,9 +239,6 @@ static int nvme_pr_read_keys(struct block_device *bdev,
 	 * enough to get enough keys to fill the return keys buffer.
 	 */
 	rse_len = struct_size(rse, regctl_eds, num_keys);
-	if (rse_len > U32_MAX)
-		return -EINVAL;
-
 	rse = kvzalloc(rse_len, GFP_KERNEL);
 	if (!rse)
 		return -ENOMEM;
-- 
2.51.0

Re: [PATCH] nvme: remove bogus check in nvme_pr_read_keys()

[Keith Busch]

On Sat, Mar 21, 2026 at 01:26:25PM +0300, Dan Carpenter wrote:
> This check for if (rse_len > U32_MAX) is confusing because if
> rse_len is > INT_MAX, that will trigger a WARN() in kvzalloc().
> Fortunately, the caller blkdev_pr_read_keys(), puts a limit on num_keys.
> The number of keys can't be more than PR_KEYS_MAX (65536) and the
> condition is impossible.

There's actually two callers: blkdev_pr_read_keys() ensures the number of
keys is smaller than 65536 and iblock_pr_read_keys() is a fixed size at
16. But begs the question, what guarantee does nvme_pr_read_keys() have
that all the callers validated the number of keys such that it can
bravely skip checking it? I think nvme should validate that it's a
reasonable value before calling kvalloc so we return an apporpriate
EINVAL instead of ENOMEM. The existing UINT_MAX check is certainly far
too high, but I think something like a 4MB payload would be a totally
reasonable upper limit for nvme on this function.

Re: [PATCH] nvme: remove bogus check in nvme_pr_read_keys()

[Christoph Hellwig]

On Mon, Mar 23, 2026 at 11:53:23AM -0600, Keith Busch wrote:
> On Sat, Mar 21, 2026 at 01:26:25PM +0300, Dan Carpenter wrote:
> > This check for if (rse_len > U32_MAX) is confusing because if
> > rse_len is > INT_MAX, that will trigger a WARN() in kvzalloc().
> > Fortunately, the caller blkdev_pr_read_keys(), puts a limit on num_keys.
> > The number of keys can't be more than PR_KEYS_MAX (65536) and the
> > condition is impossible.
> 
> There's actually two callers: blkdev_pr_read_keys() ensures the number of
> keys is smaller than 65536 and iblock_pr_read_keys() is a fixed size at
> 16. But begs the question, what guarantee does nvme_pr_read_keys() have
> that all the callers validated the number of keys such that it can
> bravely skip checking it? I think nvme should validate that it's a
> reasonable value before calling kvalloc so we return an apporpriate
> EINVAL instead of ENOMEM. The existing UINT_MAX check is certainly far
> too high, but I think something like a 4MB payload would be a totally
> reasonable upper limit for nvme on this function.

Agreed.

Re: [PATCH] nvme: remove bogus check in nvme_pr_read_keys()

[Dan Carpenter]

On Mon, Mar 23, 2026 at 11:53:23AM -0600, Keith Busch wrote:
> On Sat, Mar 21, 2026 at 01:26:25PM +0300, Dan Carpenter wrote:
> > This check for if (rse_len > U32_MAX) is confusing because if
> > rse_len is > INT_MAX, that will trigger a WARN() in kvzalloc().
> > Fortunately, the caller blkdev_pr_read_keys(), puts a limit on num_keys.
> > The number of keys can't be more than PR_KEYS_MAX (65536) and the
> > condition is impossible.
> 
> There's actually two callers: blkdev_pr_read_keys() ensures the number of
> keys is smaller than 65536 and iblock_pr_read_keys() is a fixed size at
> 16. But begs the question, what guarantee does nvme_pr_read_keys() have
> that all the callers validated the number of keys such that it can
> bravely skip checking it?

We normally wouldn't check the return from struct_size().  We would just
pass it to the allocation function and let the failure happen since
nothing can allocate SIZE_MAX.

Linus added the INT_MAX check in kvzalloc() because it used to allocate
more but we capped it at INT_MAX to avoid a problem where sometimes
people store sizes int a u32.  vmalloc() can still allocate larger
sizes than that if you really need to.  Linus has since suggested that
the WARN() could be removed if people want to since hopefully all the
people who were using kvmalloc() to allocate more than 2GB have
changed to vmalloc() now.  So far no one has done that.

> I think nvme should validate that it's a
> reasonable value before calling kvalloc so we return an apporpriate
> EINVAL instead of ENOMEM. The existing UINT_MAX check is certainly far
> too high, but I think something like a 4MB payload would be a totally
> reasonable upper limit for nvme on this function.

That also works.

regards,
dan carpenter