| Message ID | 20240529160656.209352-1-yuntao.wang@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs/file: fix the check in find_next_fd() | expand |
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 >
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...
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.
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
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;
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(-)