| Message ID | 20240919140611.1771651-3-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | API for exporting connectable file handles to userspace | expand |
On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote: > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2). > This was not allowed before, so we use it to enable a new API for > decoding "connectable" file handles that were created using the > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2). > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested, > use that as a hint to try to decode a "connected" fd with known path, > which is accessible (to capable user) from mount fd path. > > Note that this does not check if the path is accessible to the calling > user, just that it is accessible wrt the mount namesapce, so if there > is no "connected" alias, or if parts of the path are hidden in the > mount namespace, open_by_handle_at(2) will return -ESTALE. > > Note that the file handles used to decode a "connected" fd do not have > to be encoded with the AT_HANDLE_CONNECTABLE flag. Specifically, > directory file handles are always "connectable", regardless of using > the AT_HANDLE_CONNECTABLE flag. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/fhandle.c | 61 +++++++++++++++++++++++++++++++--------------------- > 1 file changed, 37 insertions(+), 24 deletions(-) > The mountfd is only used to get a path, so I don't see a problem with allowing that to be an O_PATH fd. I'm less keen on using the fact that mountfd is an O_PATH fd to change the behaviour of open_by_handle_at(). That seems very subtle. Is there a good reason to do it that way instead of just declaring a new AT_* flag for this? > diff --git a/fs/fhandle.c b/fs/fhandle.c > index 956d9b25d4f7..1fabfb79fd55 100644 > --- a/fs/fhandle.c > +++ b/fs/fhandle.c > @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name, > return err; > } > > -static int get_path_from_fd(int fd, struct path *root) > +enum handle_to_path_flags { > + HANDLE_CHECK_PERMS = (1 << 0), > + HANDLE_CHECK_SUBTREE = (1 << 1), > +}; > + > +struct handle_to_path_ctx { > + struct path root; > + enum handle_to_path_flags flags; > + unsigned int fh_flags; > + unsigned int o_flags; > +}; > + > +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx) > { > if (fd == AT_FDCWD) { > struct fs_struct *fs = current->fs; > spin_lock(&fs->lock); > - *root = fs->pwd; > - path_get(root); > + ctx->root = fs->pwd; > + path_get(&ctx->root); > spin_unlock(&fs->lock); > } else { > - struct fd f = fdget(fd); > + struct fd f = fdget_raw(fd); > if (!f.file) > return -EBADF; > - *root = f.file->f_path; > - path_get(root); > + ctx->root = f.file->f_path; > + path_get(&ctx->root); > + /* > + * Use O_PATH mount fd and requested O_PATH fd as a hint for > + * decoding an fd with connected path, that is accessible from > + * the mount fd path. > + */ > + if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH) > + ctx->flags |= HANDLE_CHECK_SUBTREE; > fdput(f); > } > > return 0; > } > > -enum handle_to_path_flags { > - HANDLE_CHECK_PERMS = (1 << 0), > - HANDLE_CHECK_SUBTREE = (1 << 1), > -}; > - > -struct handle_to_path_ctx { > - struct path root; > - enum handle_to_path_flags flags; > - unsigned int fh_flags; > -}; > - > static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > { > struct handle_to_path_ctx *ctx = context; > @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > > if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root) > retval = 1; > - WARN_ON_ONCE(d != root && d != root->d_sb->s_root); > + /* > + * exportfs_decode_fh_raw() does not call acceptable() callback with > + * a disconnected directory dentry, so we should have reached either > + * mount fd directory or sb root. > + */ > + if (ctx->fh_flags & EXPORT_FH_DIR_ONLY) > + WARN_ON_ONCE(d != root && d != root->d_sb->s_root); I don't quite understand why the above change is necessary. Can you explain why we need to limit this only to the case where EXPORT_FH_DIR_ONLY is set? > dput(d); > return retval; > } > @@ -265,8 +279,7 @@ static int do_handle_to_path(struct file_handle *handle, struct path *path, > * filesystem but that only applies to procfs and sysfs neither of which > * support decoding file handles. > */ > -static inline bool may_decode_fh(struct handle_to_path_ctx *ctx, > - unsigned int o_flags) > +static inline bool may_decode_fh(struct handle_to_path_ctx *ctx) > { > struct path *root = &ctx->root; > > @@ -276,7 +289,7 @@ static inline bool may_decode_fh(struct handle_to_path_ctx *ctx, > * > * There's only one dentry for each directory inode (VFS rule)... > */ > - if (!(o_flags & O_DIRECTORY)) > + if (!(ctx->o_flags & O_DIRECTORY)) > return false; > > if (ns_capable(root->mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN)) > @@ -303,13 +316,13 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh, > int retval = 0; > struct file_handle f_handle; > struct file_handle *handle = NULL; > - struct handle_to_path_ctx ctx = {}; > + struct handle_to_path_ctx ctx = { .o_flags = o_flags }; > > - retval = get_path_from_fd(mountdirfd, &ctx.root); > + retval = get_path_from_fd(mountdirfd, &ctx); > if (retval) > goto out_err; > > - if (!capable(CAP_DAC_READ_SEARCH) && !may_decode_fh(&ctx, o_flags)) { > + if (!capable(CAP_DAC_READ_SEARCH) && !may_decode_fh(&ctx)) { > retval = -EPERM; > goto out_path; > }
On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote: > > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2). > > This was not allowed before, so we use it to enable a new API for > > decoding "connectable" file handles that were created using the > > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2). > > > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested, > > use that as a hint to try to decode a "connected" fd with known path, > > which is accessible (to capable user) from mount fd path. > > > > Note that this does not check if the path is accessible to the calling > > user, just that it is accessible wrt the mount namesapce, so if there > > is no "connected" alias, or if parts of the path are hidden in the > > mount namespace, open_by_handle_at(2) will return -ESTALE. > > > > Note that the file handles used to decode a "connected" fd do not have > > to be encoded with the AT_HANDLE_CONNECTABLE flag. Specifically, > > directory file handles are always "connectable", regardless of using > > the AT_HANDLE_CONNECTABLE flag. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/fhandle.c | 61 +++++++++++++++++++++++++++++++--------------------- > > 1 file changed, 37 insertions(+), 24 deletions(-) > > > > The mountfd is only used to get a path, so I don't see a problem with > allowing that to be an O_PATH fd. > > I'm less keen on using the fact that mountfd is an O_PATH fd to change > the behaviour of open_by_handle_at(). That seems very subtle. Is there > a good reason to do it that way instead of just declaring a new AT_* > flag for this? > Not sure if it is a good reason, but open_by_handle_at() has an O_ flags argument, not an AT_ flags argument... If my hack API is not acceptable then we will need to add open_by_handle_at2(), with struct open_how argument or something. > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > index 956d9b25d4f7..1fabfb79fd55 100644 > > --- a/fs/fhandle.c > > +++ b/fs/fhandle.c > > @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name, > > return err; > > } > > > > -static int get_path_from_fd(int fd, struct path *root) > > +enum handle_to_path_flags { > > + HANDLE_CHECK_PERMS = (1 << 0), > > + HANDLE_CHECK_SUBTREE = (1 << 1), > > +}; > > + > > +struct handle_to_path_ctx { > > + struct path root; > > + enum handle_to_path_flags flags; > > + unsigned int fh_flags; > > + unsigned int o_flags; > > +}; > > + > > +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx) > > { > > if (fd == AT_FDCWD) { > > struct fs_struct *fs = current->fs; > > spin_lock(&fs->lock); > > - *root = fs->pwd; > > - path_get(root); > > + ctx->root = fs->pwd; > > + path_get(&ctx->root); > > spin_unlock(&fs->lock); > > } else { > > - struct fd f = fdget(fd); > > + struct fd f = fdget_raw(fd); > > if (!f.file) > > return -EBADF; > > - *root = f.file->f_path; > > - path_get(root); > > + ctx->root = f.file->f_path; > > + path_get(&ctx->root); > > + /* > > + * Use O_PATH mount fd and requested O_PATH fd as a hint for > > + * decoding an fd with connected path, that is accessible from > > + * the mount fd path. > > + */ > > + if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH) > > + ctx->flags |= HANDLE_CHECK_SUBTREE; > > fdput(f); > > } > > > > return 0; > > } > > > > -enum handle_to_path_flags { > > - HANDLE_CHECK_PERMS = (1 << 0), > > - HANDLE_CHECK_SUBTREE = (1 << 1), > > -}; > > - > > -struct handle_to_path_ctx { > > - struct path root; > > - enum handle_to_path_flags flags; > > - unsigned int fh_flags; > > -}; > > - > > static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > > { > > struct handle_to_path_ctx *ctx = context; > > @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > > > > if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root) > > retval = 1; > > - WARN_ON_ONCE(d != root && d != root->d_sb->s_root); > > + /* > > + * exportfs_decode_fh_raw() does not call acceptable() callback with > > + * a disconnected directory dentry, so we should have reached either > > + * mount fd directory or sb root. > > + */ > > + if (ctx->fh_flags & EXPORT_FH_DIR_ONLY) > > + WARN_ON_ONCE(d != root && d != root->d_sb->s_root); > > I don't quite understand why the above change is necessary. Can you > explain why we need to limit this only to the case where > EXPORT_FH_DIR_ONLY is set? > With EXPORT_FH_DIR_ONLY, exportfs_decode_fh_raw() should only be calling acceptable() with a connected directory dentry. Until this patch, vfs_dentry_acceptable() would only be called with EXPORT_FH_DIR_ONLY so the WARN_ON could be unconditional. After this patch, vfs_dentry_acceptable() could also be called for a disconnected non-dir dentry and then it should just fail to accept the dentry, but should not WARN_ON. Thanks for the review! Amir.
On Fri, 2024-09-20 at 18:38 +0200, Amir Goldstein wrote: > On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote: > > > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2). > > > This was not allowed before, so we use it to enable a new API for > > > decoding "connectable" file handles that were created using the > > > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2). > > > > > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested, > > > use that as a hint to try to decode a "connected" fd with known path, > > > which is accessible (to capable user) from mount fd path. > > > > > > Note that this does not check if the path is accessible to the calling > > > user, just that it is accessible wrt the mount namesapce, so if there > > > is no "connected" alias, or if parts of the path are hidden in the > > > mount namespace, open_by_handle_at(2) will return -ESTALE. > > > > > > Note that the file handles used to decode a "connected" fd do not have > > > to be encoded with the AT_HANDLE_CONNECTABLE flag. Specifically, > > > directory file handles are always "connectable", regardless of using > > > the AT_HANDLE_CONNECTABLE flag. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/fhandle.c | 61 +++++++++++++++++++++++++++++++--------------------- > > > 1 file changed, 37 insertions(+), 24 deletions(-) > > > > > > > The mountfd is only used to get a path, so I don't see a problem with > > allowing that to be an O_PATH fd. > > > > I'm less keen on using the fact that mountfd is an O_PATH fd to change > > the behaviour of open_by_handle_at(). That seems very subtle. Is there > > a good reason to do it that way instead of just declaring a new AT_* > > flag for this? > > > > Not sure if it is a good reason, but open_by_handle_at() has an O_ flags > argument, not an AT_ flags argument... > > If my hack API is not acceptable then we will need to add > open_by_handle_at2(), with struct open_how argument or something. > Oh right, I forgot that open_by_handle_at doesn't take AT_* flags. A new syscall may be best then. I can see a couple of other potential approaches: 1/ You could add a new fcntl() cmd that puts the mountfd into a "connectable filehandles" mode. The downside there is that it'd take 2 syscalls to do your open. 2/ You could add flags to open_how that make openat2() behave like open_by_handle_at(). Add a flag that allows "pathname" to point to a filehandle instead, and a second flag that indicates that the fh is connectable. Both of those are pretty hacky though. > > > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > > index 956d9b25d4f7..1fabfb79fd55 100644 > > > --- a/fs/fhandle.c > > > +++ b/fs/fhandle.c > > > @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name, > > > return err; > > > } > > > > > > -static int get_path_from_fd(int fd, struct path *root) > > > +enum handle_to_path_flags { > > > + HANDLE_CHECK_PERMS = (1 << 0), > > > + HANDLE_CHECK_SUBTREE = (1 << 1), > > > +}; > > > + > > > +struct handle_to_path_ctx { > > > + struct path root; > > > + enum handle_to_path_flags flags; > > > + unsigned int fh_flags; > > > + unsigned int o_flags; > > > +}; > > > + > > > +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx) > > > { > > > if (fd == AT_FDCWD) { > > > struct fs_struct *fs = current->fs; > > > spin_lock(&fs->lock); > > > - *root = fs->pwd; > > > - path_get(root); > > > + ctx->root = fs->pwd; > > > + path_get(&ctx->root); > > > spin_unlock(&fs->lock); > > > } else { > > > - struct fd f = fdget(fd); > > > + struct fd f = fdget_raw(fd); > > > if (!f.file) > > > return -EBADF; > > > - *root = f.file->f_path; > > > - path_get(root); > > > + ctx->root = f.file->f_path; > > > + path_get(&ctx->root); > > > + /* > > > + * Use O_PATH mount fd and requested O_PATH fd as a hint for > > > + * decoding an fd with connected path, that is accessible from > > > + * the mount fd path. > > > + */ > > > + if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH) > > > + ctx->flags |= HANDLE_CHECK_SUBTREE; > > > fdput(f); > > > } > > > > > > return 0; > > > } > > > > > > -enum handle_to_path_flags { > > > - HANDLE_CHECK_PERMS = (1 << 0), > > > - HANDLE_CHECK_SUBTREE = (1 << 1), > > > -}; > > > - > > > -struct handle_to_path_ctx { > > > - struct path root; > > > - enum handle_to_path_flags flags; > > > - unsigned int fh_flags; > > > -}; > > > - > > > static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > > > { > > > struct handle_to_path_ctx *ctx = context; > > > @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > > > > > > if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root) > > > retval = 1; > > > - WARN_ON_ONCE(d != root && d != root->d_sb->s_root); > > > + /* > > > + * exportfs_decode_fh_raw() does not call acceptable() callback with > > > + * a disconnected directory dentry, so we should have reached either > > > + * mount fd directory or sb root. > > > + */ > > > + if (ctx->fh_flags & EXPORT_FH_DIR_ONLY) > > > + WARN_ON_ONCE(d != root && d != root->d_sb->s_root); > > > > I don't quite understand why the above change is necessary. Can you > > explain why we need to limit this only to the case where > > EXPORT_FH_DIR_ONLY is set? > > > > With EXPORT_FH_DIR_ONLY, exportfs_decode_fh_raw() should > only be calling acceptable() with a connected directory dentry. > > Until this patch, vfs_dentry_acceptable() would only be called with > EXPORT_FH_DIR_ONLY so the WARN_ON could be unconditional. > > After this patch, vfs_dentry_acceptable() could also be called for > a disconnected non-dir dentry and then it should just fail to > accept the dentry, but should not WARN_ON. > > Thanks for the review! > Amir.
On 2024-09-21, Jeff Layton <jlayton@kernel.org> wrote: > On Fri, 2024-09-20 at 18:38 +0200, Amir Goldstein wrote: > > On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote: > > > > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2). > > > > This was not allowed before, so we use it to enable a new API for > > > > decoding "connectable" file handles that were created using the > > > > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2). > > > > > > > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested, > > > > use that as a hint to try to decode a "connected" fd with known path, > > > > which is accessible (to capable user) from mount fd path. > > > > > > > > Note that this does not check if the path is accessible to the calling > > > > user, just that it is accessible wrt the mount namesapce, so if there > > > > is no "connected" alias, or if parts of the path are hidden in the > > > > mount namespace, open_by_handle_at(2) will return -ESTALE. > > > > > > > > Note that the file handles used to decode a "connected" fd do not have > > > > to be encoded with the AT_HANDLE_CONNECTABLE flag. Specifically, > > > > directory file handles are always "connectable", regardless of using > > > > the AT_HANDLE_CONNECTABLE flag. > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > fs/fhandle.c | 61 +++++++++++++++++++++++++++++++--------------------- > > > > 1 file changed, 37 insertions(+), 24 deletions(-) > > > > > > > > > > The mountfd is only used to get a path, so I don't see a problem with > > > allowing that to be an O_PATH fd. > > > > > > I'm less keen on using the fact that mountfd is an O_PATH fd to change > > > the behaviour of open_by_handle_at(). That seems very subtle. Is there > > > a good reason to do it that way instead of just declaring a new AT_* > > > flag for this? > > > > > > > Not sure if it is a good reason, but open_by_handle_at() has an O_ flags > > argument, not an AT_ flags argument... > > > > If my hack API is not acceptable then we will need to add > > open_by_handle_at2(), with struct open_how argument or something. > > > > Oh right, I forgot that open_by_handle_at doesn't take AT_* flags. > A new syscall may be best then. > > I can see a couple of other potential approaches: > > 1/ You could add a new fcntl() cmd that puts the mountfd into a > "connectable filehandles" mode. The downside there is that it'd take 2 > syscalls to do your open. > > 2/ You could add flags to open_how that make openat2() behave like > open_by_handle_at(). Add a flag that allows "pathname" to point to a > filehandle instead, and a second flag that indicates that the fh is > connectable. Hackiness aside, the latter is not workable until we can filter extensible structs with seccomp. Container runtimes all block open_by_handle_at(2) because it can be used to break out of non-userns containers. > Both of those are pretty hacky though. > > > > > > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > > > index 956d9b25d4f7..1fabfb79fd55 100644 > > > > --- a/fs/fhandle.c > > > > +++ b/fs/fhandle.c > > > > @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name, > > > > return err; > > > > } > > > > > > > > -static int get_path_from_fd(int fd, struct path *root) > > > > +enum handle_to_path_flags { > > > > + HANDLE_CHECK_PERMS = (1 << 0), > > > > + HANDLE_CHECK_SUBTREE = (1 << 1), > > > > +}; > > > > + > > > > +struct handle_to_path_ctx { > > > > + struct path root; > > > > + enum handle_to_path_flags flags; > > > > + unsigned int fh_flags; > > > > + unsigned int o_flags; > > > > +}; > > > > + > > > > +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx) > > > > { > > > > if (fd == AT_FDCWD) { > > > > struct fs_struct *fs = current->fs; > > > > spin_lock(&fs->lock); > > > > - *root = fs->pwd; > > > > - path_get(root); > > > > + ctx->root = fs->pwd; > > > > + path_get(&ctx->root); > > > > spin_unlock(&fs->lock); > > > > } else { > > > > - struct fd f = fdget(fd); > > > > + struct fd f = fdget_raw(fd); > > > > if (!f.file) > > > > return -EBADF; > > > > - *root = f.file->f_path; > > > > - path_get(root); > > > > + ctx->root = f.file->f_path; > > > > + path_get(&ctx->root); > > > > + /* > > > > + * Use O_PATH mount fd and requested O_PATH fd as a hint for > > > > + * decoding an fd with connected path, that is accessible from > > > > + * the mount fd path. > > > > + */ > > > > + if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH) > > > > + ctx->flags |= HANDLE_CHECK_SUBTREE; > > > > fdput(f); > > > > } > > > > > > > > return 0; > > > > } > > > > > > > > -enum handle_to_path_flags { > > > > - HANDLE_CHECK_PERMS = (1 << 0), > > > > - HANDLE_CHECK_SUBTREE = (1 << 1), > > > > -}; > > > > - > > > > -struct handle_to_path_ctx { > > > > - struct path root; > > > > - enum handle_to_path_flags flags; > > > > - unsigned int fh_flags; > > > > -}; > > > > - > > > > static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > > > > { > > > > struct handle_to_path_ctx *ctx = context; > > > > @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > > > > > > > > if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root) > > > > retval = 1; > > > > - WARN_ON_ONCE(d != root && d != root->d_sb->s_root); > > > > + /* > > > > + * exportfs_decode_fh_raw() does not call acceptable() callback with > > > > + * a disconnected directory dentry, so we should have reached either > > > > + * mount fd directory or sb root. > > > > + */ > > > > + if (ctx->fh_flags & EXPORT_FH_DIR_ONLY) > > > > + WARN_ON_ONCE(d != root && d != root->d_sb->s_root); > > > > > > I don't quite understand why the above change is necessary. Can you > > > explain why we need to limit this only to the case where > > > EXPORT_FH_DIR_ONLY is set? > > > > > > > With EXPORT_FH_DIR_ONLY, exportfs_decode_fh_raw() should > > only be calling acceptable() with a connected directory dentry. > > > > Until this patch, vfs_dentry_acceptable() would only be called with > > EXPORT_FH_DIR_ONLY so the WARN_ON could be unconditional. > > > > After this patch, vfs_dentry_acceptable() could also be called for > > a disconnected non-dir dentry and then it should just fail to > > accept the dentry, but should not WARN_ON. > > > > Thanks for the review! > > Amir. > > -- > Jeff Layton <jlayton@kernel.org>
On 2024-09-20, Amir Goldstein <amir73il@gmail.com> wrote: > On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote: > > > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2). > > > This was not allowed before, so we use it to enable a new API for > > > decoding "connectable" file handles that were created using the > > > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2). > > > > > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested, > > > use that as a hint to try to decode a "connected" fd with known path, > > > which is accessible (to capable user) from mount fd path. > > > > > > Note that this does not check if the path is accessible to the calling > > > user, just that it is accessible wrt the mount namesapce, so if there > > > is no "connected" alias, or if parts of the path are hidden in the > > > mount namespace, open_by_handle_at(2) will return -ESTALE. > > > > > > Note that the file handles used to decode a "connected" fd do not have > > > to be encoded with the AT_HANDLE_CONNECTABLE flag. Specifically, > > > directory file handles are always "connectable", regardless of using > > > the AT_HANDLE_CONNECTABLE flag. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/fhandle.c | 61 +++++++++++++++++++++++++++++++--------------------- > > > 1 file changed, 37 insertions(+), 24 deletions(-) > > > > > > > The mountfd is only used to get a path, so I don't see a problem with > > allowing that to be an O_PATH fd. > > > > I'm less keen on using the fact that mountfd is an O_PATH fd to change > > the behaviour of open_by_handle_at(). That seems very subtle. Is there > > a good reason to do it that way instead of just declaring a new AT_* > > flag for this? > > > > Not sure if it is a good reason, but open_by_handle_at() has an O_ flags > argument, not an AT_ flags argument... > > If my hack API is not acceptable then we will need to add > open_by_handle_at2(), with struct open_how argument or something. Does all of the stuff in openat2 make sense for open_by_handle_at? What should RESOLVE_* or mode do? There's no standard namei lookup done. > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > > index 956d9b25d4f7..1fabfb79fd55 100644 > > > --- a/fs/fhandle.c > > > +++ b/fs/fhandle.c > > > @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name, > > > return err; > > > } > > > > > > -static int get_path_from_fd(int fd, struct path *root) > > > +enum handle_to_path_flags { > > > + HANDLE_CHECK_PERMS = (1 << 0), > > > + HANDLE_CHECK_SUBTREE = (1 << 1), > > > +}; > > > + > > > +struct handle_to_path_ctx { > > > + struct path root; > > > + enum handle_to_path_flags flags; > > > + unsigned int fh_flags; > > > + unsigned int o_flags; > > > +}; > > > + > > > +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx) > > > { > > > if (fd == AT_FDCWD) { > > > struct fs_struct *fs = current->fs; > > > spin_lock(&fs->lock); > > > - *root = fs->pwd; > > > - path_get(root); > > > + ctx->root = fs->pwd; > > > + path_get(&ctx->root); > > > spin_unlock(&fs->lock); > > > } else { > > > - struct fd f = fdget(fd); > > > + struct fd f = fdget_raw(fd); > > > if (!f.file) > > > return -EBADF; > > > - *root = f.file->f_path; > > > - path_get(root); > > > + ctx->root = f.file->f_path; > > > + path_get(&ctx->root); > > > + /* > > > + * Use O_PATH mount fd and requested O_PATH fd as a hint for > > > + * decoding an fd with connected path, that is accessible from > > > + * the mount fd path. > > > + */ > > > + if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH) > > > + ctx->flags |= HANDLE_CHECK_SUBTREE; > > > fdput(f); > > > } > > > > > > return 0; > > > } > > > > > > -enum handle_to_path_flags { > > > - HANDLE_CHECK_PERMS = (1 << 0), > > > - HANDLE_CHECK_SUBTREE = (1 << 1), > > > -}; > > > - > > > -struct handle_to_path_ctx { > > > - struct path root; > > > - enum handle_to_path_flags flags; > > > - unsigned int fh_flags; > > > -}; > > > - > > > static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > > > { > > > struct handle_to_path_ctx *ctx = context; > > > @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry) > > > > > > if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root) > > > retval = 1; > > > - WARN_ON_ONCE(d != root && d != root->d_sb->s_root); > > > + /* > > > + * exportfs_decode_fh_raw() does not call acceptable() callback with > > > + * a disconnected directory dentry, so we should have reached either > > > + * mount fd directory or sb root. > > > + */ > > > + if (ctx->fh_flags & EXPORT_FH_DIR_ONLY) > > > + WARN_ON_ONCE(d != root && d != root->d_sb->s_root); > > > > I don't quite understand why the above change is necessary. Can you > > explain why we need to limit this only to the case where > > EXPORT_FH_DIR_ONLY is set? > > > > With EXPORT_FH_DIR_ONLY, exportfs_decode_fh_raw() should > only be calling acceptable() with a connected directory dentry. > > Until this patch, vfs_dentry_acceptable() would only be called with > EXPORT_FH_DIR_ONLY so the WARN_ON could be unconditional. > > After this patch, vfs_dentry_acceptable() could also be called for > a disconnected non-dir dentry and then it should just fail to > accept the dentry, but should not WARN_ON. > > Thanks for the review! > Amir.
On Sat, Sep 21, 2024 at 7:33 AM Jeff Layton <jlayton@kernel.org> wrote: > > On Fri, 2024-09-20 at 18:38 +0200, Amir Goldstein wrote: > > On Fri, Sep 20, 2024 at 6:02 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote: > > > > Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2). > > > > This was not allowed before, so we use it to enable a new API for > > > > decoding "connectable" file handles that were created using the > > > > AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2). > > > > > > > > When mount fd is an O_PATH fd and decoding an O_PATH fd is requested, > > > > use that as a hint to try to decode a "connected" fd with known path, > > > > which is accessible (to capable user) from mount fd path. > > > > > > > > Note that this does not check if the path is accessible to the calling > > > > user, just that it is accessible wrt the mount namesapce, so if there > > > > is no "connected" alias, or if parts of the path are hidden in the > > > > mount namespace, open_by_handle_at(2) will return -ESTALE. > > > > > > > > Note that the file handles used to decode a "connected" fd do not have > > > > to be encoded with the AT_HANDLE_CONNECTABLE flag. Specifically, > > > > directory file handles are always "connectable", regardless of using > > > > the AT_HANDLE_CONNECTABLE flag. > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > fs/fhandle.c | 61 +++++++++++++++++++++++++++++++--------------------- > > > > 1 file changed, 37 insertions(+), 24 deletions(-) > > > > > > > > > > The mountfd is only used to get a path, so I don't see a problem with > > > allowing that to be an O_PATH fd. > > > > > > I'm less keen on using the fact that mountfd is an O_PATH fd to change > > > the behaviour of open_by_handle_at(). That seems very subtle. Is there > > > a good reason to do it that way instead of just declaring a new AT_* > > > flag for this? > > > > > > > Not sure if it is a good reason, but open_by_handle_at() has an O_ flags > > argument, not an AT_ flags argument... > > > > If my hack API is not acceptable then we will need to add > > open_by_handle_at2(), with struct open_how argument or something. > > > > Oh right, I forgot that open_by_handle_at doesn't take AT_* flags. > A new syscall may be best then. > > I can see a couple of other potential approaches: > > 1/ You could add a new fcntl() cmd that puts the mountfd into a > "connectable filehandles" mode. The downside there is that it'd take 2 > syscalls to do your open. > > 2/ You could add flags to open_how that make openat2() behave like > open_by_handle_at(). Add a flag that allows "pathname" to point to a > filehandle instead, and a second flag that indicates that the fh is > connectable. > > Both of those are pretty hacky though. > Yes, especially #2, very creative ;) But I had another less hacky idea. Instead of (ab)using a sycall flag to get a "connected" fd, encode an explicit "connectable" fh, something like this untested patch. WDYT? Thanks, Amir. --- a/fs/fhandle.c +++ b/fs/fhandle.c @@ -59,6 +59,7 @@ static long do_sys_name_to_handle(const struct path *path, handle_bytes = handle_dwords * sizeof(u32); handle->handle_bytes = handle_bytes; if ((handle->handle_bytes > f_handle.handle_bytes) || + WARN_ON_ONCE(retval > FILEID_INVALID) || (retval == FILEID_INVALID) || (retval < 0)) { /* As per old exportfs_encode_fh documentation * we could return ENOSPC to indicate overflow @@ -72,8 +73,21 @@ static long do_sys_name_to_handle(const struct path *path, * non variable part of the file_handle */ handle_bytes = 0; - } else + } else { + /* + * When asked to encode a connectable file handle, encode this + * property in the file handle itself, so we know how to decode it. + * For sanity, also encode in the file handle if the encoded object + * is a directory, because connectable directory file handles do not + * encode the parent. + */ + if (fh_flags & EXPORT_FH_CONNECTABLE) { + if (d_is_dir(path->dentry)) + fh_flags |= EXPORT_FH_DIR_ONLY; + handle->handle_flags = fh_flags; + } retval = 0; + } ... @@ -336,6 +350,19 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh, retval = -EINVAL; goto out_path; } + if (f_handle.handle_flags & ~EXPORT_FH_USER_FLAGS) { + retval = -EINVAL; + goto out_path; + } + /* + * If handle was encoded with AT_HANDLE_CONNECTABLE, verify that we + * are decoding an fd with connected path, which is accessible from + * the mount fd path. + */ + ctx.fh_flags |= f_handle.handle_flags; + if (ctx.fh_flags & EXPORT_FH_CONNECTABLE) + ctx.flags |= HANDLE_CHECK_SUBTREE; + handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes), GFP_KERNEL); ... --- a/include/linux/exportfs.h +++ b/include/linux/exportfs.h @@ -159,6 +159,8 @@ struct fid { #define EXPORT_FH_CONNECTABLE 0x1 /* Encode file handle with parent */ #define EXPORT_FH_FID 0x2 /* File handle may be non-decodeable */ #define EXPORT_FH_DIR_ONLY 0x4 /* Only decode file handle for a directory */ +/* Flags allowed in encoded handle_flags that is exported to user */ +#define EXPORT_FH_USER_FLAGS (EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY) ... --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1071,7 +1071,8 @@ struct file { struct file_handle { __u32 handle_bytes; - int handle_type; + int handle_type:16; + int handle_flags:16; /* file identifier */ unsigned char f_handle[] __counted_by(handle_bytes); };
diff --git a/fs/fhandle.c b/fs/fhandle.c index 956d9b25d4f7..1fabfb79fd55 100644 --- a/fs/fhandle.c +++ b/fs/fhandle.c @@ -146,37 +146,45 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name, return err; } -static int get_path_from_fd(int fd, struct path *root) +enum handle_to_path_flags { + HANDLE_CHECK_PERMS = (1 << 0), + HANDLE_CHECK_SUBTREE = (1 << 1), +}; + +struct handle_to_path_ctx { + struct path root; + enum handle_to_path_flags flags; + unsigned int fh_flags; + unsigned int o_flags; +}; + +static int get_path_from_fd(int fd, struct handle_to_path_ctx *ctx) { if (fd == AT_FDCWD) { struct fs_struct *fs = current->fs; spin_lock(&fs->lock); - *root = fs->pwd; - path_get(root); + ctx->root = fs->pwd; + path_get(&ctx->root); spin_unlock(&fs->lock); } else { - struct fd f = fdget(fd); + struct fd f = fdget_raw(fd); if (!f.file) return -EBADF; - *root = f.file->f_path; - path_get(root); + ctx->root = f.file->f_path; + path_get(&ctx->root); + /* + * Use O_PATH mount fd and requested O_PATH fd as a hint for + * decoding an fd with connected path, that is accessible from + * the mount fd path. + */ + if (ctx->o_flags & O_PATH && f.file->f_mode & FMODE_PATH) + ctx->flags |= HANDLE_CHECK_SUBTREE; fdput(f); } return 0; } -enum handle_to_path_flags { - HANDLE_CHECK_PERMS = (1 << 0), - HANDLE_CHECK_SUBTREE = (1 << 1), -}; - -struct handle_to_path_ctx { - struct path root; - enum handle_to_path_flags flags; - unsigned int fh_flags; -}; - static int vfs_dentry_acceptable(void *context, struct dentry *dentry) { struct handle_to_path_ctx *ctx = context; @@ -224,7 +232,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry) if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root) retval = 1; - WARN_ON_ONCE(d != root && d != root->d_sb->s_root); + /* + * exportfs_decode_fh_raw() does not call acceptable() callback with + * a disconnected directory dentry, so we should have reached either + * mount fd directory or sb root. + */ + if (ctx->fh_flags & EXPORT_FH_DIR_ONLY) + WARN_ON_ONCE(d != root && d != root->d_sb->s_root); dput(d); return retval; } @@ -265,8 +279,7 @@ static int do_handle_to_path(struct file_handle *handle, struct path *path, * filesystem but that only applies to procfs and sysfs neither of which * support decoding file handles. */ -static inline bool may_decode_fh(struct handle_to_path_ctx *ctx, - unsigned int o_flags) +static inline bool may_decode_fh(struct handle_to_path_ctx *ctx) { struct path *root = &ctx->root; @@ -276,7 +289,7 @@ static inline bool may_decode_fh(struct handle_to_path_ctx *ctx, * * There's only one dentry for each directory inode (VFS rule)... */ - if (!(o_flags & O_DIRECTORY)) + if (!(ctx->o_flags & O_DIRECTORY)) return false; if (ns_capable(root->mnt->mnt_sb->s_user_ns, CAP_SYS_ADMIN)) @@ -303,13 +316,13 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh, int retval = 0; struct file_handle f_handle; struct file_handle *handle = NULL; - struct handle_to_path_ctx ctx = {}; + struct handle_to_path_ctx ctx = { .o_flags = o_flags }; - retval = get_path_from_fd(mountdirfd, &ctx.root); + retval = get_path_from_fd(mountdirfd, &ctx); if (retval) goto out_err; - if (!capable(CAP_DAC_READ_SEARCH) && !may_decode_fh(&ctx, o_flags)) { + if (!capable(CAP_DAC_READ_SEARCH) && !may_decode_fh(&ctx)) { retval = -EPERM; goto out_path; }
Allow using an O_PATH fd as mount fd argument of open_by_handle_at(2). This was not allowed before, so we use it to enable a new API for decoding "connectable" file handles that were created using the AT_HANDLE_CONNECTABLE flag to name_to_handle_at(2). When mount fd is an O_PATH fd and decoding an O_PATH fd is requested, use that as a hint to try to decode a "connected" fd with known path, which is accessible (to capable user) from mount fd path. Note that this does not check if the path is accessible to the calling user, just that it is accessible wrt the mount namesapce, so if there is no "connected" alias, or if parts of the path are hidden in the mount namespace, open_by_handle_at(2) will return -ESTALE. Note that the file handles used to decode a "connected" fd do not have to be encoded with the AT_HANDLE_CONNECTABLE flag. Specifically, directory file handles are always "connectable", regardless of using the AT_HANDLE_CONNECTABLE flag. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/fhandle.c | 61 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 24 deletions(-)