Polish formerly known as Bottom Of #11731, cloexec everywhere #11866

nabijaczleweli · 2021-04-08T22:04:54Z

Motivation and Context

See individual commit messages.

Description

All but the last one came from the bottom of #11731 and should be relatively uncontroversial.

Additionally, this makes all of lib/ cloexec-clean (save for the files from pidfile_open(3), the manpage says nothing about those), I'm pretty sure.

I also found uu_open_tmp() in uu_open.c to be quite a sad sight – there are no users here, nor did I find any in DCS (just openzfs and zfs-fuse), going out to google led me to an Apple import of the same file from 2007, with the same format string Problem, and a MidnightBSD one from 2008, likewise. Do you have a policy of purging this sort of thing that, as far as I can tell, hasn't been used or touched in the past, like, 10 years at least, if not more?

How Has This Been Tested?

Ran it, mostly. 99% of the cloexecs are no-brainers, since the fd is closed like 50 lines down anyway. The one in libzfs_run_process_impl() is used by zpool -c, and that still works.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv) – technically this does break the libzfs and libzfs_core ABI, since they no longer leak their persistent fds, but, y'know
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes. – none apply
I have run the ZFS Test Suite with this change applied. – CI take my hand
All commit messages are properly formatted and contain Signed-off-by.

nabijaczleweli · 2021-04-09T02:35:06Z

So I looked at illumos-gate, bisected all the way back to the root 2005 "OpenSolaris Launch" commit, and at current HEAD there's three hits for uu_open_tmp – the header, implementation, and API listing, launch sees those at the maximum with five – the header, implementation, a different API listing, and ABI listings for i386 and SPARC.

Similarly:

nabijaczleweli@tarta:~/uwu/illumos-gate$ git log usr/src/lib/libuutil/common/uu_open.c
commit 7c478bd95313f5f23a4c958a745db2134aa03244
Author: stevel@tonic-gate <none@none>
Date:   Tue Jun 14 00:00:00 2005 -0700

    OpenSolaris Launch

it's rather safe to say that nobody's used this function in over sixteen years. I don't have Solaris sources at hand (does anyone?) but I expect that to go much deeper.

behlendorf

Looks good. I think the only thing mildly controversial here is setting O_CLOEXEC everywhere, but considering we do fork/exec in a few places adding it does make good sense. I'd just suggest squashing all of those changes in to a single commit.

As for uu_open_tmp() I don't think there's a good reason to not just drop it. The only reason it was pulled in was probably for the sake of completeness at the time. It would technically be an ABI change but since it's almost certainly never been used by anything I don't think that's a real issue.

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

This changes the password prompt for new encryption roots from Enter passphrase: Re-enter passphrase: to Enter new passphrase: Re-enter new passphrase: which makes more sense and is more consistent with "new passphrase" now always meaning "come up with something" and plain "passphrase" "remember that thing" Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

These were fd 3, 4, and 5 by the time zfs change-key hit execute_key_fob() glibc appends "e" to setmntent() mode, but musl's just returns fopen() Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

As found by git grep -E '(open|setmntent|pipe2?)\(' | grep -vE '((zfs|zpool)_|fd|dl|lzc_re|pidfile_|g_)open\(' FreeBSD's pidfile_open() says nothing about the flags of the files it opens, but we can't do anything about it anyway; the implementation does open all files with O_CLOEXEC Consider this output with zpool.d/media appended with "pid=$$; (ls -l /proc/$pid/fd > /dev/tty)": $ /sbin/zpool iostat -vc media lrwx------ 0 -> /dev/pts/0 l-wx------ 1 -> 'pipe:[3278500]' l-wx------ 2 -> /dev/null lrwx------ 3 -> /dev/zfs lr-x------ 4 -> /proc/31895/mounts lrwx------ 5 -> /dev/zfs lr-x------ 10 -> /usr/lib/zfs-linux/zpool.d/media vs $ ./zpool iostat -vc vendor,upath,iostat,media lrwx------ 0 -> /dev/pts/0 l-wx------ 1 -> 'pipe:[3279887]' l-wx------ 2 -> /dev/null lr-x------ 10 -> /usr/lib/zfs-linux/zpool.d/media Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

nabijaczleweli · 2021-04-09T12:27:10Z

Squashed. Personally, I'd say that not opening random library fds with O_CLOEXEC is more controversial from an absolute stand-point, but.

As far as data goes, I've appended pid=$$; (ls -l /proc/$pid/fd > /dev/tty) to zpool.d/media; compare:

nabijaczleweli@szarotka:~/code/zfs/cmd/zpool$ /sbin/zpool iostat -vc media
total 0
lrwx------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:22 0 -> /dev/pts/0
l-wx------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:22 1 -> 'pipe:[3278500]'
lr-x------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:22 10 -> /usr/lib/zfs-linux/zpool.d/media
l-wx------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:22 2 -> /dev/null
lrwx------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:22 3 -> /dev/zfs
lr-x------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:22 4 -> /proc/31895/mounts
lrwx------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:22 5 -> /dev/zfs
                        capacity     operations     bandwidth
pool                  alloc   free   read  write   read  write  media
--------------------  -----  -----  -----  -----  -----  -----  -----
zest                  39.7G  4.77G      0      0      8     79
  zest-rebalanced-ng  39.7G  4.77G      0      0      8     79      -
--------------------  -----  -----  -----  -----  -----  -----  -----

with

nabijaczleweli@szarotka:~/code/zfs/cmd/zpool$ ./zpool iostat -vc media
total 0
lrwx------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:23 0 -> /dev/pts/0
l-wx------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:23 1 -> 'pipe:[3279887]'
lr-x------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:23 10 -> /usr/lib/zfs-linux/zpool.d/media
l-wx------ 1 nabijaczleweli nabijaczleweli 64 Apr  9 14:23 2 -> /dev/null
                        capacity     operations     bandwidth
pool                  alloc   free   read  write   read  write  media
--------------------  -----  -----  -----  -----  -----  -----  -----
zest                  39.7G  4.77G      0      0      8     79
  zest-rebalanced-ng  39.7G  4.77G      0      0      8     79      -
--------------------  -----  -----  -----  -----  -----  -----  -----

In that case, I'll prepare a PR purging uu_open_tmp()

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11866

This changes the password prompt for new encryption roots from Enter passphrase: Re-enter passphrase: to Enter new passphrase: Re-enter new passphrase: which makes more sense and is more consistent with "new passphrase" now always meaning "come up with something" and plain "passphrase" "remember that thing" Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11866

These were fd 3, 4, and 5 by the time zfs change-key hit execute_key_fob() glibc appends "e" to setmntent() mode, but musl's just returns fopen() Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11866

As found by git grep -E '(open|setmntent|pipe2?)\(' | grep -vE '((zfs|zpool)_|fd|dl|lzc_re|pidfile_|g_)open\(' FreeBSD's pidfile_open() says nothing about the flags of the files it opens, but we can't do anything about it anyway; the implementation does open all files with O_CLOEXEC Consider this output with zpool.d/media appended with "pid=$$; (ls -l /proc/$pid/fd > /dev/tty)": $ /sbin/zpool iostat -vc media lrwx------ 0 -> /dev/pts/0 l-wx------ 1 -> 'pipe:[3278500]' l-wx------ 2 -> /dev/null lrwx------ 3 -> /dev/zfs lr-x------ 4 -> /proc/31895/mounts lrwx------ 5 -> /dev/zfs lr-x------ 10 -> /usr/lib/zfs-linux/zpool.d/media vs $ ./zpool iostat -vc vendor,upath,iostat,media lrwx------ 0 -> /dev/pts/0 l-wx------ 1 -> 'pipe:[3279887]' l-wx------ 2 -> /dev/null lr-x------ 10 -> /usr/lib/zfs-linux/zpool.d/media Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11866

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11866

This changes the password prompt for new encryption roots from Enter passphrase: Re-enter passphrase: to Enter new passphrase: Re-enter new passphrase: which makes more sense and is more consistent with "new passphrase" now always meaning "come up with something" and plain "passphrase" "remember that thing" Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11866