Re: [RFC PATCH 2/2] fs: open_by_handle_at() support for decoding connectable file handles

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024-09-21, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxx>

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux