diff mbox series

[RFC] move close_range(2) into fs/file.c, fold __close_range() into it

Message ID 20240602204238.GD1629371@ZenIV (mailing list archive)
State New, archived
Headers show
Series [RFC] move close_range(2) into fs/file.c, fold __close_range() into it | expand

Commit Message

Al Viro June 2, 2024, 8:42 p.m. UTC 
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>
---

Comments

Al Viro June 2, 2024, 9:58 p.m. UTC | #1 
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;
 }
 
 /*
Christian Brauner June 3, 2024, 1:56 p.m. UTC | #2 
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);
Christian Brauner June 3, 2024, 1:59 p.m. UTC | #3 
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 mbox series

Patch

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);