[PATCH] fs/file: fix the check in find_next_fd()

[PATCH] fs/file: fix the check in find_next_fd()

[Yuntao Wang]

The maximum possible return value of find_next_zero_bit(fdt->full_fds_bits,
maxbit, bitbit) is maxbit. This return value, multiplied by BITS_PER_LONG,
gives the value of bitbit, which can never be greater than maxfd, it can
only be equal to maxfd at most, so the following check 'if (bitbit > maxfd)'
will never be true.

Moreover, when bitbit equals maxfd, it indicates that there are no unused
fds, and the function can directly return.

Fix this check.

Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
 fs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/file.c b/fs/file.c
index 8076aef9c210..7058901a2154 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -491,7 +491,7 @@ static unsigned int find_next_fd(struct fdtable *fdt, unsigned int start)
 	unsigned int bitbit = start / BITS_PER_LONG;
 
 	bitbit = find_next_zero_bit(fdt->full_fds_bits, maxbit, bitbit) * BITS_PER_LONG;
-	if (bitbit > maxfd)
+	if (bitbit >= maxfd)
 		return maxfd;
 	if (bitbit > start)
 		start = bitbit;
-- 
2.45.1

Re: [PATCH] fs/file: fix the check in find_next_fd()

[Jan Kara]

On Thu 30-05-24 00:06:56, Yuntao Wang wrote:
> The maximum possible return value of find_next_zero_bit(fdt->full_fds_bits,
> maxbit, bitbit) is maxbit. This return value, multiplied by BITS_PER_LONG,
> gives the value of bitbit, which can never be greater than maxfd, it can
> only be equal to maxfd at most, so the following check 'if (bitbit > maxfd)'
> will never be true.
> 
> Moreover, when bitbit equals maxfd, it indicates that there are no unused
> fds, and the function can directly return.
> 
> Fix this check.
> 
> Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>

Good point. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 8076aef9c210..7058901a2154 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -491,7 +491,7 @@ static unsigned int find_next_fd(struct fdtable *fdt, unsigned int start)
>  	unsigned int bitbit = start / BITS_PER_LONG;
>  
>  	bitbit = find_next_zero_bit(fdt->full_fds_bits, maxbit, bitbit) * BITS_PER_LONG;
> -	if (bitbit > maxfd)
> +	if (bitbit >= maxfd)
>  		return maxfd;
>  	if (bitbit > start)
>  		start = bitbit;
> -- 
> 2.45.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs/file: fix the check in find_next_fd()

[Al Viro]

On Thu, May 30, 2024 at 12:06:56AM +0800, Yuntao Wang wrote:
> The maximum possible return value of find_next_zero_bit(fdt->full_fds_bits,
> maxbit, bitbit) is maxbit. This return value, multiplied by BITS_PER_LONG,
> gives the value of bitbit, which can never be greater than maxfd, it can
> only be equal to maxfd at most, so the following check 'if (bitbit > maxfd)'
> will never be true.
> 
> Moreover, when bitbit equals maxfd, it indicates that there are no unused
> fds, and the function can directly return.
> 
> Fix this check.

Hmm...  The patch is correct, AFAICS.  I _think_ what happened is that
Linus decided to play it safe around the last word.  In the reality
->max_fds is always a multiple of BITS_PER_LONG, so there's no boundary
effects - a word can not cross the ->max_fds boundary, so "no zero
bits in full_fds_bits under max_fds/BITS_PER_LONG" does mean there's
no point checking in range starting at round_down(max_fds, BITS_PER_LONG).

Perhaps a comment along the lines of

        unsigned int maxfd = fdt->max_fds; // always a multiple of BITS_PER_LONG

would be useful in there...

Re: [PATCH] fs/file: fix the check in find_next_fd()

[Yuntao Wang]

On Wed, 29 May 2024 20:03:28 +0100, Al Viro <viro@zeniv.linux.org.uk> wrote:

> On Thu, May 30, 2024 at 12:06:56AM +0800, Yuntao Wang wrote:
> > The maximum possible return value of find_next_zero_bit(fdt->full_fds_bits,
> > maxbit, bitbit) is maxbit. This return value, multiplied by BITS_PER_LONG,
> > gives the value of bitbit, which can never be greater than maxfd, it can
> > only be equal to maxfd at most, so the following check 'if (bitbit > maxfd)'
> > will never be true.
> > 
> > Moreover, when bitbit equals maxfd, it indicates that there are no unused
> > fds, and the function can directly return.
> > 
> > Fix this check.
> 
> Hmm...  The patch is correct, AFAICS.  I _think_ what happened is that
> Linus decided to play it safe around the last word.  In the reality
> ->max_fds is always a multiple of BITS_PER_LONG, so there's no boundary
> effects - a word can not cross the ->max_fds boundary, so "no zero
> bits in full_fds_bits under max_fds/BITS_PER_LONG" does mean there's
> no point checking in range starting at round_down(max_fds, BITS_PER_LONG).

Yes.

> Perhaps a comment along the lines of
> 
>         unsigned int maxfd = fdt->max_fds; // always a multiple of BITS_PER_LONG
> 
> would be useful in there...

Actually, we can simplify this issue. When 'bitbit >= maxfd', it indicates that
there are no unused fds in 'fdt->open_fds', and we can directly return maxfd,
regardless of whether maxfd is a multiple of BITS_PER_LONG or not. Therefore, I
think this comment may not be very necessary.

Of course, I don't oppose adding this comment.

Re: [PATCH] fs/file: fix the check in find_next_fd()

[Christian Brauner]

On Thu, 30 May 2024 00:06:56 +0800, Yuntao Wang wrote:
> The maximum possible return value of find_next_zero_bit(fdt->full_fds_bits,
> maxbit, bitbit) is maxbit. This return value, multiplied by BITS_PER_LONG,
> gives the value of bitbit, which can never be greater than maxfd, it can
> only be equal to maxfd at most, so the following check 'if (bitbit > maxfd)'
> will never be true.
> 
> Moreover, when bitbit equals maxfd, it indicates that there are no unused
> fds, and the function can directly return.
> 
> [...]

Comment added as that's really useful in general.

---

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fs/file: fix the check in find_next_fd()
      https://git.kernel.org/vfs/vfs/c/96998332ac4d