| Message ID | 20240923082829.1910210-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | API for exporting connectable file handles to userspace | expand |
> open_by_handle_at(2) does not have AT_ flags argument, but also, I find > it more useful API that encoding a connectable file handle can mandate > the resolving of a connected fd, without having to opt-in for a > connected fd independently. This seems the best option to me too if this api is to be added.
On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > > it more useful API that encoding a connectable file handle can mandate > > the resolving of a connected fd, without having to opt-in for a > > connected fd independently. > > This seems the best option to me too if this api is to be added. Thanks. Jeff, Chuck, Any thoughts on this? Thanks, Amir.
> On Oct 7, 2024, at 11:26 AM, Amir Goldstein <amir73il@gmail.com> wrote: > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: >> >>> open_by_handle_at(2) does not have AT_ flags argument, but also, I find >>> it more useful API that encoding a connectable file handle can mandate >>> the resolving of a connected fd, without having to opt-in for a >>> connected fd independently. >> >> This seems the best option to me too if this api is to be added. > > Thanks. > > Jeff, Chuck, > > Any thoughts on this? I don't have anything clever to add. -- Chuck Lever
On Mon, Oct 7, 2024 at 8:09 PM Chuck Lever III <chuck.lever@oracle.com> wrote: > > > > > On Oct 7, 2024, at 11:26 AM, Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: > >> > >>> open_by_handle_at(2) does not have AT_ flags argument, but also, I find > >>> it more useful API that encoding a connectable file handle can mandate > >>> the resolving of a connected fd, without having to opt-in for a > >>> connected fd independently. > >> > >> This seems the best option to me too if this api is to be added. > > > > Thanks. > > > > Jeff, Chuck, > > > > Any thoughts on this? > > I don't have anything clever to add. > Sometimes that's a good thing ;) FYI, I wrote a draft for the would be man page update to go with the proposed flag: name_to_handle_at() The name_to_handle_at() system call returns a file handle and a mount ID corresponding to the file specified by the dirfd and pathname arguments. ... When flags contain the AT_HANDLE_FID (since Linux 6.5) flag, the caller indicates that the returned file_handle is needed to identify the filesystem object, and not for opening the file later, so it should be expected that a subsequent call to open_by_handle_at() with the returned file_handle may fail. + When flags contain the AT_HANDLE_CONNECTABLE (since Linux 6.x) flag, + the caller indicates that the returned file_handle is needed to open a file with known path later, + so it should be expected that a subsequent call to open_by_handle_at() + with the returned file_handle may fail if the file was moved, but otherwise, + the path of the opened file is expected to be visible from the /proc/pid/fd/* magic link. + This flag can not be used in combination with the flags AT_HANDLE_FID, and AT_EMPTY_PATH. ... ESTALE The specified handle is not valid for opening a file. This error will occur if, for example, the file has been deleted. This error can also occur if the handle was acquired using the AT_HANDLE_FID flag and the filesystem does not support open_by_handle_at(). + This error can also occur if the handle was acquired using the AT_HANDLE_CONNECTABLE + flag and the file was moved to a different parent. -- Apropos man page, Aleksa, Are you planning to post a man page patch for AT_HANDLE_MNT_ID_UNIQUE? I realize that there is no man page maintainer at the moment, but I myself would love to have this patch in my man-page updates queue, until a man-page maintainer shows up, because, well, my update to AT_HANDLE_CONNECTABLE depends on it and so will my changes to fanotify to support mount/unmount events if/when I will get to them. Thanks, Amir.
On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote: > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > > > it more useful API that encoding a connectable file handle can mandate > > > the resolving of a connected fd, without having to opt-in for a > > > connected fd independently. > > > > This seems the best option to me too if this api is to be added. > > Thanks. > > Jeff, Chuck, > > Any thoughts on this? > Sorry for the delay. I think encoding the new flag into the fh itself is a reasonable approach. I'm less thrilled with using bitfields for this, just because I have a general dislike of them, and they aren't implemented the same way on all arches. Would it break ABI if we just turned the handle_type int into two uint16_t's instead?
On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote: > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > > > > it more useful API that encoding a connectable file handle can mandate > > > > the resolving of a connected fd, without having to opt-in for a > > > > connected fd independently. > > > > > > This seems the best option to me too if this api is to be added. > > > > Thanks. > > > > Jeff, Chuck, > > > > Any thoughts on this? > > > > Sorry for the delay. I think encoding the new flag into the fh itself > is a reasonable approach. > Adding Jan. Sorry I forgot to CC you on the patches, but struct file_handle is officially a part of fanotify ABI, so your ACK is also needed on this change. > I'm less thrilled with using bitfields for this, just because I have a > general dislike of them, and they aren't implemented the same way on > all arches. Would it break ABI if we just turned the handle_type int > into two uint16_t's instead? I think it will because this will not be backward compat on LE arch: struct file_handle { __u32 handle_bytes; - int handle_type; + __u16 handle_type; + __u16 handle_flags; /* file identifier */ unsigned char f_handle[] __counted_by(handle_bytes); }; But I can also do without the bitfileds, maybe it's better this way. See diff from v2: diff --git a/fs/fhandle.c b/fs/fhandle.c index 4ce4ffddec62..64d44fc61d43 100644 --- a/fs/fhandle.c +++ b/fs/fhandle.c @@ -87,9 +87,9 @@ static long do_sys_name_to_handle(const struct path *path, * decoding connectable non-directory file handles. */ if (fh_flags & EXPORT_FH_CONNECTABLE) { + handle->handle_type |= FILEID_IS_CONNECTABLE; if (d_is_dir(path->dentry)) - fh_flags |= EXPORT_FH_DIR_ONLY; - handle->handle_flags = fh_flags; + fh_flags |= FILEID_IS_DIR; } retval = 0; } @@ -352,7 +352,7 @@ 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) { + if (!FILEID_USER_TYPE_IS_VALID(f_handle.handle_type)) { retval = -EINVAL; goto out_path; } @@ -377,10 +377,14 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh, * 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) + if (f_handle.handle_type & FILEID_IS_CONNECTABLE) { + ctx.fh_flags |= EXPORT_FH_CONNECTABLE; ctx.flags |= HANDLE_CHECK_SUBTREE; - + if (f_handle.handle_type & FILEID_IS_DIR) + ctx.fh_flags |= EXPORT_FH_DIR_ONLY; + } + /* Filesystem code should not be exposed to user flags */ + handle->handle_type &= ~FILEID_USER_FLAGS_MASK; retval = do_handle_to_path(handle, path, &ctx); out_handle: diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h index 96b62e502f71..3e60bac74fa3 100644 --- a/include/linux/exportfs.h +++ b/include/linux/exportfs.h @@ -159,8 +159,17 @@ 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) + +/* Flags supported in encoded handle_type that is exported to user */ +#define FILEID_USER_FLAGS_MASK 0xffff0000 +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) + +#define FILEID_IS_CONNECTABLE 0x10000 +#define FILEID_IS_DIR 0x40000 +#define FILEID_VALID_USER_FLAGS (FILEID_IS_CONNECTABLE | FILEID_IS_DIR) + +#define FILEID_USER_TYPE_IS_VALID(type) \ + (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS) /** * struct export_operations - for nfsd to communicate with file systems diff --git a/include/linux/fs.h b/include/linux/fs.h index cca7e575d1f8..6329fec40872 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1071,8 +1071,7 @@ struct file { struct file_handle { __u32 handle_bytes; - int handle_type:16; - int handle_flags:16; + int handle_type; /* file identifier */ unsigned char f_handle[] __counted_by(handle_bytes); };
On Tue, 2024-10-08 at 15:11 +0200, Amir Goldstein wrote: > On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote: > > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > > > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > > > > > it more useful API that encoding a connectable file handle can mandate > > > > > the resolving of a connected fd, without having to opt-in for a > > > > > connected fd independently. > > > > > > > > This seems the best option to me too if this api is to be added. > > > > > > Thanks. > > > > > > Jeff, Chuck, > > > > > > Any thoughts on this? > > > > > > > Sorry for the delay. I think encoding the new flag into the fh itself > > is a reasonable approach. > > > > Adding Jan. > Sorry I forgot to CC you on the patches, but struct file_handle is officially > a part of fanotify ABI, so your ACK is also needed on this change. > > > I'm less thrilled with using bitfields for this, just because I have a > > general dislike of them, and they aren't implemented the same way on > > all arches. Would it break ABI if we just turned the handle_type int > > into two uint16_t's instead? > > I think it will because this will not be backward compat on LE arch: > > struct file_handle { > __u32 handle_bytes; > - int handle_type; > + __u16 handle_type; > + __u16 handle_flags; > /* file identifier */ > unsigned char f_handle[] __counted_by(handle_bytes); > }; > Ok, good point. > But I can also do without the bitfileds, maybe it's better this way. > See diff from v2: > > diff --git a/fs/fhandle.c b/fs/fhandle.c > index 4ce4ffddec62..64d44fc61d43 100644 > --- a/fs/fhandle.c > +++ b/fs/fhandle.c > @@ -87,9 +87,9 @@ static long do_sys_name_to_handle(const struct path *path, > * decoding connectable non-directory file handles. > */ > if (fh_flags & EXPORT_FH_CONNECTABLE) { > + handle->handle_type |= FILEID_IS_CONNECTABLE; > if (d_is_dir(path->dentry)) > - fh_flags |= EXPORT_FH_DIR_ONLY; > - handle->handle_flags = fh_flags; > + fh_flags |= FILEID_IS_DIR; > } > retval = 0; > } > @@ -352,7 +352,7 @@ 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) { > + if (!FILEID_USER_TYPE_IS_VALID(f_handle.handle_type)) { > retval = -EINVAL; > goto out_path; > } > @@ -377,10 +377,14 @@ static int handle_to_path(int mountdirfd, struct > file_handle __user *ufh, > * 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) > + if (f_handle.handle_type & FILEID_IS_CONNECTABLE) { > + ctx.fh_flags |= EXPORT_FH_CONNECTABLE; > ctx.flags |= HANDLE_CHECK_SUBTREE; > - > + if (f_handle.handle_type & FILEID_IS_DIR) > + ctx.fh_flags |= EXPORT_FH_DIR_ONLY; > + } > + /* Filesystem code should not be exposed to user flags */ > + handle->handle_type &= ~FILEID_USER_FLAGS_MASK; > retval = do_handle_to_path(handle, path, &ctx); > > out_handle: > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > index 96b62e502f71..3e60bac74fa3 100644 > --- a/include/linux/exportfs.h > +++ b/include/linux/exportfs.h > @@ -159,8 +159,17 @@ 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) Maybe add a nice comment here about how the handle_type word is partitioned? > + > +/* Flags supported in encoded handle_type that is exported to user */ > +#define FILEID_USER_FLAGS_MASK 0xffff0000 > +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) > + > +#define FILEID_IS_CONNECTABLE 0x10000 > +#define FILEID_IS_DIR 0x40000 > +#define FILEID_VALID_USER_FLAGS (FILEID_IS_CONNECTABLE | FILEID_IS_DIR) > + > +#define FILEID_USER_TYPE_IS_VALID(type) \ > + (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS) > > /** > * struct export_operations - for nfsd to communicate with file systems > diff --git a/include/linux/fs.h b/include/linux/fs.h > index cca7e575d1f8..6329fec40872 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1071,8 +1071,7 @@ struct file { > > struct file_handle { > __u32 handle_bytes; > - int handle_type:16; > - int handle_flags:16; > + int handle_type; > /* file identifier */ > unsigned char f_handle[] __counted_by(handle_bytes); > }; I like that better than bitfields, fwiw.
On Tue, Oct 8, 2024 at 3:43 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Tue, 2024-10-08 at 15:11 +0200, Amir Goldstein wrote: > > On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote: > > > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > > > > > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > > > > > > it more useful API that encoding a connectable file handle can mandate > > > > > > the resolving of a connected fd, without having to opt-in for a > > > > > > connected fd independently. > > > > > > > > > > This seems the best option to me too if this api is to be added. > > > > > > > > Thanks. > > > > > > > > Jeff, Chuck, > > > > > > > > Any thoughts on this? > > > > > > > > > > Sorry for the delay. I think encoding the new flag into the fh itself > > > is a reasonable approach. > > > > > > > Adding Jan. > > Sorry I forgot to CC you on the patches, but struct file_handle is officially > > a part of fanotify ABI, so your ACK is also needed on this change. > > > > > I'm less thrilled with using bitfields for this, just because I have a > > > general dislike of them, and they aren't implemented the same way on > > > all arches. Would it break ABI if we just turned the handle_type int > > > into two uint16_t's instead? > > > > I think it will because this will not be backward compat on LE arch: > > > > struct file_handle { > > __u32 handle_bytes; > > - int handle_type; > > + __u16 handle_type; > > + __u16 handle_flags; > > /* file identifier */ > > unsigned char f_handle[] __counted_by(handle_bytes); > > }; > > > > Ok, good point. > > > But I can also do without the bitfileds, maybe it's better this way. > > See diff from v2: > > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > index 4ce4ffddec62..64d44fc61d43 100644 > > --- a/fs/fhandle.c > > +++ b/fs/fhandle.c > > @@ -87,9 +87,9 @@ static long do_sys_name_to_handle(const struct path *path, > > * decoding connectable non-directory file handles. > > */ > > if (fh_flags & EXPORT_FH_CONNECTABLE) { > > + handle->handle_type |= FILEID_IS_CONNECTABLE; > > if (d_is_dir(path->dentry)) > > - fh_flags |= EXPORT_FH_DIR_ONLY; > > - handle->handle_flags = fh_flags; > > + fh_flags |= FILEID_IS_DIR; > > } > > retval = 0; > > } > > @@ -352,7 +352,7 @@ 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) { > > + if (!FILEID_USER_TYPE_IS_VALID(f_handle.handle_type)) { > > retval = -EINVAL; > > goto out_path; > > } > > @@ -377,10 +377,14 @@ static int handle_to_path(int mountdirfd, struct > > file_handle __user *ufh, > > * 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) > > + if (f_handle.handle_type & FILEID_IS_CONNECTABLE) { > > + ctx.fh_flags |= EXPORT_FH_CONNECTABLE; > > ctx.flags |= HANDLE_CHECK_SUBTREE; > > - > > + if (f_handle.handle_type & FILEID_IS_DIR) > > + ctx.fh_flags |= EXPORT_FH_DIR_ONLY; > > + } > > + /* Filesystem code should not be exposed to user flags */ > > + handle->handle_type &= ~FILEID_USER_FLAGS_MASK; > > retval = do_handle_to_path(handle, path, &ctx); > > > > out_handle: > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > > index 96b62e502f71..3e60bac74fa3 100644 > > --- a/include/linux/exportfs.h > > +++ b/include/linux/exportfs.h > > @@ -159,8 +159,17 @@ 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) > > Maybe add a nice comment here about how the handle_type word is > partitioned? > Sure, I added: +/* + * Filesystems use only lower 8 bits of file_handle type for fid_type. + * name_to_handle_at() uses upper 16 bits of type as user flags to be + * interpreted by open_by_handle_at(). + */ > > + > > +/* Flags supported in encoded handle_type that is exported to user */ > > +#define FILEID_USER_FLAGS_MASK 0xffff0000 > > +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) > > + > > +#define FILEID_IS_CONNECTABLE 0x10000 > > +#define FILEID_IS_DIR 0x40000 > > +#define FILEID_VALID_USER_FLAGS (FILEID_IS_CONNECTABLE | FILEID_IS_DIR) > > + > > +#define FILEID_USER_TYPE_IS_VALID(type) \ > > + (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS) > > > > /** > > * struct export_operations - for nfsd to communicate with file systems > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index cca7e575d1f8..6329fec40872 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -1071,8 +1071,7 @@ struct file { > > > > struct file_handle { > > __u32 handle_bytes; > > - int handle_type:16; > > - int handle_flags:16; > > + int handle_type; > > /* file identifier */ > > unsigned char f_handle[] __counted_by(handle_bytes); > > }; > > > I like that better than bitfields, fwiw. Me too :) I also added assertions in case there is an out of tree fs with weird handle type: --- a/fs/exportfs/expfs.c +++ b/fs/exportfs/expfs.c @@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid, int *max_len, struct inode *parent, int flags) { const struct export_operations *nop = inode->i_sb->s_export_op; + enum fid_type type; if (!exportfs_can_encode_fh(nop, flags)) return -EOPNOTSUPP; if (!nop && (flags & EXPORT_FH_FID)) - return exportfs_encode_ino64_fid(inode, fid, max_len); + type = exportfs_encode_ino64_fid(inode, fid, max_len); + else + type = nop->encode_fh(inode, fid->raw, max_len, parent); + + if (WARN_ON_ONCE(FILEID_USER_FLAGS(type))) + return -EINVAL; + + return type; - return nop->encode_fh(inode, fid->raw, max_len, parent); } EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh); @@ -436,6 +443,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len, char nbuf[NAME_MAX+1]; int err; + if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type))) + return -EINVAL; + I will post it in v3 after testing. Thanks for the review! Amir.
On Tue 08-10-24 15:11:39, Amir Goldstein wrote: > On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote: > > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > > > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > > > > > it more useful API that encoding a connectable file handle can mandate > > > > > the resolving of a connected fd, without having to opt-in for a > > > > > connected fd independently. > > > > > > > > This seems the best option to me too if this api is to be added. > > > > > > Thanks. > > > > > > Jeff, Chuck, > > > > > > Any thoughts on this? > > > > > > > Sorry for the delay. I think encoding the new flag into the fh itself > > is a reasonable approach. > > > > Adding Jan. > Sorry I forgot to CC you on the patches, but struct file_handle is officially > a part of fanotify ABI, so your ACK is also needed on this change. Thanks. I've actually seen this series on list, went "eww bitfields, let's sleep to this" and never got back to it. > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > index 96b62e502f71..3e60bac74fa3 100644 > --- a/include/linux/exportfs.h > +++ b/include/linux/exportfs.h > @@ -159,8 +159,17 @@ 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) > + > +/* Flags supported in encoded handle_type that is exported to user */ > +#define FILEID_USER_FLAGS_MASK 0xffff0000 > +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) > + > +#define FILEID_IS_CONNECTABLE 0x10000 > +#define FILEID_IS_DIR 0x40000 > +#define FILEID_VALID_USER_FLAGS (FILEID_IS_CONNECTABLE | FILEID_IS_DIR) FWIW I prefer this variant much over bitfields as their binary format depends on the compiler which leads to unpleasant surprises sooner rather than later. > +#define FILEID_USER_TYPE_IS_VALID(type) \ > + (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS) The macro name is confusing because it speaks about type but actually checks flags. Frankly, I'd just fold this in the single call site to make things obvious. > diff --git a/include/linux/fs.h b/include/linux/fs.h > index cca7e575d1f8..6329fec40872 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1071,8 +1071,7 @@ struct file { > > struct file_handle { > __u32 handle_bytes; > - int handle_type:16; > - int handle_flags:16; > + int handle_type; Maybe you want to make handle_type unsigned when you treat it (partially) as flags? Otherwise some constructs can lead to surprises with sign extension etc... Honza
On Wed, Oct 9, 2024 at 11:40 AM Jan Kara <jack@suse.cz> wrote: > > On Tue 08-10-24 15:11:39, Amir Goldstein wrote: > > On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote: > > > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > > > > > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > > > > > > it more useful API that encoding a connectable file handle can mandate > > > > > > the resolving of a connected fd, without having to opt-in for a > > > > > > connected fd independently. > > > > > > > > > > This seems the best option to me too if this api is to be added. > > > > > > > > Thanks. > > > > > > > > Jeff, Chuck, > > > > > > > > Any thoughts on this? > > > > > > > > > > Sorry for the delay. I think encoding the new flag into the fh itself > > > is a reasonable approach. > > > > > > > Adding Jan. > > Sorry I forgot to CC you on the patches, but struct file_handle is officially > > a part of fanotify ABI, so your ACK is also needed on this change. > > Thanks. I've actually seen this series on list, went "eww bitfields, let's > sleep to this" and never got back to it. > > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > > index 96b62e502f71..3e60bac74fa3 100644 > > --- a/include/linux/exportfs.h > > +++ b/include/linux/exportfs.h > > @@ -159,8 +159,17 @@ 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) > > + > > +/* Flags supported in encoded handle_type that is exported to user */ > > +#define FILEID_USER_FLAGS_MASK 0xffff0000 > > +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) > > + > > +#define FILEID_IS_CONNECTABLE 0x10000 > > +#define FILEID_IS_DIR 0x40000 > > +#define FILEID_VALID_USER_FLAGS (FILEID_IS_CONNECTABLE | FILEID_IS_DIR) > > FWIW I prefer this variant much over bitfields as their binary format > depends on the compiler which leads to unpleasant surprises sooner rather > than later. >mask > > +#define FILEID_USER_TYPE_IS_VALID(type) \ > > + (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS) > > The macro name is confusing Confusing enough to hide the fact that it was negated in v2... > because it speaks about type but actually > checks flags. Frankly, I'd just fold this in the single call site to make > things obvious. Agree. but see below... > > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index cca7e575d1f8..6329fec40872 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -1071,8 +1071,7 @@ struct file { > > > > struct file_handle { > > __u32 handle_bytes; > > - int handle_type:16; > > - int handle_flags:16; > > + int handle_type; > > Maybe you want to make handle_type unsigned when you treat it (partially) > as flags? Otherwise some constructs can lead to surprises with sign > extension etc... That seems like a good idea, but when I look at: /* we ask for a non connectable maybe decodeable file handle */ retval = exportfs_encode_fh(path->dentry, (struct fid *)handle->f_handle, &handle_dwords, fh_flags); handle->handle_type = retval; /* convert handle size to bytes */ handle_bytes = handle_dwords * sizeof(u32); handle->handle_bytes = handle_bytes; if ((handle->handle_bytes > f_handle.handle_bytes) || (retval == FILEID_INVALID) || (retval < 0)) { /* As per old exportfs_encode_fh documentation * we could return ENOSPC to indicate overflow * But file system returned 255 always. So handle * both the values */ if (retval == FILEID_INVALID || retval == -ENOSPC) retval = -EOVERFLOW; /* I realize that we actually return negative values in handle_type (-ESTALE would be quite common). So we probably don't want to make handle_type unsigned now, but maybe we do want a macro: #define FILEID_IS_USER_TYPE_VALID(type) \ ((type) >= 0 && \ !(FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)) that will be used for the assertions instead of assuming that FILEID_USER_FLAGS_MASK includes the sign bit. huh? Thanks, Amir.
On Wed, Oct 9, 2024 at 5:16 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Wed, Oct 9, 2024 at 11:40 AM Jan Kara <jack@suse.cz> wrote: > > > > On Tue 08-10-24 15:11:39, Amir Goldstein wrote: > > > On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote: > > > > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > > > > > > > > > open_by_handle_at(2) does not have AT_ flags argument, but also, I find > > > > > > > it more useful API that encoding a connectable file handle can mandate > > > > > > > the resolving of a connected fd, without having to opt-in for a > > > > > > > connected fd independently. > > > > > > > > > > > > This seems the best option to me too if this api is to be added. > > > > > > > > > > Thanks. > > > > > > > > > > Jeff, Chuck, > > > > > > > > > > Any thoughts on this? > > > > > > > > > > > > > Sorry for the delay. I think encoding the new flag into the fh itself > > > > is a reasonable approach. > > > > > > > > > > Adding Jan. > > > Sorry I forgot to CC you on the patches, but struct file_handle is officially > > > a part of fanotify ABI, so your ACK is also needed on this change. > > > > Thanks. I've actually seen this series on list, went "eww bitfields, let's > > sleep to this" and never got back to it. > > > > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > > > index 96b62e502f71..3e60bac74fa3 100644 > > > --- a/include/linux/exportfs.h > > > +++ b/include/linux/exportfs.h > > > @@ -159,8 +159,17 @@ 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) > > > + > > > +/* Flags supported in encoded handle_type that is exported to user */ > > > +#define FILEID_USER_FLAGS_MASK 0xffff0000 > > > +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK) > > > + > > > +#define FILEID_IS_CONNECTABLE 0x10000 > > > +#define FILEID_IS_DIR 0x40000 > > > +#define FILEID_VALID_USER_FLAGS (FILEID_IS_CONNECTABLE | FILEID_IS_DIR) > > > > FWIW I prefer this variant much over bitfields as their binary format > > depends on the compiler which leads to unpleasant surprises sooner rather > > than later. > >mask > > > +#define FILEID_USER_TYPE_IS_VALID(type) \ > > > + (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS) > > > > The macro name is confusing > > Confusing enough to hide the fact that it was negated in v2... > > > because it speaks about type but actually > > checks flags. Frankly, I'd just fold this in the single call site to make > > things obvious. > > Agree. but see below... > > > > > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > > index cca7e575d1f8..6329fec40872 100644 > > > --- a/include/linux/fs.h > > > +++ b/include/linux/fs.h > > > @@ -1071,8 +1071,7 @@ struct file { > > > > > > struct file_handle { > > > __u32 handle_bytes; > > > - int handle_type:16; > > > - int handle_flags:16; > > > + int handle_type; > > > > Maybe you want to make handle_type unsigned when you treat it (partially) > > as flags? Otherwise some constructs can lead to surprises with sign > > extension etc... > > That seems like a good idea, but when I look at: > > /* we ask for a non connectable maybe decodeable file handle */ > retval = exportfs_encode_fh(path->dentry, > (struct fid *)handle->f_handle, > &handle_dwords, fh_flags); > handle->handle_type = retval; > /* convert handle size to bytes */ > handle_bytes = handle_dwords * sizeof(u32); > handle->handle_bytes = handle_bytes; > if ((handle->handle_bytes > f_handle.handle_bytes) || > (retval == FILEID_INVALID) || (retval < 0)) { > /* As per old exportfs_encode_fh documentation > * we could return ENOSPC to indicate overflow > * But file system returned 255 always. So handle > * both the values > */ > if (retval == FILEID_INVALID || retval == -ENOSPC) > retval = -EOVERFLOW; > /* > > I realize that we actually return negative values in handle_type > (-ESTALE would be quite common). > > So we probably don't want to make handle_type unsigned now, > but maybe we do want a macro: > > #define FILEID_IS_USER_TYPE_VALID(type) \ > ((type) >= 0 && \ > !(FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)) > > that will be used for the assertions instead of assuming that > FILEID_USER_FLAGS_MASK includes the sign bit. > > huh? Scratch that, the assertions check for any user flags at all. I will just open code type < 0 there as well. Thanks, Amir.
Jeff, These patches bring the NFS connectable file handles feature to userspace servers. They rely on Christian's and Aleksa's changes recently merged to v6.12. I am aware of the usability implications with connectable file handles, which are not consistent throughout the inode lifetime (i.e. when moved to another parent), but the nfsd feature does exists and some users (me) are interested in exporting this feature to userspace. The API I chose for encoding conenctable file handles is pretty conventional (AT_HANDLE_CONNECTABLE). open_by_handle_at(2) does not have AT_ flags argument, but also, I find it more useful API that encoding a connectable file handle can mandate the resolving of a connected fd, without having to opt-in for a connected fd independently. Therefore, the whacky API from RFC was replaced with an explicit connectable flag in the unused (*) upper bits of the handle_type. (*) It may be valid for filesystems to return a handle type with upper bits set, but AFAIK, no filesystem does that. I chose to implemnent this by re-farmatting struct file_handle using bit feilds. While using bit fields in UAPI is a questionable practice, file_handle is not actually in the UAPI and the legacy struct file_handle which is described in the man page, is binary compatible with the modified kernel definition with bit fields. If this is a problem, I can add (and strip) the connectable bit using plain arithmetics. Thought and flames are welcome. Thanks, Amir. Changes since v1 [1]: - Assert on encode for disconnected path (Jeff) - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH - Drop the O_PATH mount_fd API hack (Jeff) - Encode an explicit "connectable" flag in handle type [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/ Amir Goldstein (2): fs: name_to_handle_at() support for "explicit connectable" file handles fs: open_by_handle_at() support for decoding "explicit connectable" file handles fs/fhandle.c | 70 ++++++++++++++++++++++++++++++++++---- include/linux/exportfs.h | 2 ++ include/linux/fs.h | 3 +- include/uapi/linux/fcntl.h | 1 + 4 files changed, 69 insertions(+), 7 deletions(-)