kern_descrip.c: provide helpers to translate between fd flags namespace

[markj]

sys/kern/kern_descrip.c

488

Extra newline.

559

1520

For my taste, abbreviating close_range to clorange isn't really useful: it's only 3 chars, and all of the related functions (close_range_flags(), close_range_impl()) use close_range_ as a prefix.

[kib]

Review comments:
clorange->close_range
assert that sticky_orb is right

[kib]

sys/kern/kern_descrip.c

635

So might be lets discuss it there:
is it required behavior to have O_CLOFORK sticky on F_SETFD?
Are there tests that trigger it? I know that it is not documented.

[markj]

sys/kern/kern_descrip.c

635

Thinking again, I suspect it should be non-sticky. POSIX requires this, I was looking at an old version earlier and didn't see any mention, but it is pretty clear: https://pubs.opengroup.org/onlinepubs/9799919799/

Most code which uses F_SETFD is ok since it sets FD_CLOEXEC immediately after the descriptor is opened, i.e., the code predates O_CLOEXEC, or it is trying to be portable. In this case there is no significant risk of accidentally clearing FD_CLOFORK.

I do not understand why it is necessary for fdopendir() to atomically set FD_CLOEXEC, as you said on github. When fdopendir() is used, the fd is effectively owned by libc, the application should not be modifying it concurrently.

[kib]

sys/kern/kern_descrip.c

635

I am somewhat confused by the first paragraph of your response, it is not clear to me what is your position: are you pro or contra making O_CLOFORK non-sticky? The pointer to the POSIX is to the title page.

I think that any code that modifies flags in mt env should be mt-safe, even if the fd is used internally by libc and not yet returned as an object from internal libc function. Right after finstall(9) the fd is visible to other threads by other means, like enumeration of fds using fdescfs, close_from() etc. This is legal and more, other thread cannot know if the specific fd is internal or not.

[markj]

sys/kern/kern_descrip.c

635

I am pro making O_CLOFORK non-sticky. That is, fcntl(fd, F_SETFD, FD_CLOEXEC) should clear FD_CLOFORK.

I think that any code that modifies flags in mt env should be mt-safe

What exactly do you mean by mt-safe? The actual update to fde_flags is mt-safe of course. Are you saying that the non-atomic update might trigger a kernel bug?

other thread cannot know if the specific fd is internal or not

I don't understand this viewpoint. It must know, otherwise the application will simply be broken. What prevents another thread from simply closing an fd while another one is using it?

[kib]

sys/kern/kern_descrip.c

635

I am pro making O_CLOFORK non-sticky. That is, fcntl(fd, F_SETFD, FD_CLOEXEC) should clear FD_CLOFORK.

I will post the patch shortly.

I think that any code that modifies flags in mt env should be mt-safe

What exactly do you mean by mt-safe? The actual update to fde_flags is mt-safe of course. Are you saying that the non-atomic update might trigger a kernel bug?

No, I am saying that apps forced to do something like this now, in absence of the proper OR/NAND fcntl ops:

flags = fcntl(fd, F_GETFD, 0);
flags |= FD_RESOLVE_BENEATH;
fcntl(fd, F_SETFD, flags);

and fd looses flags if two threads try to do something like this in parallel.

other thread cannot know if the specific fd is internal or not

I don't understand this viewpoint. It must know, otherwise the application will simply be broken. What prevents another thread from simply closing an fd while another one is using it?

Not completely unrealistic example: App might do something in preparation for fork(), and there it walks over all fds setting some flags.

[markj]

BTW there are some test failures when this change is applied. e.g.,

lib/libc/gen/popen_test:popen_all_modes_test  ->  failed: 3 checks failed; see output for more details  [0.051s]
lib/libc/gen/popen_test:popen_rmodes_test  ->  failed: 4 checks failed; see output for more details  [0.069s]
lib/libc/gen/popen_test:popen_rwmodes_test  ->  failed: 1 checks failed; see output for more details  [0.026s]
lib/libc/gen/popen_test:popen_wmodes_test  ->  failed: 4 checks failed; see output for more details  [0.318s]

I did not yet spot the bug.

[markj]

sys/kern/kern_descrip.c

635

No, I am saying that apps forced to do something like this now, in absence of the proper OR/NAND fcntl ops:

flags = fcntl(fd, F_GETFD, 0);
flags |= FD_RESOLVE_BENEATH;
fcntl(fd, F_SETFD, flags);

and fd looses flags if two threads try to do something like this in parallel.

Well, this is morally the same as lseek(fd, mumble, SEEK_SET); read(fd);`, except with the file offset instead of flags. Yes, if some thread goes through all open fds and sets a flag, then it might be clobbered. But there is closefrom() to help with this (though there is no CLOSE_RANGE_RESOLVE_BENEATH).

other thread cannot know if the specific fd is internal or not

I don't understand this viewpoint. It must know, otherwise the application will simply be broken. What prevents another thread from simply closing an fd while another one is using it?

Not completely unrealistic example: App might do something in preparation for fork(), and there it walks over all fds setting some flags.

But this is racy anyway, suppose some thread opens an fd after flags are set but before the fork(). Then the flag will not be set on that new fd.

[kib]

sys/kern/kern_descrip.c

635

No, I am saying that apps forced to do something like this now, in absence of the proper OR/NAND fcntl ops:

flags = fcntl(fd, F_GETFD, 0);
flags |= FD_RESOLVE_BENEATH;
fcntl(fd, F_SETFD, flags);

and fd looses flags if two threads try to do something like this in parallel.

Well, this is morally the same as lseek(fd, mumble, SEEK_SET); read(fd);`, except with the file offset instead of flags. Yes, if some thread goes through all open fds and sets a flag, then it might be clobbered. But there is closefrom() to help with this (though there is no CLOSE_RANGE_RESOLVE_BENEATH).

Also it only sets the flags, it cannot clear them.

other thread cannot know if the specific fd is internal or not

I don't understand this viewpoint. It must know, otherwise the application will simply be broken. What prevents another thread from simply closing an fd while another one is using it?

Not completely unrealistic example: App might do something in preparation for fork(), and there it walks over all fds setting some flags.

But this is racy anyway, suppose some thread opens an fd after flags are set but before the fork(). Then the flag will not be set on that new fd.

This is a different race.

Anyway, lets start with making the flag non-sticky.

636

This should fix the tests.