Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Miklos Szeredi]

On Mon, Jun 4, 2018 at 7:35 PM, Al Viro  wrote:
> On Mon, Jun 04, 2018 at 10:16:30AM -0700, Matthew Wilcox wrote:
>> On Sun, Jun 03, 2018 at 01:55:37AM +0100, Al Viro wrote:
>> > +SYSCALL_DEFINE3(open_tree, int, dfd, const char *, filename, unsigned, 
>> > flags)
>> > +{
>> > +   struct file *file;
>> > +   struct path path;
>> > +   int lookup_flags = LOOKUP_AUTOMOUNT | LOOKUP_FOLLOW;
>> > +   bool detached = flags & OPEN_TREE_CLONE;
>> > +   int error;
>> > +   int fd;
>> > +
>> > +   BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);
>>
>> Why do we need OPEN_TREE_CLOEXEC?  Wouldn't we be better off just making
>> the fd returned by open_tree implicitly close-on-exec?  I can think of
>> no good reason for these file descriptors to be inherited across exec()
>
> How are they different from any file descriptor?  It's not as if it was
> something usable only for mounting stuff - again, you can use them
> with any ...at() syscalls.

Defaulting to close on exec helps keep out clutter from the API.

Is there a disadvantage to needing an explicit fcntl(F_SETFD) call to
disable close on exec?

Thanks,
Miklos

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Miklos Szeredi]

On Mon, Jun 4, 2018 at 7:35 PM, Al Viro  wrote:
> On Mon, Jun 04, 2018 at 10:16:30AM -0700, Matthew Wilcox wrote:
>> On Sun, Jun 03, 2018 at 01:55:37AM +0100, Al Viro wrote:
>> > +SYSCALL_DEFINE3(open_tree, int, dfd, const char *, filename, unsigned, 
>> > flags)
>> > +{
>> > +   struct file *file;
>> > +   struct path path;
>> > +   int lookup_flags = LOOKUP_AUTOMOUNT | LOOKUP_FOLLOW;
>> > +   bool detached = flags & OPEN_TREE_CLONE;
>> > +   int error;
>> > +   int fd;
>> > +
>> > +   BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);
>>
>> Why do we need OPEN_TREE_CLOEXEC?  Wouldn't we be better off just making
>> the fd returned by open_tree implicitly close-on-exec?  I can think of
>> no good reason for these file descriptors to be inherited across exec()
>
> How are they different from any file descriptor?  It's not as if it was
> something usable only for mounting stuff - again, you can use them
> with any ...at() syscalls.

Defaulting to close on exec helps keep out clutter from the API.

Is there a disadvantage to needing an explicit fcntl(F_SETFD) call to
disable close on exec?

Thanks,
Miklos

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Miklos Szeredi]

On Mon, Jun 4, 2018 at 5:52 PM, Al Viro  wrote:
> On Mon, Jun 04, 2018 at 12:34:44PM +0200, Miklos Szeredi wrote:
>
>> fsopen = create fsfd
>> fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd
>> fspick = path -> fsfd
>> move_mount = attach mountfd or move existing
>> fsinfo = info from path
>> open_tree = new mountfd from path or clone
>> mount_setattr = set attr on mountfd
>>
>> Notice that fsmount() encompasses mount_setattr() + move_mount()
>> functionality.   Split those out and leave fsmount() to actually do
>> the "fsfd ->mountfd" translation?
>
> Might make sense.

> FWIW, to make it clear: fsmount(2) in this series actually does *NOT*
> attach it to the tree.

Ah, that leaves the mount_setattr() functionality to split out.   I'd
be more happy to rid this new API of all the old MS_* crap and have
have a new set of attributes, that just apply to mounts.   It will
also need  two args: a bitmap of new attributes and a mask to tell us
which attributes to change.

>  Commit message definitely needs updating - as it
> is, it's
>
> +SYSCALL_DEFINE5(fsmount, int, fs_fd, unsigned int, flags, unsigned int, 
> ms_flags,
> +   void *, reserved4, void *, reserved5)
>
> PS: IMO these reserved... arguments are in bad taste; if anyone has good 
> reasons
> for that practice in ABI design, I'd like to hear those.

Agreed.  A flags argument is often wise to add even if currently
unused (and should be checked for undefined flags), but adding a
random number of pointers doesn't seem to make a lot of sense.

>
>> fsinfo() name suggests it's in the same class as
>> fsopen/fsmount/fspick, operating on fsfd object, but's it's not and I
>> think that's slightly confusing.
>>
>> Rename move_mount() -> mount_move()?
>
> mount_move_bikeshed_bikeshed_bikeshed(), surely?

Consistent naming for related functions... not unheard of in API
design.  The above set definitely does not qualify.

>> Also does it make sense to make the cloning behavior of open_tree()
>> optional?  Without cloning it's just a plain open(O_PATH).  That  way
>> it could be renamed mount_clone().
>
> Umm...  I'm not sure about that one.  If nothing else, OPEN_TREE_DETACH
> might be a good idea, in which case cloning is not the primary effect;
> hell knows.

So conceptually we have the following distinct mount tree operations:

treefd = clone(path);
treefd = detach(path);
attach(treefd, path);
move(path1, path2);

The detach/move/attach trio are more related in functionality, while
clone and detach have the same signature. I'm not sure either.

Thanks,
Miklos

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Miklos Szeredi]

On Mon, Jun 4, 2018 at 5:52 PM, Al Viro  wrote:
> On Mon, Jun 04, 2018 at 12:34:44PM +0200, Miklos Szeredi wrote:
>
>> fsopen = create fsfd
>> fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd
>> fspick = path -> fsfd
>> move_mount = attach mountfd or move existing
>> fsinfo = info from path
>> open_tree = new mountfd from path or clone
>> mount_setattr = set attr on mountfd
>>
>> Notice that fsmount() encompasses mount_setattr() + move_mount()
>> functionality.   Split those out and leave fsmount() to actually do
>> the "fsfd ->mountfd" translation?
>
> Might make sense.

> FWIW, to make it clear: fsmount(2) in this series actually does *NOT*
> attach it to the tree.

Ah, that leaves the mount_setattr() functionality to split out.   I'd
be more happy to rid this new API of all the old MS_* crap and have
have a new set of attributes, that just apply to mounts.   It will
also need  two args: a bitmap of new attributes and a mask to tell us
which attributes to change.

>  Commit message definitely needs updating - as it
> is, it's
>
> +SYSCALL_DEFINE5(fsmount, int, fs_fd, unsigned int, flags, unsigned int, 
> ms_flags,
> +   void *, reserved4, void *, reserved5)
>
> PS: IMO these reserved... arguments are in bad taste; if anyone has good 
> reasons
> for that practice in ABI design, I'd like to hear those.

Agreed.  A flags argument is often wise to add even if currently
unused (and should be checked for undefined flags), but adding a
random number of pointers doesn't seem to make a lot of sense.

>
>> fsinfo() name suggests it's in the same class as
>> fsopen/fsmount/fspick, operating on fsfd object, but's it's not and I
>> think that's slightly confusing.
>>
>> Rename move_mount() -> mount_move()?
>
> mount_move_bikeshed_bikeshed_bikeshed(), surely?

Consistent naming for related functions... not unheard of in API
design.  The above set definitely does not qualify.

>> Also does it make sense to make the cloning behavior of open_tree()
>> optional?  Without cloning it's just a plain open(O_PATH).  That  way
>> it could be renamed mount_clone().
>
> Umm...  I'm not sure about that one.  If nothing else, OPEN_TREE_DETACH
> might be a good idea, in which case cloning is not the primary effect;
> hell knows.

So conceptually we have the following distinct mount tree operations:

treefd = clone(path);
treefd = detach(path);
attach(treefd, path);
move(path1, path2);

The detach/move/attach trio are more related in functionality, while
clone and detach have the same signature. I'm not sure either.

Thanks,
Miklos

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Al Viro]

On Mon, Jun 04, 2018 at 10:16:30AM -0700, Matthew Wilcox wrote:
> On Sun, Jun 03, 2018 at 01:55:37AM +0100, Al Viro wrote:
> > +SYSCALL_DEFINE3(open_tree, int, dfd, const char *, filename, unsigned, 
> > flags)
> > +{
> > +   struct file *file;
> > +   struct path path;
> > +   int lookup_flags = LOOKUP_AUTOMOUNT | LOOKUP_FOLLOW;
> > +   bool detached = flags & OPEN_TREE_CLONE;
> > +   int error;
> > +   int fd;
> > +
> > +   BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);
> 
> Why do we need OPEN_TREE_CLOEXEC?  Wouldn't we be better off just making
> the fd returned by open_tree implicitly close-on-exec?  I can think of
> no good reason for these file descriptors to be inherited across exec()

How are they different from any file descriptor?  It's not as if it was
something usable only for mounting stuff - again, you can use them
with any ...at() syscalls.

> and if someone comes up with such a reason, fcntl(F_SETFD) is not an
> expensive call to make.

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Al Viro]

On Mon, Jun 04, 2018 at 10:16:30AM -0700, Matthew Wilcox wrote:
> On Sun, Jun 03, 2018 at 01:55:37AM +0100, Al Viro wrote:
> > +SYSCALL_DEFINE3(open_tree, int, dfd, const char *, filename, unsigned, 
> > flags)
> > +{
> > +   struct file *file;
> > +   struct path path;
> > +   int lookup_flags = LOOKUP_AUTOMOUNT | LOOKUP_FOLLOW;
> > +   bool detached = flags & OPEN_TREE_CLONE;
> > +   int error;
> > +   int fd;
> > +
> > +   BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);
> 
> Why do we need OPEN_TREE_CLOEXEC?  Wouldn't we be better off just making
> the fd returned by open_tree implicitly close-on-exec?  I can think of
> no good reason for these file descriptors to be inherited across exec()

How are they different from any file descriptor?  It's not as if it was
something usable only for mounting stuff - again, you can use them
with any ...at() syscalls.

> and if someone comes up with such a reason, fcntl(F_SETFD) is not an
> expensive call to make.

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Matthew Wilcox]

On Sun, Jun 03, 2018 at 01:55:37AM +0100, Al Viro wrote:
> +SYSCALL_DEFINE3(open_tree, int, dfd, const char *, filename, unsigned, flags)
> +{
> + struct file *file;
> + struct path path;
> + int lookup_flags = LOOKUP_AUTOMOUNT | LOOKUP_FOLLOW;
> + bool detached = flags & OPEN_TREE_CLONE;
> + int error;
> + int fd;
> +
> + BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);

Why do we need OPEN_TREE_CLOEXEC?  Wouldn't we be better off just making
the fd returned by open_tree implicitly close-on-exec?  I can think of
no good reason for these file descriptors to be inherited across exec()
and if someone comes up with such a reason, fcntl(F_SETFD) is not an
expensive call to make.

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Matthew Wilcox]

On Sun, Jun 03, 2018 at 01:55:37AM +0100, Al Viro wrote:
> +SYSCALL_DEFINE3(open_tree, int, dfd, const char *, filename, unsigned, flags)
> +{
> + struct file *file;
> + struct path path;
> + int lookup_flags = LOOKUP_AUTOMOUNT | LOOKUP_FOLLOW;
> + bool detached = flags & OPEN_TREE_CLONE;
> + int error;
> + int fd;
> +
> + BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);

Why do we need OPEN_TREE_CLOEXEC?  Wouldn't we be better off just making
the fd returned by open_tree implicitly close-on-exec?  I can think of
no good reason for these file descriptors to be inherited across exec()
and if someone comes up with such a reason, fcntl(F_SETFD) is not an
expensive call to make.

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Al Viro]

On Mon, Jun 04, 2018 at 04:52:05PM +0100, Al Viro wrote:
> On Mon, Jun 04, 2018 at 12:34:44PM +0200, Miklos Szeredi wrote:
> 
> > fsopen = create fsfd
> > fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd
> > fspick = path -> fsfd
> > move_mount = attach mountfd or move existing
> > fsinfo = info from path
> > open_tree = new mountfd from path or clone
> > mount_setattr = set attr on mountfd
> > 
> > Notice that fsmount() encompasses mount_setattr() + move_mount()
> > functionality.   Split those out and leave fsmount() to actually do
> > the "fsfd ->mountfd" translation?
> 
> Might make sense.

FWIW, to make it clear: fsmount(2) in this series actually does *NOT*
attach it to the tree.  Commit message definitely needs updating - as it
is, it's

+SYSCALL_DEFINE5(fsmount, int, fs_fd, unsigned int, flags, unsigned int, 
ms_flags,
+   void *, reserved4, void *, reserved5)

PS: IMO these reserved... arguments are in bad taste; if anyone has good reasons
for that practice in ABI design, I'd like to hear those.

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Al Viro]

On Mon, Jun 04, 2018 at 04:52:05PM +0100, Al Viro wrote:
> On Mon, Jun 04, 2018 at 12:34:44PM +0200, Miklos Szeredi wrote:
> 
> > fsopen = create fsfd
> > fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd
> > fspick = path -> fsfd
> > move_mount = attach mountfd or move existing
> > fsinfo = info from path
> > open_tree = new mountfd from path or clone
> > mount_setattr = set attr on mountfd
> > 
> > Notice that fsmount() encompasses mount_setattr() + move_mount()
> > functionality.   Split those out and leave fsmount() to actually do
> > the "fsfd ->mountfd" translation?
> 
> Might make sense.

FWIW, to make it clear: fsmount(2) in this series actually does *NOT*
attach it to the tree.  Commit message definitely needs updating - as it
is, it's

+SYSCALL_DEFINE5(fsmount, int, fs_fd, unsigned int, flags, unsigned int, 
ms_flags,
+   void *, reserved4, void *, reserved5)

PS: IMO these reserved... arguments are in bad taste; if anyone has good reasons
for that practice in ABI design, I'd like to hear those.

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Al Viro]

On Mon, Jun 04, 2018 at 12:34:44PM +0200, Miklos Szeredi wrote:

> fsopen = create fsfd
> fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd
> fspick = path -> fsfd
> move_mount = attach mountfd or move existing
> fsinfo = info from path
> open_tree = new mountfd from path or clone
> mount_setattr = set attr on mountfd
> 
> Notice that fsmount() encompasses mount_setattr() + move_mount()
> functionality.   Split those out and leave fsmount() to actually do
> the "fsfd ->mountfd" translation?

Might make sense.

> fsinfo() name suggests it's in the same class as
> fsopen/fsmount/fspick, operating on fsfd object, but's it's not and I
> think that's slightly confusing.
> 
> Rename move_mount() -> mount_move()?

mount_move_bikeshed_bikeshed_bikeshed(), surely?

> Also does it make sense to make the cloning behavior of open_tree()
> optional?  Without cloning it's just a plain open(O_PATH).  That  way
> it could be renamed mount_clone().

Umm...  I'm not sure about that one.  If nothing else, OPEN_TREE_DETACH
might be a good idea, in which case cloning is not the primary effect;
hell knows.

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Al Viro]

On Mon, Jun 04, 2018 at 12:34:44PM +0200, Miklos Szeredi wrote:

> fsopen = create fsfd
> fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd
> fspick = path -> fsfd
> move_mount = attach mountfd or move existing
> fsinfo = info from path
> open_tree = new mountfd from path or clone
> mount_setattr = set attr on mountfd
> 
> Notice that fsmount() encompasses mount_setattr() + move_mount()
> functionality.   Split those out and leave fsmount() to actually do
> the "fsfd ->mountfd" translation?

Might make sense.

> fsinfo() name suggests it's in the same class as
> fsopen/fsmount/fspick, operating on fsfd object, but's it's not and I
> think that's slightly confusing.
> 
> Rename move_mount() -> mount_move()?

mount_move_bikeshed_bikeshed_bikeshed(), surely?

> Also does it make sense to make the cloning behavior of open_tree()
> optional?  Without cloning it's just a plain open(O_PATH).  That  way
> it could be renamed mount_clone().

Umm...  I'm not sure about that one.  If nothing else, OPEN_TREE_DETACH
might be a good idea, in which case cloning is not the primary effect;
hell knows.

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[David Howells]

Miklos Szeredi  wrote:

> fsinfo = info from path

Actually, I was thinking of making fsinfo() detect if it's been given an fsfd
and go through an fs_context operation instead in that case.

David

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[David Howells]

Miklos Szeredi  wrote:

> fsinfo = info from path

Actually, I was thinking of making fsinfo() detect if it's been given an fsfd
and go through an fs_context operation instead in that case.

David

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Miklos Szeredi]

On Sun, Jun 3, 2018 at 2:55 AM, Al Viro  wrote:
> On Sat, Jun 02, 2018 at 06:49:58PM +0100, Al Viro wrote:
>
>> > > Hell, might even add AT_UMOUNT for "open root and detach, to be 
>> > > dissolved on
>> > > close", incompatible with AT_CLONE.
>> >
>> > Cute.  Guess you could do:
>> >
>> > fd = open_mount(..., OPEN_TREE_DETACH);
>> > mount_setattr(fd, "",
>> >   MOUNT_SETATTR_EMPTY_PATH,
>> >   MOUNT_SETATTR_NOSUID, NULL);
>> > move_mount(fd, "", ...);
>
> Hadn't added that yet, but as for the rest of open_tree() - see
> vfs.git#mount-open_tree.  open() and its flags are not touched at all.
> Other changes compared to the previous:
> * may_mount() is required for OPEN_TREE_CLONE
> * sys_ni.c cruft is dropped - those make no sense until and unless
> those syscalls become conditional.
>
> Appears to work, combined with move_mount() it yields eqiuvalents of
> mount --{move,bind,rbind}.  Combined with mount_setattr(2) (when that
> gets added) we'll get mount -o remount,bind,nodev et.al.

fsopen = create fsfd
fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd
fspick = path -> fsfd
move_mount = attach mountfd or move existing
fsinfo = info from path
open_tree = new mountfd from path or clone
mount_setattr = set attr on mountfd

Notice that fsmount() encompasses mount_setattr() + move_mount()
functionality.   Split those out and leave fsmount() to actually do
the "fsfd ->mountfd" translation?

fsinfo() name suggests it's in the same class as
fsopen/fsmount/fspick, operating on fsfd object, but's it's not and I
think that's slightly confusing.

Rename move_mount() -> mount_move()?

Also does it make sense to make the cloning behavior of open_tree()
optional?  Without cloning it's just a plain open(O_PATH).  That  way
it could be renamed mount_clone().

Thanks,
Miklos

Re: [PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Miklos Szeredi]

On Sun, Jun 3, 2018 at 2:55 AM, Al Viro  wrote:
> On Sat, Jun 02, 2018 at 06:49:58PM +0100, Al Viro wrote:
>
>> > > Hell, might even add AT_UMOUNT for "open root and detach, to be 
>> > > dissolved on
>> > > close", incompatible with AT_CLONE.
>> >
>> > Cute.  Guess you could do:
>> >
>> > fd = open_mount(..., OPEN_TREE_DETACH);
>> > mount_setattr(fd, "",
>> >   MOUNT_SETATTR_EMPTY_PATH,
>> >   MOUNT_SETATTR_NOSUID, NULL);
>> > move_mount(fd, "", ...);
>
> Hadn't added that yet, but as for the rest of open_tree() - see
> vfs.git#mount-open_tree.  open() and its flags are not touched at all.
> Other changes compared to the previous:
> * may_mount() is required for OPEN_TREE_CLONE
> * sys_ni.c cruft is dropped - those make no sense until and unless
> those syscalls become conditional.
>
> Appears to work, combined with move_mount() it yields eqiuvalents of
> mount --{move,bind,rbind}.  Combined with mount_setattr(2) (when that
> gets added) we'll get mount -o remount,bind,nodev et.al.

fsopen = create fsfd
fsmount = fsfd -> mountfd & set attr on mountfd & attach mountfd
fspick = path -> fsfd
move_mount = attach mountfd or move existing
fsinfo = info from path
open_tree = new mountfd from path or clone
mount_setattr = set attr on mountfd

Notice that fsmount() encompasses mount_setattr() + move_mount()
functionality.   Split those out and leave fsmount() to actually do
the "fsfd ->mountfd" translation?

fsinfo() name suggests it's in the same class as
fsopen/fsmount/fspick, operating on fsfd object, but's it's not and I
think that's slightly confusing.

Rename move_mount() -> mount_move()?

Also does it make sense to make the cloning behavior of open_tree()
optional?  Without cloning it's just a plain open(O_PATH).  That  way
it could be renamed mount_clone().

Thanks,
Miklos

[PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Al Viro]

On Sat, Jun 02, 2018 at 06:49:58PM +0100, Al Viro wrote:

> > > Hell, might even add AT_UMOUNT for "open root and detach, to be dissolved 
> > > on
> > > close", incompatible with AT_CLONE.
> > 
> > Cute.  Guess you could do:
> > 
> > fd = open_mount(..., OPEN_TREE_DETACH);
> > mount_setattr(fd, "",
> >   MOUNT_SETATTR_EMPTY_PATH,
> >   MOUNT_SETATTR_NOSUID, NULL);
> > move_mount(fd, "", ...);

Hadn't added that yet, but as for the rest of open_tree() - see
vfs.git#mount-open_tree.  open() and its flags are not touched at all.
Other changes compared to the previous:
* may_mount() is required for OPEN_TREE_CLONE
* sys_ni.c cruft is dropped - those make no sense until and unless
those syscalls become conditional.

Appears to work, combined with move_mount() it yields eqiuvalents of
mount --{move,bind,rbind}.  Combined with mount_setattr(2) (when that
gets added) we'll get mount -o remount,bind,nodev et.al.
(including the currently absent whole-subtree versions) and
mount --make-{r,}{shared,slave,private,unbindable}

It also can be used to get an isolated subtree usable for at()
stuff.

The addition of syscall itself is done by the following and I'd really
like linux-abi folks to comment on that puppy

commit 6cfba4dd99b10278c2156c8d4fced2eddedf167f
Author: Al Viro 
Date:   Sat Jun 2 19:42:22 2018 -0400

new syscall: open_tree(2)

open_tree(dfd, pathname, flags)

Returns an O_PATH-opened file descriptor or an error.
dfd and pathname specify the location to open, in usual
fashion (see e.g. fstatat(2)).  flags should be an OR of
some of the following:
* AT_PATH_EMPTY, AT_NO_AUTOMOUNT, AT_SYMLINK_NOFOLLOW -
same meanings as usual
* OPEN_TREE_CLOEXEC - make the resulting descriptor
close-on-exec
* OPEN_TREE_CLONE or OPEN_TREE_CLONE | AT_RECURSIVE -
instead of opening the location in question, create a detached
mount tree matching the subtree rooted at location specified by
dfd/pathname.  With AT_RECURSIVE the entire subtree is cloned,
without it - only the part within in the mount containing the
location in question.  In other words, the same as mount --rbind
or mount --bind would've taken.  The detached tree will be
dissolved on the final close of obtained file.  Creation of such
detached trees requires the same capabilities as doing mount --bind.

Signed-off-by: Al Viro 

diff --git a/arch/x86/entry/syscalls/syscall_32.tbl 
b/arch/x86/entry/syscalls/syscall_32.tbl
index 14a2f996e543..b2b44ecd2b17 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -397,3 +397,4 @@
 383i386statx   sys_statx   
__ia32_sys_statx
 384i386arch_prctl  sys_arch_prctl  
__ia32_compat_sys_arch_prctl
 385i386io_pgetevents   sys_io_pgetevents   
__ia32_compat_sys_io_pgetevents
+391i386open_tree   sys_open_tree   
__ia32_sys_open_tree
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl 
b/arch/x86/entry/syscalls/syscall_64.tbl
index cd36232ab62f..d6f4949378e7 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -342,6 +342,7 @@
 331common  pkey_free   __x64_sys_pkey_free
 332common  statx   __x64_sys_statx
 333common  io_pgetevents   __x64_sys_io_pgetevents
+339common  open_tree   __x64_sys_open_tree
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/fs/file_table.c b/fs/file_table.c
index 7ec0b3e5f05d..7480271a0d21 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -189,6 +189,7 @@ static void __fput(struct file *file)
struct dentry *dentry = file->f_path.dentry;
struct vfsmount *mnt = file->f_path.mnt;
struct inode *inode = file->f_inode;
+   fmode_t mode = file->f_mode;
 
might_sleep();
 
@@ -209,14 +210,14 @@ static void __fput(struct file *file)
file->f_op->release(inode, file);
security_file_free(file);
if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
-!(file->f_mode & FMODE_PATH))) {
+!(mode & FMODE_PATH))) {
cdev_put(inode->i_cdev);
}
fops_put(file->f_op);
put_pid(file->f_owner.pid);
-   if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+   if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
i_readcount_dec(inode);
-   if (file->f_mode & FMODE_WRITER) {
+   if (mode & FMODE_WRITER) {
put_write_access(inode);
__mnt_drop_write(mnt);
}
@@ -224,6 +225,8 @@ static void __fput(struct file *file)
file->f_path.mnt = NULL;
file->f_inode = NULL;

[PATCH][RFC] open_tree(2) (was Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8])

[Al Viro]

On Sat, Jun 02, 2018 at 06:49:58PM +0100, Al Viro wrote:

> > > Hell, might even add AT_UMOUNT for "open root and detach, to be dissolved 
> > > on
> > > close", incompatible with AT_CLONE.
> > 
> > Cute.  Guess you could do:
> > 
> > fd = open_mount(..., OPEN_TREE_DETACH);
> > mount_setattr(fd, "",
> >   MOUNT_SETATTR_EMPTY_PATH,
> >   MOUNT_SETATTR_NOSUID, NULL);
> > move_mount(fd, "", ...);

Hadn't added that yet, but as for the rest of open_tree() - see
vfs.git#mount-open_tree.  open() and its flags are not touched at all.
Other changes compared to the previous:
* may_mount() is required for OPEN_TREE_CLONE
* sys_ni.c cruft is dropped - those make no sense until and unless
those syscalls become conditional.

Appears to work, combined with move_mount() it yields eqiuvalents of
mount --{move,bind,rbind}.  Combined with mount_setattr(2) (when that
gets added) we'll get mount -o remount,bind,nodev et.al.
(including the currently absent whole-subtree versions) and
mount --make-{r,}{shared,slave,private,unbindable}

It also can be used to get an isolated subtree usable for at()
stuff.

The addition of syscall itself is done by the following and I'd really
like linux-abi folks to comment on that puppy

commit 6cfba4dd99b10278c2156c8d4fced2eddedf167f
Author: Al Viro 
Date:   Sat Jun 2 19:42:22 2018 -0400

new syscall: open_tree(2)

open_tree(dfd, pathname, flags)

Returns an O_PATH-opened file descriptor or an error.
dfd and pathname specify the location to open, in usual
fashion (see e.g. fstatat(2)).  flags should be an OR of
some of the following:
* AT_PATH_EMPTY, AT_NO_AUTOMOUNT, AT_SYMLINK_NOFOLLOW -
same meanings as usual
* OPEN_TREE_CLOEXEC - make the resulting descriptor
close-on-exec
* OPEN_TREE_CLONE or OPEN_TREE_CLONE | AT_RECURSIVE -
instead of opening the location in question, create a detached
mount tree matching the subtree rooted at location specified by
dfd/pathname.  With AT_RECURSIVE the entire subtree is cloned,
without it - only the part within in the mount containing the
location in question.  In other words, the same as mount --rbind
or mount --bind would've taken.  The detached tree will be
dissolved on the final close of obtained file.  Creation of such
detached trees requires the same capabilities as doing mount --bind.

Signed-off-by: Al Viro 

diff --git a/arch/x86/entry/syscalls/syscall_32.tbl 
b/arch/x86/entry/syscalls/syscall_32.tbl
index 14a2f996e543..b2b44ecd2b17 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -397,3 +397,4 @@
 383i386statx   sys_statx   
__ia32_sys_statx
 384i386arch_prctl  sys_arch_prctl  
__ia32_compat_sys_arch_prctl
 385i386io_pgetevents   sys_io_pgetevents   
__ia32_compat_sys_io_pgetevents
+391i386open_tree   sys_open_tree   
__ia32_sys_open_tree
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl 
b/arch/x86/entry/syscalls/syscall_64.tbl
index cd36232ab62f..d6f4949378e7 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -342,6 +342,7 @@
 331common  pkey_free   __x64_sys_pkey_free
 332common  statx   __x64_sys_statx
 333common  io_pgetevents   __x64_sys_io_pgetevents
+339common  open_tree   __x64_sys_open_tree
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/fs/file_table.c b/fs/file_table.c
index 7ec0b3e5f05d..7480271a0d21 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -189,6 +189,7 @@ static void __fput(struct file *file)
struct dentry *dentry = file->f_path.dentry;
struct vfsmount *mnt = file->f_path.mnt;
struct inode *inode = file->f_inode;
+   fmode_t mode = file->f_mode;
 
might_sleep();
 
@@ -209,14 +210,14 @@ static void __fput(struct file *file)
file->f_op->release(inode, file);
security_file_free(file);
if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
-!(file->f_mode & FMODE_PATH))) {
+!(mode & FMODE_PATH))) {
cdev_put(inode->i_cdev);
}
fops_put(file->f_op);
put_pid(file->f_owner.pid);
-   if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+   if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
i_readcount_dec(inode);
-   if (file->f_mode & FMODE_WRITER) {
+   if (mode & FMODE_WRITER) {
put_write_access(inode);
__mnt_drop_write(mnt);
}
@@ -224,6 +225,8 @@ static void __fput(struct file *file)
file->f_path.mnt = NULL;
file->f_inode = NULL;

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Sat, Jun 02, 2018 at 04:45:21PM +0100, David Howells wrote:
> Al Viro  wrote:
> 
> > TBH, I would probably prefer separate mount_setattr(2) for that kind
> > of work, with something like
> > int mount_setattr(int dirfd, const char *path, int flags, int attr)
> > *not* opening any files.
> > flags:
> > AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE
> 
> I would call these MOUNT_SETATTR_* rather than AT_*.

Why?  AT_EMPTY_PATH/AT_NO_AUTOMOUNT are common with other ...at()
syscalls; AT_RECURSIVE - maybe, but it's still more like AT_...
namespace fodder, IMO.

> > attr:
> > MOUNT_SETATTR_DEV   (1<<0)
> > MOUNT_SETATTR_NODEV (1<<0)|(1<<1)
> > MOUNT_SETATTR_EXEC  (1<<2)
> > MOUNT_SETATTR_NOEXEC(1<<2)|(1<<3)
> > MOUNT_SETATTR_SUID  (1<<4)
> > MOUNT_SETATTR_NOSUID(1<<4)|(1<<5)
> > MOUNT_SETATTR_RW(1<<6)
> > MOUNT_SETATTR_RO(1<<6)|(1<<7)
> > MOUNT_SETATTR_RELATIME  (1<<8)
> > MOUNT_SETATTR_NOATIME   (1<<8)|(1<<9)
> > MOUNT_SETATTR_NODIRATIME(1<<8)|(2<<9)
> > MOUNT_SETATTR_STRICTATIME   (1<<8)|(3<<9)
> > MOUNT_SETATTR_PRIVATE   (1<<11)
> > MOUNT_SETATTR_SHARED(1<<11)|(1<<12)
> > MOUNT_SETATTR_SLAVE (1<<11)|(2<<12)
> > MOUNT_SETATTR_UNBINDABLE(1<<11)|(3<<12)
> 
> So, I like this generally, some notes though:
> 
> I wonder if this should be two separate parameters, a mask and the settings?
> I'm not sure that's worth it since some of the mask bits would cover multiple
> settings.

Nah, better put those bits in the same word, as in above.  Here bits 0, 2, 4, 6,
8 and 11 tell which attributes are to be modified, with values to set living
in bits 1, 3, 5, 7, 9--10 and 12--13.  Look at the constants above..

> Also, should NODIRATIME be separate from the other *ATIME flags?  I do also
> like treating some of these settings as enumerations rather than a set of
> bits.

Huh?  That's precisely what I'm doing there: bit 8 is "want to change atime
settings", bits 9 and 10 hold a 4-element enumeration (rel/no/nodir/strict).
Similar for propagation settings (bit 11 indicates that we want to set those,
bits 12 and 13 - 4-element enum)...

> I would make the prototype:
> 
>   int mount_setattr(int dirfd, const char *path,
> unsigned int flags, unsigned int attr,
> void *reserved5);
> 
> Further, do we want to say you can either change the propagation type *or*
> reconfigure the mountpoint restrictions, but not both at the same time?

Why?  MOUNT_SETATTR_PRIVATE | MOUNT_NOATIME | MOUNT_SUID, i.e. 0010110001,
i.e. 0xb10 for "turn nosuid off, switch atime polcy to noatime, change 
propagation
to private, leave everything else as-is"...

And for fsck sake, what's that "void *reserved5" for?

> > With either openat() used as in this series, or explicit
> > int open_tree(int dirfd, const char *path, int flags)
> 
> Maybe open_mount(), grab_mount() or pick_mount()?
> 
> I wonder if fsopen()/fspick() should be create_fs()/open_fs()...
> 
> > returning a descriptor, with flags being
> > AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE, AT_CLONE
> > with AT_RECURSIVE without AT_CLONE being an error.
> 
> You also need an O_CLOEXEC equivalent.

Point.

> I would make it:
> 
>   OPEN_TREE_CLOEXEC   0x0001

Why not O_CLOEXEC, as with epoll_create()/timerfd_create()/etc.?

>   OPEN_TREE_EMPTY_PATH0x0002
>   OPEN_TREE_FOLLOW_SYMLINK0x0004
>   OPEN_TREE_NO_AUTOMOUNT  0x0008

Why?  How are those different from normal AT_EMPTY_PATH/AT_NO_AUTOMOUNT?

>   OPEN_TREE_CLONE 0x0010
>   OPEN_TREE_RECURSIVE 0x0020
> 
> adding the follow-symlinks so that you don't grab a symlink target by
> accident.  (Can you actually mount on top of a symlink?)

You can't - mount(2) uses LOOKUP_FOLLOW for mountpoint (well, user_path(),
actually).

> > Hell, might even add AT_UMOUNT for "open root and detach, to be dissolved on
> > close", incompatible with AT_CLONE.
> 
> Cute.  Guess you could do:
> 
>   fd = open_mount(..., OPEN_TREE_DETACH);
>   mount_setattr(fd, "",
> MOUNT_SETATTR_EMPTY_PATH,
> MOUNT_SETATTR_NOSUID, NULL);
>   move_mount(fd, "", ...);

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Sat, Jun 02, 2018 at 04:45:21PM +0100, David Howells wrote:
> Al Viro  wrote:
> 
> > TBH, I would probably prefer separate mount_setattr(2) for that kind
> > of work, with something like
> > int mount_setattr(int dirfd, const char *path, int flags, int attr)
> > *not* opening any files.
> > flags:
> > AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE
> 
> I would call these MOUNT_SETATTR_* rather than AT_*.

Why?  AT_EMPTY_PATH/AT_NO_AUTOMOUNT are common with other ...at()
syscalls; AT_RECURSIVE - maybe, but it's still more like AT_...
namespace fodder, IMO.

> > attr:
> > MOUNT_SETATTR_DEV   (1<<0)
> > MOUNT_SETATTR_NODEV (1<<0)|(1<<1)
> > MOUNT_SETATTR_EXEC  (1<<2)
> > MOUNT_SETATTR_NOEXEC(1<<2)|(1<<3)
> > MOUNT_SETATTR_SUID  (1<<4)
> > MOUNT_SETATTR_NOSUID(1<<4)|(1<<5)
> > MOUNT_SETATTR_RW(1<<6)
> > MOUNT_SETATTR_RO(1<<6)|(1<<7)
> > MOUNT_SETATTR_RELATIME  (1<<8)
> > MOUNT_SETATTR_NOATIME   (1<<8)|(1<<9)
> > MOUNT_SETATTR_NODIRATIME(1<<8)|(2<<9)
> > MOUNT_SETATTR_STRICTATIME   (1<<8)|(3<<9)
> > MOUNT_SETATTR_PRIVATE   (1<<11)
> > MOUNT_SETATTR_SHARED(1<<11)|(1<<12)
> > MOUNT_SETATTR_SLAVE (1<<11)|(2<<12)
> > MOUNT_SETATTR_UNBINDABLE(1<<11)|(3<<12)
> 
> So, I like this generally, some notes though:
> 
> I wonder if this should be two separate parameters, a mask and the settings?
> I'm not sure that's worth it since some of the mask bits would cover multiple
> settings.

Nah, better put those bits in the same word, as in above.  Here bits 0, 2, 4, 6,
8 and 11 tell which attributes are to be modified, with values to set living
in bits 1, 3, 5, 7, 9--10 and 12--13.  Look at the constants above..

> Also, should NODIRATIME be separate from the other *ATIME flags?  I do also
> like treating some of these settings as enumerations rather than a set of
> bits.

Huh?  That's precisely what I'm doing there: bit 8 is "want to change atime
settings", bits 9 and 10 hold a 4-element enumeration (rel/no/nodir/strict).
Similar for propagation settings (bit 11 indicates that we want to set those,
bits 12 and 13 - 4-element enum)...

> I would make the prototype:
> 
>   int mount_setattr(int dirfd, const char *path,
> unsigned int flags, unsigned int attr,
> void *reserved5);
> 
> Further, do we want to say you can either change the propagation type *or*
> reconfigure the mountpoint restrictions, but not both at the same time?

Why?  MOUNT_SETATTR_PRIVATE | MOUNT_NOATIME | MOUNT_SUID, i.e. 0010110001,
i.e. 0xb10 for "turn nosuid off, switch atime polcy to noatime, change 
propagation
to private, leave everything else as-is"...

And for fsck sake, what's that "void *reserved5" for?

> > With either openat() used as in this series, or explicit
> > int open_tree(int dirfd, const char *path, int flags)
> 
> Maybe open_mount(), grab_mount() or pick_mount()?
> 
> I wonder if fsopen()/fspick() should be create_fs()/open_fs()...
> 
> > returning a descriptor, with flags being
> > AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE, AT_CLONE
> > with AT_RECURSIVE without AT_CLONE being an error.
> 
> You also need an O_CLOEXEC equivalent.

Point.

> I would make it:
> 
>   OPEN_TREE_CLOEXEC   0x0001

Why not O_CLOEXEC, as with epoll_create()/timerfd_create()/etc.?

>   OPEN_TREE_EMPTY_PATH0x0002
>   OPEN_TREE_FOLLOW_SYMLINK0x0004
>   OPEN_TREE_NO_AUTOMOUNT  0x0008

Why?  How are those different from normal AT_EMPTY_PATH/AT_NO_AUTOMOUNT?

>   OPEN_TREE_CLONE 0x0010
>   OPEN_TREE_RECURSIVE 0x0020
> 
> adding the follow-symlinks so that you don't grab a symlink target by
> accident.  (Can you actually mount on top of a symlink?)

You can't - mount(2) uses LOOKUP_FOLLOW for mountpoint (well, user_path(),
actually).

> > Hell, might even add AT_UMOUNT for "open root and detach, to be dissolved on
> > close", incompatible with AT_CLONE.
> 
> Cute.  Guess you could do:
> 
>   fd = open_mount(..., OPEN_TREE_DETACH);
>   mount_setattr(fd, "",
> MOUNT_SETATTR_EMPTY_PATH,
> MOUNT_SETATTR_NOSUID, NULL);
>   move_mount(fd, "", ...);

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[David Howells]

Al Viro  wrote:

> TBH, I would probably prefer separate mount_setattr(2) for that kind
> of work, with something like
>   int mount_setattr(int dirfd, const char *path, int flags, int attr)
> *not* opening any files.
> flags:
>   AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE

I would call these MOUNT_SETATTR_* rather than AT_*.

> attr:
>   MOUNT_SETATTR_DEV   (1<<0)
>   MOUNT_SETATTR_NODEV (1<<0)|(1<<1)
>   MOUNT_SETATTR_EXEC  (1<<2)
>   MOUNT_SETATTR_NOEXEC(1<<2)|(1<<3)
>   MOUNT_SETATTR_SUID  (1<<4)
>   MOUNT_SETATTR_NOSUID(1<<4)|(1<<5)
>   MOUNT_SETATTR_RW(1<<6)
>   MOUNT_SETATTR_RO(1<<6)|(1<<7)
>   MOUNT_SETATTR_RELATIME  (1<<8)
>   MOUNT_SETATTR_NOATIME   (1<<8)|(1<<9)
>   MOUNT_SETATTR_NODIRATIME(1<<8)|(2<<9)
>   MOUNT_SETATTR_STRICTATIME   (1<<8)|(3<<9)
>   MOUNT_SETATTR_PRIVATE   (1<<11)
>   MOUNT_SETATTR_SHARED(1<<11)|(1<<12)
>   MOUNT_SETATTR_SLAVE (1<<11)|(2<<12)
>   MOUNT_SETATTR_UNBINDABLE(1<<11)|(3<<12)

So, I like this generally, some notes though:

I wonder if this should be two separate parameters, a mask and the settings?
I'm not sure that's worth it since some of the mask bits would cover multiple
settings.

Also, should NODIRATIME be separate from the other *ATIME flags?  I do also
like treating some of these settings as enumerations rather than a set of
bits.

I would make the prototype:

int mount_setattr(int dirfd, const char *path,
  unsigned int flags, unsigned int attr,
  void *reserved5);

Further, do we want to say you can either change the propagation type *or*
reconfigure the mountpoint restrictions, but not both at the same time?

> With either openat() used as in this series, or explicit
>   int open_tree(int dirfd, const char *path, int flags)

Maybe open_mount(), grab_mount() or pick_mount()?

I wonder if fsopen()/fspick() should be create_fs()/open_fs()...

> returning a descriptor, with flags being
>   AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE, AT_CLONE
> with AT_RECURSIVE without AT_CLONE being an error.

You also need an O_CLOEXEC equivalent.

I would make it:

OPEN_TREE_CLOEXEC   0x0001
OPEN_TREE_EMPTY_PATH0x0002
OPEN_TREE_FOLLOW_SYMLINK0x0004
OPEN_TREE_NO_AUTOMOUNT  0x0008
OPEN_TREE_CLONE 0x0010
OPEN_TREE_RECURSIVE 0x0020

adding the follow-symlinks so that you don't grab a symlink target by
accident.  (Can you actually mount on top of a symlink?)

> Hell, might even add AT_UMOUNT for "open root and detach, to be dissolved on
> close", incompatible with AT_CLONE.

Cute.  Guess you could do:

fd = open_mount(..., OPEN_TREE_DETACH);
mount_setattr(fd, "",
  MOUNT_SETATTR_EMPTY_PATH,
  MOUNT_SETATTR_NOSUID, NULL);
move_mount(fd, "", ...);

David

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[David Howells]

Al Viro  wrote:

> TBH, I would probably prefer separate mount_setattr(2) for that kind
> of work, with something like
>   int mount_setattr(int dirfd, const char *path, int flags, int attr)
> *not* opening any files.
> flags:
>   AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE

I would call these MOUNT_SETATTR_* rather than AT_*.

> attr:
>   MOUNT_SETATTR_DEV   (1<<0)
>   MOUNT_SETATTR_NODEV (1<<0)|(1<<1)
>   MOUNT_SETATTR_EXEC  (1<<2)
>   MOUNT_SETATTR_NOEXEC(1<<2)|(1<<3)
>   MOUNT_SETATTR_SUID  (1<<4)
>   MOUNT_SETATTR_NOSUID(1<<4)|(1<<5)
>   MOUNT_SETATTR_RW(1<<6)
>   MOUNT_SETATTR_RO(1<<6)|(1<<7)
>   MOUNT_SETATTR_RELATIME  (1<<8)
>   MOUNT_SETATTR_NOATIME   (1<<8)|(1<<9)
>   MOUNT_SETATTR_NODIRATIME(1<<8)|(2<<9)
>   MOUNT_SETATTR_STRICTATIME   (1<<8)|(3<<9)
>   MOUNT_SETATTR_PRIVATE   (1<<11)
>   MOUNT_SETATTR_SHARED(1<<11)|(1<<12)
>   MOUNT_SETATTR_SLAVE (1<<11)|(2<<12)
>   MOUNT_SETATTR_UNBINDABLE(1<<11)|(3<<12)

So, I like this generally, some notes though:

I wonder if this should be two separate parameters, a mask and the settings?
I'm not sure that's worth it since some of the mask bits would cover multiple
settings.

Also, should NODIRATIME be separate from the other *ATIME flags?  I do also
like treating some of these settings as enumerations rather than a set of
bits.

I would make the prototype:

int mount_setattr(int dirfd, const char *path,
  unsigned int flags, unsigned int attr,
  void *reserved5);

Further, do we want to say you can either change the propagation type *or*
reconfigure the mountpoint restrictions, but not both at the same time?

> With either openat() used as in this series, or explicit
>   int open_tree(int dirfd, const char *path, int flags)

Maybe open_mount(), grab_mount() or pick_mount()?

I wonder if fsopen()/fspick() should be create_fs()/open_fs()...

> returning a descriptor, with flags being
>   AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE, AT_CLONE
> with AT_RECURSIVE without AT_CLONE being an error.

You also need an O_CLOEXEC equivalent.

I would make it:

OPEN_TREE_CLOEXEC   0x0001
OPEN_TREE_EMPTY_PATH0x0002
OPEN_TREE_FOLLOW_SYMLINK0x0004
OPEN_TREE_NO_AUTOMOUNT  0x0008
OPEN_TREE_CLONE 0x0010
OPEN_TREE_RECURSIVE 0x0020

adding the follow-symlinks so that you don't grab a symlink target by
accident.  (Can you actually mount on top of a symlink?)

> Hell, might even add AT_UMOUNT for "open root and detach, to be dissolved on
> close", incompatible with AT_CLONE.

Cute.  Guess you could do:

fd = open_mount(..., OPEN_TREE_DETACH);
mount_setattr(fd, "",
  MOUNT_SETATTR_EMPTY_PATH,
  MOUNT_SETATTR_NOSUID, NULL);
move_mount(fd, "", ...);

David

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Sat, Jun 02, 2018 at 04:42:56AM +0100, Al Viro wrote:
> _If_ I'm interpreting that correctly, that should be something like a bitmap
> of attributes to modify + values to set for each.  Let's see -
>   propagation 1 + 2 bits
>   nodev   1 + 1
>   noexec  1 + 1
>   nosuid  1 + 1
>   ro  1 + 1
>   atime   1 + 3
> That's 15 bits.  On top of that, we have 1 bit for "clone or original"
> and 1 bit for "recursive or single-mount".  As well as AT_EMPTY_PATH,
> and AT_NO_AUTOMOUNT (inconvenient, since these are fixed bits).  In
> principle, that does fit into int, with some space to spare...
> 
> Is that what you have in mind?

TBH, I would probably prefer separate mount_setattr(2) for that kind
of work, with something like
int mount_setattr(int dirfd, const char *path, int flags, int attr)
*not* opening any files.
flags:
AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE
attr:
MOUNT_SETATTR_DEV   (1<<0)
MOUNT_SETATTR_NODEV (1<<0)|(1<<1)
MOUNT_SETATTR_EXEC  (1<<2)
MOUNT_SETATTR_NOEXEC(1<<2)|(1<<3)
MOUNT_SETATTR_SUID  (1<<4)
MOUNT_SETATTR_NOSUID(1<<4)|(1<<5)
MOUNT_SETATTR_RW(1<<6)
MOUNT_SETATTR_RO(1<<6)|(1<<7)
MOUNT_SETATTR_RELATIME  (1<<8)
MOUNT_SETATTR_NOATIME   (1<<8)|(1<<9)
MOUNT_SETATTR_NODIRATIME(1<<8)|(2<<9)
MOUNT_SETATTR_STRICTATIME   (1<<8)|(3<<9)
MOUNT_SETATTR_PRIVATE   (1<<11)
MOUNT_SETATTR_SHARED(1<<11)|(1<<12)
MOUNT_SETATTR_SLAVE (1<<11)|(2<<12)
MOUNT_SETATTR_UNBINDABLE(1<<11)|(3<<12)

With either openat() used as in this series, or explicit
int open_tree(int dirfd, const char *path, int flags)
returning a descriptor, with flags being
AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE, AT_CLONE
with AT_RECURSIVE without AT_CLONE being an error.  Hell, might
even add AT_UMOUNT for "open root and detach, to be dissolved on
close", incompatible with AT_CLONE.

Comments?

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Sat, Jun 02, 2018 at 04:42:56AM +0100, Al Viro wrote:
> _If_ I'm interpreting that correctly, that should be something like a bitmap
> of attributes to modify + values to set for each.  Let's see -
>   propagation 1 + 2 bits
>   nodev   1 + 1
>   noexec  1 + 1
>   nosuid  1 + 1
>   ro  1 + 1
>   atime   1 + 3
> That's 15 bits.  On top of that, we have 1 bit for "clone or original"
> and 1 bit for "recursive or single-mount".  As well as AT_EMPTY_PATH,
> and AT_NO_AUTOMOUNT (inconvenient, since these are fixed bits).  In
> principle, that does fit into int, with some space to spare...
> 
> Is that what you have in mind?

TBH, I would probably prefer separate mount_setattr(2) for that kind
of work, with something like
int mount_setattr(int dirfd, const char *path, int flags, int attr)
*not* opening any files.
flags:
AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE
attr:
MOUNT_SETATTR_DEV   (1<<0)
MOUNT_SETATTR_NODEV (1<<0)|(1<<1)
MOUNT_SETATTR_EXEC  (1<<2)
MOUNT_SETATTR_NOEXEC(1<<2)|(1<<3)
MOUNT_SETATTR_SUID  (1<<4)
MOUNT_SETATTR_NOSUID(1<<4)|(1<<5)
MOUNT_SETATTR_RW(1<<6)
MOUNT_SETATTR_RO(1<<6)|(1<<7)
MOUNT_SETATTR_RELATIME  (1<<8)
MOUNT_SETATTR_NOATIME   (1<<8)|(1<<9)
MOUNT_SETATTR_NODIRATIME(1<<8)|(2<<9)
MOUNT_SETATTR_STRICTATIME   (1<<8)|(3<<9)
MOUNT_SETATTR_PRIVATE   (1<<11)
MOUNT_SETATTR_SHARED(1<<11)|(1<<12)
MOUNT_SETATTR_SLAVE (1<<11)|(2<<12)
MOUNT_SETATTR_UNBINDABLE(1<<11)|(3<<12)

With either openat() used as in this series, or explicit
int open_tree(int dirfd, const char *path, int flags)
returning a descriptor, with flags being
AT_EMPTY_PATH, AT_NO_AUTOMOUNT, AT_RECURSIVE, AT_CLONE
with AT_RECURSIVE without AT_CLONE being an error.  Hell, might
even add AT_UMOUNT for "open root and detach, to be dissolved on
close", incompatible with AT_CLONE.

Comments?

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Sat, Jun 02, 2018 at 04:09:14AM +0100, Al Viro wrote:
> On Fri, Jun 01, 2018 at 09:27:43AM +0100, David Howells wrote:
> > Al Viro  wrote:
> > 
> > > > Instead of overloading this on open having a specific syscalls just
> > > > seems like a much saner idea.
> > > 
> > > It's not just mount API; these can be used independently of that.
> > > Think of the uses where you pass those to ...at() and you'll see
> > > a bunch of applications of that thing.
> > 
> > I kind of agree with Christoph on this point.  Yes, you can use the 
> > resultant
> > fd for other things, but that doesn't mean it has to be obtained initially
> > through open() or openat() rather than, say, a new pick_mount() syscall.
> > 
> > Further, having more parameters available gives us the opportunity to change
> > the settings on any mounts we create at the point of creation.
> 
> open_subtree(int dirfd, const char *pathname, int flags), then?  How would
> flags be interpreted?  What I see mapping at that thing is
>   * equivalent of O_PATH open
>   * clone subtree, O_PATH open root
>   * clone one mount, O_PATH open root
> and apparently you want to add (orthogonal to that)
>   * make shared/slave/private/unbindable
>   * ditto with recursion?
>   * same for nodev/nosuid/noexec/noatime/nodiratime/relatime/ro/?
> as well as usual AT_... flags (empty path, follow)
> 
> Choose the encoding...

_If_ I'm interpreting that correctly, that should be something like a bitmap
of attributes to modify + values to set for each.  Let's see -
propagation 1 + 2 bits
nodev   1 + 1
noexec  1 + 1
nosuid  1 + 1
ro  1 + 1
atime   1 + 3
That's 15 bits.  On top of that, we have 1 bit for "clone or original"
and 1 bit for "recursive or single-mount".  As well as AT_EMPTY_PATH,
and AT_NO_AUTOMOUNT (inconvenient, since these are fixed bits).  In
principle, that does fit into int, with some space to spare...

Is that what you have in mind?

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Sat, Jun 02, 2018 at 04:09:14AM +0100, Al Viro wrote:
> On Fri, Jun 01, 2018 at 09:27:43AM +0100, David Howells wrote:
> > Al Viro  wrote:
> > 
> > > > Instead of overloading this on open having a specific syscalls just
> > > > seems like a much saner idea.
> > > 
> > > It's not just mount API; these can be used independently of that.
> > > Think of the uses where you pass those to ...at() and you'll see
> > > a bunch of applications of that thing.
> > 
> > I kind of agree with Christoph on this point.  Yes, you can use the 
> > resultant
> > fd for other things, but that doesn't mean it has to be obtained initially
> > through open() or openat() rather than, say, a new pick_mount() syscall.
> > 
> > Further, having more parameters available gives us the opportunity to change
> > the settings on any mounts we create at the point of creation.
> 
> open_subtree(int dirfd, const char *pathname, int flags), then?  How would
> flags be interpreted?  What I see mapping at that thing is
>   * equivalent of O_PATH open
>   * clone subtree, O_PATH open root
>   * clone one mount, O_PATH open root
> and apparently you want to add (orthogonal to that)
>   * make shared/slave/private/unbindable
>   * ditto with recursion?
>   * same for nodev/nosuid/noexec/noatime/nodiratime/relatime/ro/?
> as well as usual AT_... flags (empty path, follow)
> 
> Choose the encoding...

_If_ I'm interpreting that correctly, that should be something like a bitmap
of attributes to modify + values to set for each.  Let's see -
propagation 1 + 2 bits
nodev   1 + 1
noexec  1 + 1
nosuid  1 + 1
ro  1 + 1
atime   1 + 3
That's 15 bits.  On top of that, we have 1 bit for "clone or original"
and 1 bit for "recursive or single-mount".  As well as AT_EMPTY_PATH,
and AT_NO_AUTOMOUNT (inconvenient, since these are fixed bits).  In
principle, that does fit into int, with some space to spare...

Is that what you have in mind?

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Fri, Jun 01, 2018 at 09:27:43AM +0100, David Howells wrote:
> Al Viro  wrote:
> 
> > > Instead of overloading this on open having a specific syscalls just
> > > seems like a much saner idea.
> > 
> > It's not just mount API; these can be used independently of that.
> > Think of the uses where you pass those to ...at() and you'll see
> > a bunch of applications of that thing.
> 
> I kind of agree with Christoph on this point.  Yes, you can use the resultant
> fd for other things, but that doesn't mean it has to be obtained initially
> through open() or openat() rather than, say, a new pick_mount() syscall.
> 
> Further, having more parameters available gives us the opportunity to change
> the settings on any mounts we create at the point of creation.

open_subtree(int dirfd, const char *pathname, int flags), then?  How would
flags be interpreted?  What I see mapping at that thing is
* equivalent of O_PATH open
* clone subtree, O_PATH open root
* clone one mount, O_PATH open root
and apparently you want to add (orthogonal to that)
* make shared/slave/private/unbindable
* ditto with recursion?
* same for nodev/nosuid/noexec/noatime/nodiratime/relatime/ro/?
as well as usual AT_... flags (empty path, follow)

Choose the encoding...

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Fri, Jun 01, 2018 at 09:27:43AM +0100, David Howells wrote:
> Al Viro  wrote:
> 
> > > Instead of overloading this on open having a specific syscalls just
> > > seems like a much saner idea.
> > 
> > It's not just mount API; these can be used independently of that.
> > Think of the uses where you pass those to ...at() and you'll see
> > a bunch of applications of that thing.
> 
> I kind of agree with Christoph on this point.  Yes, you can use the resultant
> fd for other things, but that doesn't mean it has to be obtained initially
> through open() or openat() rather than, say, a new pick_mount() syscall.
> 
> Further, having more parameters available gives us the opportunity to change
> the settings on any mounts we create at the point of creation.

open_subtree(int dirfd, const char *pathname, int flags), then?  How would
flags be interpreted?  What I see mapping at that thing is
* equivalent of O_PATH open
* clone subtree, O_PATH open root
* clone one mount, O_PATH open root
and apparently you want to add (orthogonal to that)
* make shared/slave/private/unbindable
* ditto with recursion?
* same for nodev/nosuid/noexec/noatime/nodiratime/relatime/ro/?
as well as usual AT_... flags (empty path, follow)

Choose the encoding...

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[David Howells]

Amir Goldstein  wrote:

> Reject O_NON_RECURSIVE without O_CLONE_MOUNT?

Yes, I should add that.

> I am not sure what are the consequences of opening O_PATH with old kernel
> and getting an open file, can't think of anything bad.
> Can the same be claimed for O_PATH|O_CLONE_MOUNT?

Yes, actually, there can be consequences.  Some files have side effects.
Think open("/dev/foobar", O_PATH).

> Wouldn't it be better to apply the O_TMPFILE kludge to the new
> open flag, so that apps can check if O_CLONE_MOUNT feature is supported
> by kernel?

Ugh.  The problem is that the O_TMPFILE kludge can't be done because O_PATH
currently just masks off any bits it's not interested in rather than giving an
error.

Even the O_TMPFILE kludge doesn't protect you against someone having set
random unassigned bits when testing on a kernel that didn't support it.

And this bit:

/*
 * Clear out all open flags we don't know about so that we don't report
 * them in fcntl(F_GETFD) or similar interfaces.
 */
flags &= VALID_OPEN_FLAGS;

is just plain wrong.  Effectively, it allows userspace to set random reserved
bits without consequences.  It should give an error instead.

Probably we should really replace open() and openat() both before we can
allocate any further open flags.



David

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[David Howells]

Amir Goldstein  wrote:

> Reject O_NON_RECURSIVE without O_CLONE_MOUNT?

Yes, I should add that.

> I am not sure what are the consequences of opening O_PATH with old kernel
> and getting an open file, can't think of anything bad.
> Can the same be claimed for O_PATH|O_CLONE_MOUNT?

Yes, actually, there can be consequences.  Some files have side effects.
Think open("/dev/foobar", O_PATH).

> Wouldn't it be better to apply the O_TMPFILE kludge to the new
> open flag, so that apps can check if O_CLONE_MOUNT feature is supported
> by kernel?

Ugh.  The problem is that the O_TMPFILE kludge can't be done because O_PATH
currently just masks off any bits it's not interested in rather than giving an
error.

Even the O_TMPFILE kludge doesn't protect you against someone having set
random unassigned bits when testing on a kernel that didn't support it.

And this bit:

/*
 * Clear out all open flags we don't know about so that we don't report
 * them in fcntl(F_GETFD) or similar interfaces.
 */
flags &= VALID_OPEN_FLAGS;

is just plain wrong.  Effectively, it allows userspace to set random reserved
bits without consequences.  It should give an error instead.

Probably we should really replace open() and openat() both before we can
allocate any further open flags.



David

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[David Howells]

Al Viro  wrote:

> > Instead of overloading this on open having a specific syscalls just
> > seems like a much saner idea.
> 
> It's not just mount API; these can be used independently of that.
> Think of the uses where you pass those to ...at() and you'll see
> a bunch of applications of that thing.

I kind of agree with Christoph on this point.  Yes, you can use the resultant
fd for other things, but that doesn't mean it has to be obtained initially
through open() or openat() rather than, say, a new pick_mount() syscall.

Further, having more parameters available gives us the opportunity to change
the settings on any mounts we create at the point of creation.

David

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[David Howells]

Al Viro  wrote:

> > Instead of overloading this on open having a specific syscalls just
> > seems like a much saner idea.
> 
> It's not just mount API; these can be used independently of that.
> Think of the uses where you pass those to ...at() and you'll see
> a bunch of applications of that thing.

I kind of agree with Christoph on this point.  Yes, you can use the resultant
fd for other things, but that doesn't mean it has to be obtained initially
through open() or openat() rather than, say, a new pick_mount() syscall.

Further, having more parameters available gives us the opportunity to change
the settings on any mounts we create at the point of creation.

David

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Amir Goldstein]

[added linux-api]

On Fri, May 25, 2018 at 3:08 AM, David Howells  wrote:
> Make it possible to clone a mount tree with a new pair of open flags that
> are used in conjunction with O_PATH:
>
>  (1) O_CLONE_MOUNT - Clone the mount or mount tree at the path.
>
>  (2) O_NON_RECURSIVE - Don't clone recursively.
>
> Note that it's not a good idea to reuse other flags (such as O_CREAT)
> because the open routine for O_PATH does not give an error if any other
> flags are used in conjunction with O_PATH, but rather just masks off any it
> doesn't use.
>
> The resultant file struct is marked FMODE_NEED_UNMOUNT to as it pins an
> extra reference for the mount.  This will be cleared by the upcoming
> move_mount() syscall when it successfully moves a cloned mount into the
> filesystem tree.
>
> Note that care needs to be taken with the error handling in do_o_path() in
> the case that vfs_open() fails as the path may or may not have been
> attached to the file struct and FMODE_NEED_UNMOUNT may or may not be set.
> Note that O_DIRECT | O_PATH could be a problem with error handling too.
>
> Signed-off-by: David Howells 
> ---
>
[...]

> @@ -977,8 +979,11 @@ static inline int build_open_flags(int flags, umode_t 
> mode, struct open_flags *o
>  * If we have O_PATH in the open flag. Then we
>  * cannot have anything other than the below set of flags
>  */
> -   flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH;
> +   flags &= (O_DIRECTORY | O_NOFOLLOW | O_PATH |
> + O_CLONE_MOUNT | O_NON_RECURSIVE);
> acc_mode = 0;
> +   } else if (flags & (O_CLONE_MOUNT | O_NON_RECURSIVE)) {
> +   return -EINVAL;

Reject O_NON_RECURSIVE without O_CLONE_MOUNT?
That would free at least one flag combination for future use.

Doesn't it make more sense for user API to opt-into
O_RECURSIVE_CLONE, rather than opt-out of it?


> }
>
> op->open_flag = flags;
> diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> index 27dc7a60693e..8f60e2244740 100644
> --- a/include/linux/fcntl.h
> +++ b/include/linux/fcntl.h
> @@ -9,7 +9,8 @@
> (O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC 
> | \
>  O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
>  FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
> -O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
> +O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | \
> +O_CLONE_MOUNT | O_NON_RECURSIVE)
>
>  #ifndef force_o_largefile
>  #define force_o_largefile() (BITS_PER_LONG != 32)
> diff --git a/include/uapi/asm-generic/fcntl.h 
> b/include/uapi/asm-generic/fcntl.h
> index 0b1c7e35090c..f533e35ea19b 100644
> --- a/include/uapi/asm-generic/fcntl.h
> +++ b/include/uapi/asm-generic/fcntl.h
> @@ -88,6 +88,14 @@
>  #define __O_TMPFILE02000
>  #endif
>
> +#ifndef O_CLONE_MOUNT
> +#define O_CLONE_MOUNT  04000   /* Used with O_PATH to clone the 
> mount subtree at path */
> +#endif
> +
> +#ifndef O_NON_RECURSIVE
> +#define O_NON_RECURSIVE01  /* Used with O_CLONE_MOUNT to 
> only clone one mount */
> +#endif
> +
>  /* a horrid kludge trying to make sure that this will fail on old kernels */
>  #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
>  #define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)
>

I am not sure what are the consequences of opening O_PATH with old kernel
and getting an open file, can't think of anything bad.
Can the same be claimed for O_PATH|O_CLONE_MOUNT?

Wouldn't it be better to apply the O_TMPFILE kludge to the new
open flag, so that apps can check if O_CLONE_MOUNT feature is supported
by kernel?

Thanks,
Amir.

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Amir Goldstein]

[added linux-api]

On Fri, May 25, 2018 at 3:08 AM, David Howells  wrote:
> Make it possible to clone a mount tree with a new pair of open flags that
> are used in conjunction with O_PATH:
>
>  (1) O_CLONE_MOUNT - Clone the mount or mount tree at the path.
>
>  (2) O_NON_RECURSIVE - Don't clone recursively.
>
> Note that it's not a good idea to reuse other flags (such as O_CREAT)
> because the open routine for O_PATH does not give an error if any other
> flags are used in conjunction with O_PATH, but rather just masks off any it
> doesn't use.
>
> The resultant file struct is marked FMODE_NEED_UNMOUNT to as it pins an
> extra reference for the mount.  This will be cleared by the upcoming
> move_mount() syscall when it successfully moves a cloned mount into the
> filesystem tree.
>
> Note that care needs to be taken with the error handling in do_o_path() in
> the case that vfs_open() fails as the path may or may not have been
> attached to the file struct and FMODE_NEED_UNMOUNT may or may not be set.
> Note that O_DIRECT | O_PATH could be a problem with error handling too.
>
> Signed-off-by: David Howells 
> ---
>
[...]

> @@ -977,8 +979,11 @@ static inline int build_open_flags(int flags, umode_t 
> mode, struct open_flags *o
>  * If we have O_PATH in the open flag. Then we
>  * cannot have anything other than the below set of flags
>  */
> -   flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH;
> +   flags &= (O_DIRECTORY | O_NOFOLLOW | O_PATH |
> + O_CLONE_MOUNT | O_NON_RECURSIVE);
> acc_mode = 0;
> +   } else if (flags & (O_CLONE_MOUNT | O_NON_RECURSIVE)) {
> +   return -EINVAL;

Reject O_NON_RECURSIVE without O_CLONE_MOUNT?
That would free at least one flag combination for future use.

Doesn't it make more sense for user API to opt-into
O_RECURSIVE_CLONE, rather than opt-out of it?


> }
>
> op->open_flag = flags;
> diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> index 27dc7a60693e..8f60e2244740 100644
> --- a/include/linux/fcntl.h
> +++ b/include/linux/fcntl.h
> @@ -9,7 +9,8 @@
> (O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC 
> | \
>  O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
>  FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
> -O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
> +O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | \
> +O_CLONE_MOUNT | O_NON_RECURSIVE)
>
>  #ifndef force_o_largefile
>  #define force_o_largefile() (BITS_PER_LONG != 32)
> diff --git a/include/uapi/asm-generic/fcntl.h 
> b/include/uapi/asm-generic/fcntl.h
> index 0b1c7e35090c..f533e35ea19b 100644
> --- a/include/uapi/asm-generic/fcntl.h
> +++ b/include/uapi/asm-generic/fcntl.h
> @@ -88,6 +88,14 @@
>  #define __O_TMPFILE02000
>  #endif
>
> +#ifndef O_CLONE_MOUNT
> +#define O_CLONE_MOUNT  04000   /* Used with O_PATH to clone the 
> mount subtree at path */
> +#endif
> +
> +#ifndef O_NON_RECURSIVE
> +#define O_NON_RECURSIVE01  /* Used with O_CLONE_MOUNT to 
> only clone one mount */
> +#endif
> +
>  /* a horrid kludge trying to make sure that this will fail on old kernels */
>  #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
>  #define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)
>

I am not sure what are the consequences of opening O_PATH with old kernel
and getting an open file, can't think of anything bad.
Can the same be claimed for O_PATH|O_CLONE_MOUNT?

Wouldn't it be better to apply the O_TMPFILE kludge to the new
open flag, so that apps can check if O_CLONE_MOUNT feature is supported
by kernel?

Thanks,
Amir.

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Thu, May 31, 2018 at 11:26:54PM -0700, Christoph Hellwig wrote:
> On Fri, May 25, 2018 at 01:08:38AM +0100, David Howells wrote:
> > Make it possible to clone a mount tree with a new pair of open flags that
> > are used in conjunction with O_PATH:
> > 
> >  (1) O_CLONE_MOUNT - Clone the mount or mount tree at the path.
> > 
> >  (2) O_NON_RECURSIVE - Don't clone recursively.
> 
> Err.  I don't think we should use up two O_* flags for something
> only useful for your new mount API.  Don't we have a better place
> to for these flags?
> 
> Instead of overloading this on open having a specific syscalls just
> seems like a much saner idea.

It's not just mount API; these can be used independently of that.
Think of the uses where you pass those to ...at() and you'll see
a bunch of applications of that thing.

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Al Viro]

On Thu, May 31, 2018 at 11:26:54PM -0700, Christoph Hellwig wrote:
> On Fri, May 25, 2018 at 01:08:38AM +0100, David Howells wrote:
> > Make it possible to clone a mount tree with a new pair of open flags that
> > are used in conjunction with O_PATH:
> > 
> >  (1) O_CLONE_MOUNT - Clone the mount or mount tree at the path.
> > 
> >  (2) O_NON_RECURSIVE - Don't clone recursively.
> 
> Err.  I don't think we should use up two O_* flags for something
> only useful for your new mount API.  Don't we have a better place
> to for these flags?
> 
> Instead of overloading this on open having a specific syscalls just
> seems like a much saner idea.

It's not just mount API; these can be used independently of that.
Think of the uses where you pass those to ...at() and you'll see
a bunch of applications of that thing.

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Christoph Hellwig]

On Fri, May 25, 2018 at 01:08:38AM +0100, David Howells wrote:
> Make it possible to clone a mount tree with a new pair of open flags that
> are used in conjunction with O_PATH:
> 
>  (1) O_CLONE_MOUNT - Clone the mount or mount tree at the path.
> 
>  (2) O_NON_RECURSIVE - Don't clone recursively.

Err.  I don't think we should use up two O_* flags for something
only useful for your new mount API.  Don't we have a better place
to for these flags?

Instead of overloading this on open having a specific syscalls just
seems like a much saner idea.

Re: [PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[Christoph Hellwig]

On Fri, May 25, 2018 at 01:08:38AM +0100, David Howells wrote:
> Make it possible to clone a mount tree with a new pair of open flags that
> are used in conjunction with O_PATH:
> 
>  (1) O_CLONE_MOUNT - Clone the mount or mount tree at the path.
> 
>  (2) O_NON_RECURSIVE - Don't clone recursively.

Err.  I don't think we should use up two O_* flags for something
only useful for your new mount API.  Don't we have a better place
to for these flags?

Instead of overloading this on open having a specific syscalls just
seems like a much saner idea.

[PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[David Howells]

Make it possible to clone a mount tree with a new pair of open flags that
are used in conjunction with O_PATH:

 (1) O_CLONE_MOUNT - Clone the mount or mount tree at the path.

 (2) O_NON_RECURSIVE - Don't clone recursively.

Note that it's not a good idea to reuse other flags (such as O_CREAT)
because the open routine for O_PATH does not give an error if any other
flags are used in conjunction with O_PATH, but rather just masks off any it
doesn't use.

The resultant file struct is marked FMODE_NEED_UNMOUNT to as it pins an
extra reference for the mount.  This will be cleared by the upcoming
move_mount() syscall when it successfully moves a cloned mount into the
filesystem tree.

Note that care needs to be taken with the error handling in do_o_path() in
the case that vfs_open() fails as the path may or may not have been
attached to the file struct and FMODE_NEED_UNMOUNT may or may not be set.
Note that O_DIRECT | O_PATH could be a problem with error handling too.

Signed-off-by: David Howells 
---

 fs/fcntl.c   |2 +-
 fs/internal.h|1 +
 fs/namei.c   |   26 ++
 fs/namespace.c   |   44 ++
 fs/open.c|7 +-
 include/linux/fcntl.h|3 ++-
 include/uapi/asm-generic/fcntl.h |8 +++
 7 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 60bc5bf2f4cf..42a53cf03737 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1028,7 +1028,7 @@ static int __init fcntl_init(void)
 * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
 * is defined as O_NONBLOCK on some platforms and not on others.
 */
-   BUILD_BUG_ON(19 - 1 /* for O_RDONLY being 0 */ !=
+   BUILD_BUG_ON(20 - 1 /* for O_RDONLY being 0 */ !=
 HWEIGHT32(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)));
 
fasync_cache = kmem_cache_create("fasync_cache",
diff --git a/fs/internal.h b/fs/internal.h
index c29552e0522f..e3460a2e6b59 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -75,6 +75,7 @@ extern struct vfsmount *lookup_mnt(const struct path *);
 extern int finish_automount(struct vfsmount *, struct path *);
 
 extern int sb_prepare_remount_readonly(struct super_block *);
+extern int copy_mount_for_o_path(struct path *, struct path *, bool);
 
 extern void __init mnt_init(void);
 
diff --git a/fs/namei.c b/fs/namei.c
index 5cbd980b4031..acb8e27d4288 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3458,13 +3458,29 @@ static int do_tmpfile(struct nameidata *nd, unsigned 
flags,
 
 static int do_o_path(struct nameidata *nd, unsigned flags, struct file *file)
 {
-   struct path path;
-   int error = path_lookupat(nd, flags, );
-   if (!error) {
-   audit_inode(nd->name, path.dentry, 0);
-   error = vfs_open(, file, current_cred());
+   struct path path, tmp;
+   int error;
+
+   error = path_lookupat(nd, flags, );
+   if (error)
+   return error;
+
+   if (file->f_flags & O_CLONE_MOUNT) {
+   error = copy_mount_for_o_path(
+   , , !(file->f_flags & O_NON_RECURSIVE));
path_put();
+   if (error < 0)
+   return error;
+   path = tmp;
}
+
+   audit_inode(nd->name, path.dentry, 0);
+   error = vfs_open(, file, current_cred());
+   if (error < 0 &&
+   (flags & O_CLONE_MOUNT) &&
+   !(file->f_mode & FMODE_NEED_UNMOUNT))
+   __detach_mounts(path.dentry);
+   path_put();
return error;
 }
 
diff --git a/fs/namespace.c b/fs/namespace.c
index dba680aa1ea4..e73cfcdfb3d1 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2218,6 +2218,50 @@ static int do_loopback(struct path *path, const char 
*old_name,
return err;
 }
 
+/*
+ * Copy the mount or mount subtree at the specified path for
+ * open(O_PATH|O_CLONE_MOUNT).
+ */
+int copy_mount_for_o_path(struct path *from, struct path *to, bool recurse)
+{
+   struct mountpoint *mp;
+   struct mount *mnt = NULL, *f = real_mount(from->mnt);
+   int ret;
+
+   mp = lock_mount(from);
+   if (IS_ERR(mp))
+   return PTR_ERR(mp);
+
+   ret = -EINVAL;
+   if (IS_MNT_UNBINDABLE(f))
+   goto out_unlock;
+
+   if (!check_mnt(f) && from->dentry->d_op != _dentry_operations)
+   goto out_unlock;
+
+   if (!recurse && has_locked_children(f, from->dentry))
+   goto out_unlock;
+
+   if (recurse)
+   mnt = copy_tree(f, from->dentry, CL_COPY_MNT_NS_FILE);
+   else
+   mnt = clone_mnt(f, from->dentry, 0);
+   if (IS_ERR(mnt)) {
+   ret = PTR_ERR(mnt);
+   goto out_unlock;
+   }
+
+   mnt->mnt.mnt_flags &= ~MNT_LOCKED;
+
+   to->mnt = >mnt;
+   to->dentry = 

[PATCH 30/32] vfs: Allow cloning of a mount tree with open(O_PATH|O_CLONE_MOUNT) [ver #8]

[David Howells]

Make it possible to clone a mount tree with a new pair of open flags that
are used in conjunction with O_PATH:

 (1) O_CLONE_MOUNT - Clone the mount or mount tree at the path.

 (2) O_NON_RECURSIVE - Don't clone recursively.

Note that it's not a good idea to reuse other flags (such as O_CREAT)
because the open routine for O_PATH does not give an error if any other
flags are used in conjunction with O_PATH, but rather just masks off any it
doesn't use.

The resultant file struct is marked FMODE_NEED_UNMOUNT to as it pins an
extra reference for the mount.  This will be cleared by the upcoming
move_mount() syscall when it successfully moves a cloned mount into the
filesystem tree.

Note that care needs to be taken with the error handling in do_o_path() in
the case that vfs_open() fails as the path may or may not have been
attached to the file struct and FMODE_NEED_UNMOUNT may or may not be set.
Note that O_DIRECT | O_PATH could be a problem with error handling too.

Signed-off-by: David Howells 
---

 fs/fcntl.c   |2 +-
 fs/internal.h|1 +
 fs/namei.c   |   26 ++
 fs/namespace.c   |   44 ++
 fs/open.c|7 +-
 include/linux/fcntl.h|3 ++-
 include/uapi/asm-generic/fcntl.h |8 +++
 7 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 60bc5bf2f4cf..42a53cf03737 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1028,7 +1028,7 @@ static int __init fcntl_init(void)
 * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
 * is defined as O_NONBLOCK on some platforms and not on others.
 */
-   BUILD_BUG_ON(19 - 1 /* for O_RDONLY being 0 */ !=
+   BUILD_BUG_ON(20 - 1 /* for O_RDONLY being 0 */ !=
 HWEIGHT32(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)));
 
fasync_cache = kmem_cache_create("fasync_cache",
diff --git a/fs/internal.h b/fs/internal.h
index c29552e0522f..e3460a2e6b59 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -75,6 +75,7 @@ extern struct vfsmount *lookup_mnt(const struct path *);
 extern int finish_automount(struct vfsmount *, struct path *);
 
 extern int sb_prepare_remount_readonly(struct super_block *);
+extern int copy_mount_for_o_path(struct path *, struct path *, bool);
 
 extern void __init mnt_init(void);
 
diff --git a/fs/namei.c b/fs/namei.c
index 5cbd980b4031..acb8e27d4288 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3458,13 +3458,29 @@ static int do_tmpfile(struct nameidata *nd, unsigned 
flags,
 
 static int do_o_path(struct nameidata *nd, unsigned flags, struct file *file)
 {
-   struct path path;
-   int error = path_lookupat(nd, flags, );
-   if (!error) {
-   audit_inode(nd->name, path.dentry, 0);
-   error = vfs_open(, file, current_cred());
+   struct path path, tmp;
+   int error;
+
+   error = path_lookupat(nd, flags, );
+   if (error)
+   return error;
+
+   if (file->f_flags & O_CLONE_MOUNT) {
+   error = copy_mount_for_o_path(
+   , , !(file->f_flags & O_NON_RECURSIVE));
path_put();
+   if (error < 0)
+   return error;
+   path = tmp;
}
+
+   audit_inode(nd->name, path.dentry, 0);
+   error = vfs_open(, file, current_cred());
+   if (error < 0 &&
+   (flags & O_CLONE_MOUNT) &&
+   !(file->f_mode & FMODE_NEED_UNMOUNT))
+   __detach_mounts(path.dentry);
+   path_put();
return error;
 }
 
diff --git a/fs/namespace.c b/fs/namespace.c
index dba680aa1ea4..e73cfcdfb3d1 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2218,6 +2218,50 @@ static int do_loopback(struct path *path, const char 
*old_name,
return err;
 }
 
+/*
+ * Copy the mount or mount subtree at the specified path for
+ * open(O_PATH|O_CLONE_MOUNT).
+ */
+int copy_mount_for_o_path(struct path *from, struct path *to, bool recurse)
+{
+   struct mountpoint *mp;
+   struct mount *mnt = NULL, *f = real_mount(from->mnt);
+   int ret;
+
+   mp = lock_mount(from);
+   if (IS_ERR(mp))
+   return PTR_ERR(mp);
+
+   ret = -EINVAL;
+   if (IS_MNT_UNBINDABLE(f))
+   goto out_unlock;
+
+   if (!check_mnt(f) && from->dentry->d_op != _dentry_operations)
+   goto out_unlock;
+
+   if (!recurse && has_locked_children(f, from->dentry))
+   goto out_unlock;
+
+   if (recurse)
+   mnt = copy_tree(f, from->dentry, CL_COPY_MNT_NS_FILE);
+   else
+   mnt = clone_mnt(f, from->dentry, 0);
+   if (IS_ERR(mnt)) {
+   ret = PTR_ERR(mnt);
+   goto out_unlock;
+   }
+
+   mnt->mnt.mnt_flags &= ~MNT_LOCKED;
+
+   to->mnt = >mnt;
+   to->dentry = dget(from->dentry);
+