Re: [musl] [PATCH v2] add close_range() syscall wrapper

[Alexey Izbyshev <izbyshev@ispras.ru>]

On 2024-08-01 19:25, Rich Felker wrote:
> On Thu, Aug 01, 2024 at 12:43:00PM +0300, Alexey Izbyshev wrote:
>> On 2023-09-01 17:58, Natanael Copa wrote:
>> >close_range() is a syscall present in FreeBSD 8.0 and Linux 5.9. glibc
>> >2.34 added a wrapper.
>> >
>> >Expose it under _GNU_SOURCE similar to what GNU libc does. Also expose
>> >it under _BSD_SOURCE since it is also a FreeBSD function.
>> >---
>> >
>> >v2: use syscall without __syscall_ret
>> >
>> > include/unistd.h        | 3 +++
>> > src/linux/close_range.c | 8 ++++++++
>> > 2 files changed, 11 insertions(+)
>> > create mode 100644 src/linux/close_range.c
>> >
>> >diff --git a/include/unistd.h b/include/unistd.h
>> >index 5bc7f798..d89e3d4c 100644
>> >--- a/include/unistd.h
>> >+++ b/include/unistd.h
>> >@@ -161,6 +161,9 @@ unsigned ualarm(unsigned, unsigned);
>> > #define L_INCR 1
>> > #define L_XTND 2
>> > int brk(void *);
>> >+#define CLOSE_RANGE_UNSHARE	(1U << 1)
>> >+#define CLOSE_RANGE_CLOEXEC	(1U << 2)
>> >+int close_range(unsigned int, unsigned int, int);
>> > void *sbrk(intptr_t);
>> > pid_t vfork(void);
>> > int vhangup(void);
>> >diff --git a/src/linux/close_range.c b/src/linux/close_range.c
>> >new file mode 100644
>> >index 00000000..3f1378a0
>> >--- /dev/null
>> >+++ b/src/linux/close_range.c
>> >@@ -0,0 +1,8 @@
>> >+#define _GNU_SOURCE
>> >+#include <unistd.h>
>> >+#include "syscall.h"
>> >+
>> >+int close_range(unsigned int first, unsigned int last, int flags)
>> >+{
>> >+	return syscall(SYS_close_range, first, last, flags);
>> >+}
>> 
>> Regarding FreeBSD, close_range was added not in 8.0, but in 13.0
>> [1], and also backported to 12.2 [2].
>> 
>> Otherwise, this patch looks good to me.
>> 
>> Rich, is it possible to consider close_range wrapper inclusion
>> again? Apart from FreeBSD and glibc, bionic has it too. A cursory
>> Debian code search shows that close_range libc wrapper can be used
>> at least by openssh, libvirt, network-manager, openrc, qemu, lxc,
>> rsyslog packages (in addition to CPython that I mentioned ealier).
>> 
>> As for having a fallback in case the syscall is unavailable, I'm not
>> aware of anybody implementing it, so I'd expect all close_range
>> users to implement their own fallback/error handling. For example,
>> Debian's openssh migrated from closefrom to close_range with their
>> own fallback because of too aggressive closefrom fallback in
>> glibc[3].
>> 
>> Thanks,
>> Alexey
>> 
>> [1] 
>> https://cgit.freebsd.org/src/commit/?h=releng/13.0&id=472ced39efb537374068f06b348fe5dac389c45a
>> [2] 
>> https://cgit.freebsd.org/src/commit/?h=releng/12.2&id=a80adba5ab46ba6d44d5abfc9b7f3b6de8afda55
>> [3] 
>> https://sources.debian.org/src/openssh/1%3A9.8p1-1/debian/changelog/#L895
> 
> Thanks for looking into this.
> 
> Generally, I try to follow a principle that if an interface is
> genuinely new functionality, managing some new kind of kernel object
> or something previously not in the data model, that it's fine not to
> have fallback, but that if it's just a new way to act on existing
> things (like adding a missing flags argument to an existing
> operation), there should be fallback at least in cases where no new
> underlying functionality is needed (like in that example, if flags
> value is 0).
> 
> However, close_range really isn't an improved/generalized way to do
> close, but something intended for its own purposes, and if
> applications which use it are prepared for it to fail with ENOSYS
> (this is important! not just prepared for it to be missing at
> configure-time link check) then omitting fallback and letting them do
> their own fallbacks seems like it'd be okay.

I've checked 8 projects mentioned above, and all of them are prepared to 
handle close_range errors. Usually any error leads to a fallback, though 
there are cases where certain errors are treated differently, e.g. in 
network-manager[1], lxc[2].

The glibc manual explicitly warns that the caller has to be prepared for 
ENOSYS[3], although the Linux man-pages project doesn't mention that[4].

I've also discovered that half of those projects (libvirt, 
network-manager, openrc, lxc) can use close_range via syscall in absence 
of the wrapper, and libvirt isn't even aware of the wrapper (i.e. it 
uses syscall() only).
> 
> Unlike closefrom, whose *only* use is invoking UB by closing fds you
> don't own, close_range at least admits well-defined uses where you've
> tracked ranges you do own and want to close them quickly. That's not
> to say folks will use it this way, but having at least some valid use
> is something going for it.
> 
> Without a fallback, I'm not sure there's a lot of value to providing a
> wrapper rather than just having applications use it via
> syscall(SYS_close_range, ...), but there's also not any significant
> cost to having the wrapper if that's what programs expect.
> 
Indeed, the value of the wrapper can't be called huge, but it's still a 
convenience:

* There is no danger of invoking the usual UB by passing int variadic 
arguments to libc's syscall implementation that expects long (even 
though I'm not aware of an arch where this matters in practice).

* FreeBSD had the wrapper since day 1, so for projects that support both 
FreeBSD and Linux (and don't need close_range so desperately that they 
can't wait for the wrapper in libc) it'd be nicer to just use the 
wrapper in both cases.

> Are there any other things we should weigh on this topic?
> 
One thing that comes to mind is that CLOSE_RANGE_UNSHARE flag is an 
extra way to create an execution environment that is unexpected by musl. 
However, given that musl provides the wrapper for unshare, I don't 
believe that  breaking compatibility with glibc and bionic by rejecting 
some close_range flags (like in clone) is worth it.

I also have to add that I'm somewhat biased in the favor of the wrapper 
because it was me who enabled its usage in CPython's subprocess on Linux 
(based on pre-existing work for FreeBSD), but this code doesn't work on 
musl yet.

Thanks,
Alexey

[1] 
https://sources.debian.org/src/network-manager/1.48.6-1/src/libnm-systemd-shared/src/basic/fd-util.c/?hl=314#L314
[2] 
https://sources.debian.org/src/lxc/1:6.0.1-1/src/lxc/initutils.c/?hl=612#L612
[3] 
https://www.gnu.org/software/libc/manual/html_node/Opening-and-Closing-Files.html#index-close_005frange
[4] https://man7.org/linux/man-pages/man2/close_range.2.html