On Sat, Sep 21, 2024 at 7:33 AM 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.
>
> 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);
};
