fs/file: fix the check in find_next_fd()
PATCH 1/1 fs/file: fix the check in find_next_fd()
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.
--- fs/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/file.c +1 -1
@@ -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;
Signature · 1 line
2.45.1
Quoted 6 lines · Yuntao Wang
The maximum possible return value of
find_next_zero_bit(fdt->full_fds_bits, maxbit, bitbit)is maxbit. This return value, multiplied byBITS_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.
Quoted 6 lines
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 <[email protected]>
Good point. Feel free to add:
Honza
Quoted 20 lines
--- fs/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/file.c +1
-1@@ -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;Signature · 1 line
2.45.1
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Al Viro
maintainerQuoted 10 lines · Yuntao Wang
The maximum possible return value of
find_next_zero_bit(fdt->full_fds_bits, maxbit, bitbit)is maxbit. This return value, multiplied byBITS_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...
Quoted 10 lines · Al Viro
Quoted 9 lines · Yuntao Wang
The maximum possible return value of
find_next_zero_bit(fdt->full_fds_bits, maxbit, bitbit)is maxbit. This return value, multiplied byBITS_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.
Quoted 8 lines
Quoted 1 line
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 infull_fds_bitsunder max_fds/BITS_PER_LONG" does mean there's no point checking in range starting atround_down(max_fds, BITS_PER_LONG).
Yes.
Quoted 5 lines
Perhaps a comment along the lines of
unsigned int maxfd = fdt->max_fds; // always a multiple of BITS_PER_LONGwould 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.
Quoted 10 lines · Yuntao Wang
The maximum possible return value of
find_next_zero_bit(fdt->full_fds_bits, maxbit, bitbit)is maxbit. This return value, multiplied byBITS_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