From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ua1-x934.google.com (mail-ua1-x934.google.com [IPv6:2607:f8b0:4864:20::934]) by sourceware.org (Postfix) with ESMTPS id D71DB3858D28 for ; Tue, 23 Nov 2021 18:29:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D71DB3858D28 Received: by mail-ua1-x934.google.com with SMTP id y5so45644302ual.7 for ; Tue, 23 Nov 2021 10:29:24 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:from:to:cc:references:in-reply-to :content-transfer-encoding; bh=9QoY0FYpmXPc7FslqkuAz1rkUssWFPfX4HpE2rVvIJg=; b=KpUajeeRMQqxnixyZ/irptKm7xJBNWRUPafIGaonrYZ1LF2o++fXy9/nmjIHgKC+VW anjW3Cej1edB2P6qOsAEYA6tEoIV+bkIOlXjUW1sXL2DY9KFel92aVYcx+V3/SxCTXPT CNzgCjk3q0/8KGaPSBVAwfdl9Ho9mMsUlAsSWW6iknLMFQyMvh8kNJc+5K+8teXWwcMP 5NEBfH6c+ui0RaEdM6beIzgK1hp4KDEvR6huqunfoJAiKefVV4q5cbK2RNphpdkHKwKa cNsI00llE+Yu1p3OEwBGEFtESSElXYW5UDirzYKFEO72vaRCHr0NWzCvOSF53/ZYvY6u AlXw== X-Gm-Message-State: AOAM530iyRf9qKp4X6ifVVO+nJXiOwRzR4YBgU5K7kVlkBM9mRvHJeuZ WvrArcw1exBXeUDAXzFJi/kxbSLWsLcKPg== X-Google-Smtp-Source: ABdhPJz/l0X3EE2svj7jCg9uiPLoI4W9XngIx4eexl1dRDNQ6mXbjH0KbWuD6R6MTZIZqo/6OP0l6Q== X-Received: by 2002:a67:c511:: with SMTP id e17mr12215720vsk.40.1637692164032; Tue, 23 Nov 2021 10:29:24 -0800 (PST) Received: from ?IPV6:2804:431:c7cb:e054:570f:87aa:2a8d:f2bc? ([2804:431:c7cb:e054:570f:87aa:2a8d:f2bc]) by smtp.gmail.com with ESMTPSA id n3sm6678764vkq.6.2021.11.23.10.29.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 23 Nov 2021 10:29:23 -0800 (PST) Message-ID: <958b77d7-473f-ec94-360c-2d8fd71848bd@linaro.org> Date: Tue, 23 Nov 2021 15:29:21 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.1 Subject: Re: [PATCH v2] io: Refactor close_range and closefrom Content-Language: en-US From: Adhemerval Zanella To: libc-alpha@sourceware.org, Samuel Thibault Cc: Sergey Bugaev References: <20211108172817.2235239-1-adhemerval.zanella@linaro.org> In-Reply-To: <20211108172817.2235239-1-adhemerval.zanella@linaro.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Nov 2021 18:29:27 -0000 If no one oposes it I will commit this shortly. On 08/11/2021 14:28, Adhemerval Zanella wrote: > 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. > --- > Changes from v1: > - Changed close_range() comment to specify close() errors are > ignored. > - Fixed close_range() default implementation. > - __closefrom_fallback() for __ASSUME_CLOSE_RANGE. > --- > 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 | 10 ++++++ > 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, 76 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 > > diff --git a/include/unistd.h b/include/unistd.h > index 7849562c42..927d249380 100644 > --- a/include/unistd.h > +++ b/include/unistd.h > @@ -3,6 +3,9 @@ > > # ifndef _ISOMAC > > +# include > +# include > + > 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 false; > +} > +#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..26a615d8b9 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 > . */ > > -#include > -#include > -#include > +#include > +#include > #include > > -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 = first; i <= last && i < maxfd; i++) > + __close_nocancel_nostatus (i); > + > + return 0; > } > -weak_alias (__closefrom, closefrom) > +libc_hidden_def (__close_range) > +weak_alias (__close_range, 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 > . */ > > +#include > #include > +#include > #include > -#include > > 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..3c8a7ced6a 100644 > --- a/posix/unistd.h > +++ b/posix/unistd.h > @@ -1199,6 +1199,16 @@ 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 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). */ > +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 > 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 > - . */ > - > -#include > -#include > - > -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 > #include > > +#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 >