Re: [PATCH 0/4] uapi, vfs: Change the mount API UAPI [ver #2]

On May 16, 2019 6:50:22 PM GMT+02:00, Al Viro [off-list ref] wrote:

[linux-abi cc'd]

On Thu, May 16, 2019 at 06:31:52PM +0200, Christian Brauner wrote:

quoted

On Thu, May 16, 2019 at 05:22:59PM +0100, Al Viro wrote:

quoted

On Thu, May 16, 2019 at 12:52:04PM +0100, David Howells wrote:

quoted

Hi Linus, Al,

Here are some patches that make changes to the mount API UAPI and

two of

quoted

quoted

quoted

them really need applying, before -rc1 - if they're going to be

applied at

quoted

quoted

quoted

all.

I'm fine with 2--4, but I'm not convinced that cloexec-by-default

crusade

quoted

quoted

makes any sense.  Could somebody give coherent arguments in favour

of

quoted

quoted

abandoning the existing conventions?

So as I said in the commit message. From a userspace perspective it's
more of an issue if one accidently leaks an fd to a task during exec.

Also, most of the time one does not want to inherit an fd during an
exec. It is a hazzle to always have to specify an extra flag.

As Al pointed out to me open() semantics are not going anywhere.

Sure,

quoted

no argument there at all.
But the idea of making fds cloexec by default is only targeted at fds
that come from separate syscalls. fsopen(), open_tree_clone(), etc.

they

quoted

all return fds independent of open() so it's really easy to have them
cloexec by default without regressing anyone and we also remove the

need

quoted

for a bunch of separate flags for each syscall to turn them into
cloexec-fds. I mean, those for syscalls came with 4 separate flags to

be

quoted

able to specify that the returned fd should be made cloexec. The

other

quoted

way around, cloexec by default, fcntl() to remove the cloexec bit is

way

quoted

saner imho.

Re separate flags - it is, in principle, a valid argument.  OTOH, I'm
not
sure if they need to be separate - they all have the same value and
I don't see any reason for that to change...

One last thing I'd like to point out is that
we already have syscalls and ioctls that
return cloexec fds. So the consistency
argument is kinda dead.

If you still prefer to have cloexec flags
for the 4 new syscalls then yes,
if they could at least all have the same name
(FSMOUNT_CLOEXEC?) that would be good.

Only tangentially related, but I wonder if something like
close_range(from, to)
would be a more useful approach...  That kind of open-coded loops is
not
rare in userland and kernel-side code can do them much cheaper. 
Something
like
/* that exec is sensitive */
unshare(CLONE_FILES);
/* we don't want anything past stderr here */
close_range(3, ~0U);
execve(....);
on the userland side of thing.  Comments?

Said it before but, the list was mistyped so again:
I think that's a great idea.
I have a prototype for close_range(start, end, flags).
I'll wait after rc1 and then send it out.

Christian

Christian Brauner [off-list ref] wrote:

If you still prefer to have cloexec flags
for the 4 new syscalls then yes,
if they could at least all have the same name
(FSMOUNT_CLOEXEC?) that would be good.

They don't all have the same value (see OPEN_TREE_CLOEXEC).

Note that I also don't want to blindly #define them to O_CLOEXEC because it's
not necessarily the same value on all arches.  Currently it can be 02000000,
010000000 or 0x400000 for instance, which means that if it's sharing a mask
with other flags, at least three bits have to be reserved for it or we have to
have arch-dependent bit juggling.

One thing I like about your approach of just making them O_CLOEXEC by default
and removing the constants is that it avoids this mess entirely.

David

On Fri, May 17, 2019 at 9:13 AM David Howells [off-list ref] wrote:

Christian Brauner [off-list ref] wrote:

quoted

If you still prefer to have cloexec flags
for the 4 new syscalls then yes,
if they could at least all have the same name
(FSMOUNT_CLOEXEC?) that would be good.

They don't all have the same value (see OPEN_TREE_CLOEXEC).

Note that I also don't want to blindly #define them to O_CLOEXEC because it's
not necessarily the same value on all arches.  Currently it can be 02000000,
010000000 or 0x400000 for instance, which means that if it's sharing a mask
with other flags, at least three bits have to be reserved for it or we have to
have arch-dependent bit juggling.

One thing I like about your approach of just making them O_CLOEXEC by default
and removing the constants is that it avoids this mess entirely.

+1.

Confusion caused by inconsistency of naming is going to hurt more than
inconsistency of semantics wrt. open(2).

Thanks,
Miklos

On May 17, 2019 9:13:26 AM GMT+02:00, David Howells [off-list ref] wrote:

Christian Brauner [off-list ref] wrote:

quoted

If you still prefer to have cloexec flags
for the 4 new syscalls then yes,
if they could at least all have the same name
(FSMOUNT_CLOEXEC?) that would be good.

They don't all have the same value (see OPEN_TREE_CLOEXEC).

Note that I also don't want to blindly #define them to O_CLOEXEC
because it's
not necessarily the same value on all arches.  Currently it can be
02000000,
010000000 or 0x400000 for instance, which means that if it's sharing a
mask
with other flags, at least three bits have to be reserved for it or we
have to
have arch-dependent bit juggling.

Ugh. Right, I forgot about that entirely.

Christian

One thing I like about your approach of just making them O_CLOEXEC by
default
and removing the constants is that it avoids this mess entirely.

David