Message ID | 20240602204238.GD1629371@ZenIV (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [RFC] move close_range(2) into fs/file.c, fold __close_range() into it | expand |
On Sun, Jun 02, 2024 at 09:42:38PM +0100, Al Viro wrote: > We never had callers for __close_range() except for close_range(2) > itself. Nothing of that sort has appeared in four years and if any users > do show up, we can always separate those suckers again. BTW, looking through close_range()... We have static unsigned int sane_fdtable_size(struct fdtable *fdt, unsigned int max_fds) { unsigned int count; count = count_open_files(fdt); if (max_fds < NR_OPEN_DEFAULT) max_fds = NR_OPEN_DEFAULT; return ALIGN(min(count, max_fds), BITS_PER_LONG); } which decides how large a table would be needed for descriptors below max_fds that are opened in fdt. And we start with finding the last opened descriptor in fdt (well, rounded up to BITS_PER_LONG, if you look at count_open_files()). Why do we bother to look at _anything_ past the max_fds? Does anybody have objections to the following? diff --git a/fs/file.c b/fs/file.c index f9fcebc7c838..4976ede108e0 100644 --- a/fs/file.c +++ b/fs/file.c @@ -276,20 +276,6 @@ static inline bool fd_is_open(unsigned int fd, const struct fdtable *fdt) return test_bit(fd, fdt->open_fds); } -static unsigned int count_open_files(struct fdtable *fdt) -{ - unsigned int size = fdt->max_fds; - unsigned int i; - - /* Find the last open fd */ - for (i = size / BITS_PER_LONG; i > 0; ) { - if (fdt->open_fds[--i]) - break; - } - i = (i + 1) * BITS_PER_LONG; - return i; -} - /* * Note that a sane fdtable size always has to be a multiple of * BITS_PER_LONG, since we have bitmaps that are sized by this. @@ -305,12 +291,18 @@ static unsigned int count_open_files(struct fdtable *fdt) */ static unsigned int sane_fdtable_size(struct fdtable *fdt, unsigned int max_fds) { - unsigned int count; + const unsigned int min_words = BITS_TO_LONGS(NR_OPEN_DEFAULT); // 1 + unsigned int words; + + if (max_fds <= NR_OPEN_DEFAULT) + return NR_OPEN_DEFAULT; + + words = BITS_TO_LONGS(min(max_fds, fdt->max_fds)); // >= min_words + + while (words > min_words && !fdt->open_fds[words - 1]) + words--; - count = count_open_files(fdt); - if (max_fds < NR_OPEN_DEFAULT) - max_fds = NR_OPEN_DEFAULT; - return ALIGN(min(count, max_fds), BITS_PER_LONG); + return words * BITS_PER_LONG; } /*
On Sun, Jun 02, 2024 at 09:42:38PM +0100, Al Viro wrote: > We never had callers for __close_range() except for close_range(2) > itself. Nothing of that sort has appeared in four years and if any users > do show up, we can always separate those suckers again. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- Note, that the __close_range() solely existed (iirc) because want close(2) and close_range(2) shouldn't end up in two separate files because it is royally annoying to have to switch files for system calls that conceptually do very similar things. But I don't care enough so, Reviewed-by: Christian Brauner <brauner@kernel.org> > diff --git a/fs/file.c b/fs/file.c > index 8076aef9c210..f9fcebc7c838 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -732,7 +732,7 @@ static inline void __range_close(struct files_struct *files, unsigned int fd, > } > > /** > - * __close_range() - Close all file descriptors in a given range. > + * sys_close_range() - Close all file descriptors in a given range. > * > * @fd: starting file descriptor to close > * @max_fd: last file descriptor to close > @@ -740,8 +740,10 @@ static inline void __range_close(struct files_struct *files, unsigned int fd, > * > * This closes a range of file descriptors. All file descriptors > * from @fd up to and including @max_fd are closed. > + * Currently, errors to close a given file descriptor are ignored. > */ > -int __close_range(unsigned fd, unsigned max_fd, unsigned int flags) > +SYSCALL_DEFINE3(close_range, unsigned int, fd, unsigned int, max_fd, > + unsigned int, flags) > { > struct task_struct *me = current; > struct files_struct *cur_fds = me->files, *fds = NULL; > diff --git a/fs/open.c b/fs/open.c > index 89cafb572061..7ee11c4de4ca 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -1564,23 +1564,6 @@ SYSCALL_DEFINE1(close, unsigned int, fd) > return retval; > } > > -/** > - * sys_close_range() - Close all file descriptors in a given range. > - * > - * @fd: starting file descriptor to close > - * @max_fd: last file descriptor to close > - * @flags: reserved for future extensions > - * > - * This closes a range of file descriptors. All file descriptors > - * from @fd up to and including @max_fd are closed. > - * Currently, errors to close a given file descriptor are ignored. > - */ > -SYSCALL_DEFINE3(close_range, unsigned int, fd, unsigned int, max_fd, > - unsigned int, flags) > -{ > - return __close_range(fd, max_fd, flags); > -} > - > /* > * This routine simulates a hangup on the tty, to arrange that users > * are given clean terminals at login time. > diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h > index 2944d4aa413b..4e7d1bcca5b7 100644 > --- a/include/linux/fdtable.h > +++ b/include/linux/fdtable.h > @@ -113,7 +113,6 @@ int iterate_fd(struct files_struct *, unsigned, > const void *); > > extern int close_fd(unsigned int fd); > -extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags); > extern struct file *file_close_fd(unsigned int fd); > extern int unshare_fd(unsigned long unshare_flags, unsigned int max_fds, > struct files_struct **new_fdp);
On Sun, Jun 02, 2024 at 10:58:03PM +0100, Al Viro wrote: > On Sun, Jun 02, 2024 at 09:42:38PM +0100, Al Viro wrote: > > We never had callers for __close_range() except for close_range(2) > > itself. Nothing of that sort has appeared in four years and if any users > > do show up, we can always separate those suckers again. > > BTW, looking through close_range()... We have > > static unsigned int sane_fdtable_size(struct fdtable *fdt, unsigned int max_fds) > { > unsigned int count; > > count = count_open_files(fdt); > if (max_fds < NR_OPEN_DEFAULT) > max_fds = NR_OPEN_DEFAULT; > return ALIGN(min(count, max_fds), BITS_PER_LONG); > } > > which decides how large a table would be needed for descriptors below max_fds > that are opened in fdt. And we start with finding the last opened descriptor > in fdt (well, rounded up to BITS_PER_LONG, if you look at count_open_files()). > > Why do we bother to look at _anything_ past the max_fds? Does anybody have > objections to the following? No, it's fine but folding count_open_files() into sane_fdtable_size() is a bit less readable imho. In any case, could you please slap an explanatory comment on top of while (words > min_words && !fdt->open_fds[words - 1]) words--; That code is unreadable enough as it is. > > diff --git a/fs/file.c b/fs/file.c > index f9fcebc7c838..4976ede108e0 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -276,20 +276,6 @@ static inline bool fd_is_open(unsigned int fd, const struct fdtable *fdt) > return test_bit(fd, fdt->open_fds); > } > > -static unsigned int count_open_files(struct fdtable *fdt) > -{ > - unsigned int size = fdt->max_fds; > - unsigned int i; > - > - /* Find the last open fd */ > - for (i = size / BITS_PER_LONG; i > 0; ) { > - if (fdt->open_fds[--i]) > - break; > - } > - i = (i + 1) * BITS_PER_LONG; > - return i; > -} > - > /* > * Note that a sane fdtable size always has to be a multiple of > * BITS_PER_LONG, since we have bitmaps that are sized by this. > @@ -305,12 +291,18 @@ static unsigned int count_open_files(struct fdtable *fdt) > */ > static unsigned int sane_fdtable_size(struct fdtable *fdt, unsigned int max_fds) > { > - unsigned int count; > + const unsigned int min_words = BITS_TO_LONGS(NR_OPEN_DEFAULT); // 1 > + unsigned int words; > + > + if (max_fds <= NR_OPEN_DEFAULT) > + return NR_OPEN_DEFAULT; > + > + words = BITS_TO_LONGS(min(max_fds, fdt->max_fds)); // >= min_words > + > + while (words > min_words && !fdt->open_fds[words - 1]) > + words--; > > - count = count_open_files(fdt); > - if (max_fds < NR_OPEN_DEFAULT) > - max_fds = NR_OPEN_DEFAULT; > - return ALIGN(min(count, max_fds), BITS_PER_LONG); > + return words * BITS_PER_LONG; > } > > /*
diff --git a/fs/file.c b/fs/file.c index 8076aef9c210..f9fcebc7c838 100644 --- a/fs/file.c +++ b/fs/file.c @@ -732,7 +732,7 @@ static inline void __range_close(struct files_struct *files, unsigned int fd, } /** - * __close_range() - Close all file descriptors in a given range. + * sys_close_range() - Close all file descriptors in a given range. * * @fd: starting file descriptor to close * @max_fd: last file descriptor to close @@ -740,8 +740,10 @@ static inline void __range_close(struct files_struct *files, unsigned int fd, * * This closes a range of file descriptors. All file descriptors * from @fd up to and including @max_fd are closed. + * Currently, errors to close a given file descriptor are ignored. */ -int __close_range(unsigned fd, unsigned max_fd, unsigned int flags) +SYSCALL_DEFINE3(close_range, unsigned int, fd, unsigned int, max_fd, + unsigned int, flags) { struct task_struct *me = current; struct files_struct *cur_fds = me->files, *fds = NULL; diff --git a/fs/open.c b/fs/open.c index 89cafb572061..7ee11c4de4ca 100644 --- a/fs/open.c +++ b/fs/open.c @@ -1564,23 +1564,6 @@ SYSCALL_DEFINE1(close, unsigned int, fd) return retval; } -/** - * sys_close_range() - Close all file descriptors in a given range. - * - * @fd: starting file descriptor to close - * @max_fd: last file descriptor to close - * @flags: reserved for future extensions - * - * This closes a range of file descriptors. All file descriptors - * from @fd up to and including @max_fd are closed. - * Currently, errors to close a given file descriptor are ignored. - */ -SYSCALL_DEFINE3(close_range, unsigned int, fd, unsigned int, max_fd, - unsigned int, flags) -{ - return __close_range(fd, max_fd, flags); -} - /* * This routine simulates a hangup on the tty, to arrange that users * are given clean terminals at login time. diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h index 2944d4aa413b..4e7d1bcca5b7 100644 --- a/include/linux/fdtable.h +++ b/include/linux/fdtable.h @@ -113,7 +113,6 @@ int iterate_fd(struct files_struct *, unsigned, const void *); extern int close_fd(unsigned int fd); -extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags); extern struct file *file_close_fd(unsigned int fd); extern int unshare_fd(unsigned long unshare_flags, unsigned int max_fds, struct files_struct **new_fdp);
We never had callers for __close_range() except for close_range(2) itself. Nothing of that sort has appeared in four years and if any users do show up, we can always separate those suckers again. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> ---