diff mbox series

io: Refactor close_range and closefrom

Message ID 20211108135800.2189664-1-adhemerval.zanella@linaro.org
State New
Headers show
Series io: Refactor close_range and closefrom | expand

Commit Message

Adhemerval Zanella Netto Nov. 8, 2021, 1:58 p.m. UTC 
Now that Hurd implementis both close_range and closefrom (f2c996597d),
we can make close_range() a base ABI, and make the default closefrom()
implementation on top of close_range().

The generic closefrom() implementation based on __getdtablesize() is
moved to generic close_range().  On Linux it will be overriden by
the auto-generation syscall while on Hurd it will be a system specific
implementation.

The closefrom() now calls close_range() and __closefrom_fallback().
Since on Hurd close_range() does not fail, __closefrom_fallback() is an
empty static inline function set by__ASSUME_CLOSE_RANGE.

The __ASSUME_CLOSE_RANGE also allows optimize Linux
__closefrom_fallback() implementation when --enable-kernel=5.9 or
higher is used.

Finally the Linux specific tst-close_range.c is moved to io and
enabled as default.  The Linuxism and CLOSE_RANGE_UNSHARE are
guarded so it can be built for Hurd (I have not actually test it).

Checked on x86_64-linux-gnu, i686-linux-gnu, and with a i686-gnu
build.
---
 include/unistd.h                              | 10 ++++++
 io/Makefile                                   |  3 +-
 .../linux/closefrom.c => io/close_range.c     | 34 ++++++++++++-------
 io/closefrom.c                                | 16 +++++----
 .../unix/sysv/linux => io}/tst-close_range.c  |  8 +++++
 posix/unistd.h                                | 11 ++++++
 sysdeps/mach/hurd/Makefile                    |  2 +-
 sysdeps/mach/hurd/bits/unistd_ext.h           |  6 ----
 sysdeps/mach/hurd/closefrom.c                 | 29 ----------------
 sysdeps/mach/hurd/kernel-features.h           |  2 ++
 sysdeps/unix/sysv/linux/Makefile              |  1 -
 sysdeps/unix/sysv/linux/bits/unistd_ext.h     |  9 -----
 sysdeps/unix/sysv/linux/closefrom_fallback.c  |  4 +++
 sysdeps/unix/sysv/linux/kernel-features.h     |  8 +++++
 sysdeps/unix/sysv/linux/syscalls.list         |  2 +-
 15 files changed, 77 insertions(+), 68 deletions(-)
 rename sysdeps/unix/sysv/linux/closefrom.c => io/close_range.c (59%)
 rename {sysdeps/unix/sysv/linux => io}/tst-close_range.c (98%)
 delete mode 100644 sysdeps/mach/hurd/closefrom.c

Comments

Sergey Bugaev Nov. 8, 2021, 2:13 p.m. UTC | #1 
Hello,

I hope it's appropriate for me to leave some comments on the patch...

On Mon, Nov 8, 2021 at 4:58 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> +#if __ASSUME_CLOSE_RANGE
> +static inline _Bool __closefrom_fallback (int __lowfd, _Bool dirfd_fallback)
> +{
> +  return true;
> +}

My understanding is that this will get called if we assume close_range
is available, but it still somehow fails. Should it maybe return false
in this case, to cause closefrom to fail/abort, rather than silently
succeeding?

> +stub_warning (close_range)

Wouldn't this warn that "close_range is not implemented and will
always fail"? I'd expect that warning for a stub that always fails
with ENOSYS, but not for a working (if non-atomic etc) implementation.

> --- a/posix/unistd.h
> +++ b/posix/unistd.h
> @@ -1199,6 +1199,17 @@ int getentropy (void *__buffer, size_t __length) __wur
>      __attr_access ((__write_only__, 1, 2));
>  #endif
>
> +#ifdef __USE_GNU
> +/* Close all file descriptors in the range FD up to MAX_FD.  The flag FLAGS
> +   are define by the CLOSE_RANGE prefix.  This function behaves like close
> +   on the range, but in a fail-safe where it will either fail and not close
> +   any file descriptor or close all of them.  Gaps where the file descriptor

Is this (either closes everything or nothing) an appropriate thing to
promise in the common header? Similarly, if the default implementation
accepts no flags, should the common description mention "the
CLOSE_RANGE prefix"?

Sergey
Adhemerval Zanella Netto Nov. 8, 2021, 2:55 p.m. UTC | #2 
On 08/11/2021 11:13, Sergey Bugaev wrote:
> Hello,
> 
> I hope it's appropriate for me to leave some comments on the patch...
> 
> On Mon, Nov 8, 2021 at 4:58 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> +#if __ASSUME_CLOSE_RANGE
>> +static inline _Bool __closefrom_fallback (int __lowfd, _Bool dirfd_fallback)
>> +{
>> +  return true;
>> +}
> 
> My understanding is that this will get called if we assume close_range
> is available, but it still somehow fails. Should it maybe return false
> in this case, to cause closefrom to fail/abort, rather than silently
> succeeding?

Indeed it makes sense to return false and force the __fortify_fail() on
the generic implementation.

> 
>> +stub_warning (close_range)
> 
> Wouldn't this warn that "close_range is not implemented and will
> always fail"? I'd expect that warning for a stub that always fails
> with ENOSYS, but not for a working (if non-atomic etc) implementation.

Yeah, this does not make sense. I will remove it.

> 
>> --- a/posix/unistd.h
>> +++ b/posix/unistd.h
>> @@ -1199,6 +1199,17 @@ int getentropy (void *__buffer, size_t __length) __wur
>>      __attr_access ((__write_only__, 1, 2));
>>  #endif
>>
>> +#ifdef __USE_GNU
>> +/* Close all file descriptors in the range FD up to MAX_FD.  The flag FLAGS
>> +   are define by the CLOSE_RANGE prefix.  This function behaves like close
>> +   on the range, but in a fail-safe where it will either fail and not close
>> +   any file descriptor or close all of them.  Gaps where the file descriptor
> 
> Is this (either closes everything or nothing) an appropriate thing to
> promise in the common header? Similarly, if the default implementation
> accepts no flags, should the common description mention "the
> CLOSE_RANGE prefix"?

Well, that's the semantic of the both the syscall and the Linux fallback.
If Hurd does not provide such semantic I think you should work this out.

For CLOSE_RANGE I think it does make sense because it is not setting
any support flag, but rather the way it might be provided by the system.
Sergey Bugaev Nov. 8, 2021, 3:13 p.m. UTC | #3 
On Mon, Nov 8, 2021 at 5:55 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> > Is this (either closes everything or nothing) an appropriate thing to
> > promise in the common header? Similarly, if the default implementation
> > accepts no flags, should the common description mention "the
> > CLOSE_RANGE prefix"?
>
> Well, that's the semantic of the both the syscall and the Linux fallback.
> If Hurd does not provide such semantic I think you should work this out.

My Hurd patch does provide that semantic. I'm concerned about this
being promised in the generic header; some other port/kernel could
potentially decide to instead stop & propagate an error from closing a
descriptor. Perhaps the claim could be qualified (for instance, "In
all current ports, ...")? Not that this matters.

Another suggestion:

> +  for (int i = 0; i < maxfd; i++)
> +    if (i >= lowfd)
> +      __close_nocancel_nostatus (i);

This should be

int i;
for (i = first; i <= last && i < maxfd; i++)
  __close_nocancel_nostatus (i);

shouldn't it?

How does that even compile? 'lowfd' is the name of closefrom's
argument. close_range's arguments are 'first' and 'last' (and
'flags').

Sergey
Adhemerval Zanella Netto Nov. 8, 2021, 5:04 p.m. UTC | #4 
On 08/11/2021 12:13, Sergey Bugaev wrote:
> On Mon, Nov 8, 2021 at 5:55 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>> Is this (either closes everything or nothing) an appropriate thing to
>>> promise in the common header? Similarly, if the default implementation
>>> accepts no flags, should the common description mention "the
>>> CLOSE_RANGE prefix"?
>>
>> Well, that's the semantic of the both the syscall and the Linux fallback.
>> If Hurd does not provide such semantic I think you should work this out.
> 
> My Hurd patch does provide that semantic. I'm concerned about this
> being promised in the generic header; some other port/kernel could
> potentially decide to instead stop & propagate an error from closing a
> descriptor. Perhaps the claim could be qualified (for instance, "In
> all current ports, ...")? Not that this matters.

I think it worth to align with other two current implementation (Linux
and FreeBSD) where errors are indeed ignored.  I changed to:

/* Close all file descriptors in the range FD up to MAX_FD.  The flag FLAGS
   are defined by the CLOSE_RANGE prefix.  This function behaves like close
   on the range and gaps where the file descriptor is invalid or errors
   encountered while closing file descriptors are ignored.   Returns 0 on
   successor or -1 for failure (and sets errno accordingly).  */


> 
> Another suggestion:
> 
>> +  for (int i = 0; i < maxfd; i++)
>> +    if (i >= lowfd)
>> +      __close_nocancel_nostatus (i);
> 
> This should be
> 
> int i;
> for (i = first; i <= last && i < maxfd; i++)
>   __close_nocancel_nostatus (i);
> 
> shouldn't it?
> 
> How does that even compile? 'lowfd' is the name of closefrom's
> argument. close_range's arguments are 'first' and 'last' (and
> 'flags').

It does not, mostly because it is not really used anywhere. I have
fixed it, thanks.
diff mbox series

Patch

diff --git a/include/unistd.h b/include/unistd.h
index 7849562c42..6cd806f7f6 100644
--- a/include/unistd.h
+++ b/include/unistd.h
@@ -3,6 +3,9 @@ 
 
 # ifndef _ISOMAC
 
+#  include <stdbool.h>
+#  include <kernel-features.h>
+
 libc_hidden_proto (_exit, __noreturn__)
 #  ifndef NO_RTLD_HIDDEN
 rtld_hidden_proto (_exit, __noreturn__)
@@ -158,7 +161,14 @@  extern int __brk (void *__addr) attribute_hidden;
 extern int __close (int __fd);
 libc_hidden_proto (__close)
 extern int __libc_close (int __fd);
+#if __ASSUME_CLOSE_RANGE
+static inline _Bool __closefrom_fallback (int __lowfd, _Bool dirfd_fallback)
+{
+  return true;
+}
+#else
 extern _Bool __closefrom_fallback (int __lowfd, _Bool) attribute_hidden;
+#endif
 extern ssize_t __read (int __fd, void *__buf, size_t __nbytes);
 libc_hidden_proto (__read)
 extern ssize_t __write (int __fd, const void *__buf, size_t __n);
diff --git a/io/Makefile b/io/Makefile
index ecf65aba60..83f6ffdb76 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -57,7 +57,7 @@  routines :=								\
 	utimensat futimens file_change_detection			\
 	fts64-time64							\
 	ftw64-time64							\
-	closefrom
+	closefrom close_range
 
 others		:= pwd
 test-srcs	:= ftwtest ftwtest-time64
@@ -79,6 +79,7 @@  tests		:= test-utime test-stat test-stat2 test-lfs tst-getcwd \
 		   tst-futimens \
 		   tst-utimensat \
 		   tst-closefrom \
+		   tst-close_range \
 		   tst-ftw-bz28126
 
 tests-time64 := \
diff --git a/sysdeps/unix/sysv/linux/closefrom.c b/io/close_range.c
similarity index 59%
rename from sysdeps/unix/sysv/linux/closefrom.c
rename to io/close_range.c
index 372896b775..e9715fc16b 100644
--- a/sysdeps/unix/sysv/linux/closefrom.c
+++ b/io/close_range.c
@@ -1,4 +1,4 @@ 
-/* Close a range of file descriptors.  Linux version.
+/* Close a range of file descriptors.
    Copyright (C) 2021 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -16,21 +16,29 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <stdbool.h>
-#include <stdio.h>
-#include <sys/param.h>
 #include <unistd.h>
+#include <errno.h>
 
-void
-__closefrom (int lowfd)
+/* Close the file descriptors from FIRST up to LAST, inclusive.  */
+int
+__close_range (unsigned int first, unsigned int last,
+	       int flags)
 {
-  int l = MAX (0, lowfd);
+  if (first > last || flags != 0)
+    {
+      __set_errno (EINVAL);
+      return -1;
+    }
 
-  int r = __close_range (l, ~0U, 0);
-  if (r == 0)
-    return;
+  int maxfd = __getdtablesize ();
+  if (maxfd == -1)
+    return -1;
 
-  if (!__closefrom_fallback (l, true))
-    __fortify_fail ("closefrom failed to close a file descriptor");
+  for (int i = 0; i < maxfd; i++)
+    if (i >= lowfd)
+      __close_nocancel_nostatus (i);
 }
-weak_alias (__closefrom, closefrom)
+libc_hidden_def (__close_range)
+weak_alias (__close_range, close_range)
+
+stub_warning (close_range)
diff --git a/io/closefrom.c b/io/closefrom.c
index 01660a7531..e9167687bc 100644
--- a/io/closefrom.c
+++ b/io/closefrom.c
@@ -16,19 +16,21 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#include <stdbool.h>
 #include <stdio.h>
+#include <sys/param.h>
 #include <unistd.h>
-#include <not-cancel.h>
 
 void
 __closefrom (int lowfd)
 {
-  int maxfd = __getdtablesize ();
-  if (maxfd == -1)
-    __fortify_fail ("closefrom failed to get the file descriptor table size");
+  int l = MAX (0, lowfd);
 
-  for (int i = 0; i < maxfd; i++)
-    if (i >= lowfd)
-      __close_nocancel_nostatus (i);
+  int r = __close_range (l, ~0U, 0);
+  if (r == 0)
+    return ;
+
+  if (!__closefrom_fallback (l, true))
+    __fortify_fail ("closefrom failed to close a file descriptor");
 }
 weak_alias (__closefrom, closefrom)
diff --git a/sysdeps/unix/sysv/linux/tst-close_range.c b/io/tst-close_range.c
similarity index 98%
rename from sysdeps/unix/sysv/linux/tst-close_range.c
rename to io/tst-close_range.c
index f5069d1b8a..f05b4ff6ae 100644
--- a/sysdeps/unix/sysv/linux/tst-close_range.c
+++ b/io/tst-close_range.c
@@ -119,6 +119,7 @@  close_range_test (void)
   support_descriptors_free (descrs);
 }
 
+#ifdef __linux__
 _Noreturn static int
 close_range_test_fn (void *arg)
 {
@@ -155,8 +156,10 @@  close_range_test_subprocess (void)
   support_descriptors_check (descrs);
   support_descriptors_free (descrs);
 }
+#endif
 
 
+#ifdef CLOSE_RANGE_UNSHARE
 _Noreturn static int
 close_range_unshare_test_fn (void *arg)
 {
@@ -200,6 +203,7 @@  close_range_unshare_test (void)
   support_descriptors_check (descrs1);
   support_descriptors_free (descrs1);
 }
+#endif
 
 static bool
 is_in_array (int *arr, size_t len, int fd)
@@ -282,8 +286,12 @@  do_test (void)
 {
   close_range_test_max_upper_limit ();
   close_range_test ();
+#ifdef __linux__
   close_range_test_subprocess ();
+#endif
+#ifdef CLOSE_RANGE_UNSHARE
   close_range_unshare_test ();
+#endif
   close_range_cloexec_test ();
 
   return 0;
diff --git a/posix/unistd.h b/posix/unistd.h
index 7a61ff5e86..3cdd0a6e37 100644
--- a/posix/unistd.h
+++ b/posix/unistd.h
@@ -1199,6 +1199,17 @@  int getentropy (void *__buffer, size_t __length) __wur
     __attr_access ((__write_only__, 1, 2));
 #endif
 
+#ifdef __USE_GNU
+/* Close all file descriptors in the range FD up to MAX_FD.  The flag FLAGS
+   are define by the CLOSE_RANGE prefix.  This function behaves like close
+   on the range, but in a fail-safe where it will either fail and not close
+   any file descriptor or close all of them.  Gaps where the file descriptor
+   is invalid are ignored.   Returns 0 on successor or -1 for failure (and
+   sets errno accordingly).  */
+extern int close_range (unsigned int __fd, unsigned int __max_fd,
+			int __flags) __THROW;
+#endif
+
 /* Define some macros helping to catch buffer overflows.  */
 #if __USE_FORTIFY_LEVEL > 0 && defined __fortify_function
 # include <bits/unistd.h>
diff --git a/sysdeps/mach/hurd/Makefile b/sysdeps/mach/hurd/Makefile
index 9acbe80f26..17bb643c18 100644
--- a/sysdeps/mach/hurd/Makefile
+++ b/sysdeps/mach/hurd/Makefile
@@ -196,7 +196,7 @@  sysdep_routines += cthreads
 endif
 
 ifeq (io, $(subdir))
-sysdep_routines += f_setlk close_nocancel close_nocancel_nostatus close_range \
+sysdep_routines += f_setlk close_nocancel close_nocancel_nostatus \
 		   fcntl_nocancel open_nocancel openat_nocancel read_nocancel \
 		   pread64_nocancel write_nocancel pwrite64_nocancel \
 		   wait4_nocancel \
diff --git a/sysdeps/mach/hurd/bits/unistd_ext.h b/sysdeps/mach/hurd/bits/unistd_ext.h
index 288f504a3c..14f85539d5 100644
--- a/sysdeps/mach/hurd/bits/unistd_ext.h
+++ b/sysdeps/mach/hurd/bits/unistd_ext.h
@@ -25,10 +25,4 @@ 
 /* Set the FD_CLOEXEC bit instead of closing the file descriptor.  */
 #define CLOSE_RANGE_CLOEXEC (1U << 2)
 
-/* Close the file descriptors from FIRST up to LAST, inclusive.
-   If CLOSE_RANGE_CLOEXEC is set in FLAGS, set the FD_CLOEXEC flag
-   instead of closing.  */
-extern int close_range (unsigned int __first, unsigned int __last,
-			int __flags) __THROW;
-
 #endif /* __USE_GNU  */
diff --git a/sysdeps/mach/hurd/closefrom.c b/sysdeps/mach/hurd/closefrom.c
deleted file mode 100644
index 5d667cf6c4..0000000000
--- a/sysdeps/mach/hurd/closefrom.c
+++ /dev/null
@@ -1,29 +0,0 @@ 
-/* Close a range of file descriptors.  Hurd version.
-   Copyright (C) 2021 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <unistd.h>
-#include <sys/param.h>
-
-void
-__closefrom (int lowfd)
-{
-  int l = MAX (0, lowfd);
-
-  (void) __close_range (l, ~0U, 0);
-}
-weak_alias (__closefrom, closefrom)
diff --git a/sysdeps/mach/hurd/kernel-features.h b/sysdeps/mach/hurd/kernel-features.h
index 7d4eaee0a6..5fd37a6d7b 100644
--- a/sysdeps/mach/hurd/kernel-features.h
+++ b/sysdeps/mach/hurd/kernel-features.h
@@ -19,3 +19,5 @@ 
 /* This file can define __ASSUME_* macros checked by certain source files.
    Almost none of these are used outside of sysdeps/unix/sysv/linux code.
    But those referring to POSIX-level features like O_* flags can be.  */
+
+#define __ASSUME_CLOSE_RANGE 1
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 76ad06361c..76042a6019 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -120,7 +120,6 @@  tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
 	 tst-timerfd tst-ppoll \
 	 tst-clock_adjtime tst-adjtimex tst-ntp_adjtime tst-ntp_gettime \
 	 tst-ntp_gettimex tst-sigtimedwait tst-misalign-clone \
-  tst-close_range \
   tst-prctl \
   tst-scm_rights \
   # tests
diff --git a/sysdeps/unix/sysv/linux/bits/unistd_ext.h b/sysdeps/unix/sysv/linux/bits/unistd_ext.h
index ae9994403c..8f422e60da 100644
--- a/sysdeps/unix/sysv/linux/bits/unistd_ext.h
+++ b/sysdeps/unix/sysv/linux/bits/unistd_ext.h
@@ -47,13 +47,4 @@  extern __pid_t gettid (void) __THROW;
 # define CLOSE_RANGE_CLOEXEC (1U << 2)
 #endif
 
-/* Close all file descriptors in the range FD up to MAX_FD.  The flag FLAGS
-   are define by the CLOSE_RANGE prefix.  This function behaves like close
-   on the range, but in a fail-safe where it will either fail and not close
-   any file descriptor or close all of them.  Gaps where the file descriptor
-   is invalid are ignored.   Returns 0 on successor or -1 for failure (and
-   sets errno accordingly).  */
-extern int close_range (unsigned int __fd, unsigned int __max_fd,
-			int __flags) __THROW;
-
 #endif /* __USE_GNU  */
diff --git a/sysdeps/unix/sysv/linux/closefrom_fallback.c b/sysdeps/unix/sysv/linux/closefrom_fallback.c
index f215fd2c09..a377ebc544 100644
--- a/sysdeps/unix/sysv/linux/closefrom_fallback.c
+++ b/sysdeps/unix/sysv/linux/closefrom_fallback.c
@@ -21,6 +21,8 @@ 
 #include <not-cancel.h>
 #include <stdbool.h>
 
+#if !__ASSUME_CLOSE_RANGE
+
 /* Fallback code: iterates over /proc/self/fd, closing each file descriptor
    that fall on the criteria.  If DIRFD_FALLBACK is set, a failure on
    /proc/self/fd open will trigger a fallback that tries to close a file
@@ -97,3 +99,5 @@  err:
   __close_nocancel (dirfd);
   return ret;
 }
+
+#endif
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index ffb6af196b..a1108d434f 100644
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -220,6 +220,14 @@ 
 # define __ASSUME_FACCESSAT2 0
 #endif
 
+/* The close_range system call was introduced across all architectures
+   in Linux 5.8.  */
+#if __LINUX_KERNEL_VERSION >= 0x050900
+# define __ASSUME_CLOSE_RANGE 1
+#else
+# define __ASSUME_CLOSE_RANGE 0
+#endif
+
 /* The FUTEX_LOCK_PI2 operation was introduced across all architectures in Linux
    5.14.  */
 #if __LINUX_KERNEL_VERSION >= 0x050e00
diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
index 29899eb264..c38dbb34a1 100644
--- a/sysdeps/unix/sysv/linux/syscalls.list
+++ b/sysdeps/unix/sysv/linux/syscalls.list
@@ -99,4 +99,4 @@  pkey_alloc	EXTRA	pkey_alloc	i:ii	pkey_alloc
 pkey_free	EXTRA	pkey_free	i:i	pkey_free
 gettid          EXTRA   gettid          Ei:     __gettid	gettid
 tgkill          EXTRA   tgkill          i:iii   __tgkill	tgkill
-close_range     EXTRA   close_range     i:iii   __close_range   close_range
+close_range     -       close_range     i:iii   __close_range   close_range