| Message ID | 20230502124817.3070545-4-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | Prepare for supporting more filesystems with fanotify | expand |
On Tue 02-05-23 15:48:16, Amir Goldstein wrote: > Some userspace programs use st_ino as a unique object identifier, even > though inode numbers may be recycable. > > This issue has been addressed for NFS export long ago using the exportfs > file handle API and the unique file handle identifiers are also exported > to userspace via name_to_handle_at(2). > > fanotify also uses file handles to identify objects in events, but only > for filesystems that support NFS export. > > Relax the requirement for NFS export support and allow more filesystems > to export a unique object identifier via name_to_handle_at(2) with the > flag AT_HANDLE_FID. > > A file handle requested with the AT_HANDLE_FID flag, may or may not be > usable as an argument to open_by_handle_at(2). > > To allow filesystems to opt-in to supporting AT_HANDLE_FID, a struct > export_operations is required, but even an empty struct is sufficient > for encoding FIDs. Christian (or Al), are you OK with sparing one AT_ flag for this functionality? Otherwise the patch series looks fine to me so I'd like to queue it into my tree. Thanks! Honza > > Acked-by: Jeff Layton <jlayton@kernel.org> > Acked-by: Chuck Lever <chuck.lever@oracle.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/fhandle.c | 22 ++++++++++++++-------- > include/uapi/linux/fcntl.h | 5 +++++ > 2 files changed, 19 insertions(+), 8 deletions(-) > > diff --git a/fs/fhandle.c b/fs/fhandle.c > index f2bc27d1975e..4a635cf787fc 100644 > --- a/fs/fhandle.c > +++ b/fs/fhandle.c > @@ -16,7 +16,7 @@ > > static long do_sys_name_to_handle(const struct path *path, > struct file_handle __user *ufh, > - int __user *mnt_id) > + int __user *mnt_id, int fh_flags) > { > long retval; > struct file_handle f_handle; > @@ -24,11 +24,14 @@ static long do_sys_name_to_handle(const struct path *path, > struct file_handle *handle = NULL; > > /* > - * We need to make sure whether the file system > - * support decoding of the file handle > + * We need to make sure whether the file system support decoding of > + * the file handle if decodeable file handle was requested. > + * Otherwise, even empty export_operations are sufficient to opt-in > + * to encoding FIDs. > */ > if (!path->dentry->d_sb->s_export_op || > - !path->dentry->d_sb->s_export_op->fh_to_dentry) > + (!(fh_flags & EXPORT_FH_FID) && > + !path->dentry->d_sb->s_export_op->fh_to_dentry)) > return -EOPNOTSUPP; > > if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) > @@ -45,10 +48,10 @@ static long do_sys_name_to_handle(const struct path *path, > /* convert handle size to multiple of sizeof(u32) */ > handle_dwords = f_handle.handle_bytes >> 2; > > - /* we ask for a non connected handle */ > + /* we ask for a non connectable maybe decodeable file handle */ > retval = exportfs_encode_fh(path->dentry, > (struct fid *)handle->f_handle, > - &handle_dwords, 0); > + &handle_dwords, fh_flags); > handle->handle_type = retval; > /* convert handle size to bytes */ > handle_bytes = handle_dwords * sizeof(u32); > @@ -84,6 +87,7 @@ static long do_sys_name_to_handle(const struct path *path, > * @handle: resulting file handle > * @mnt_id: mount id of the file system containing the file > * @flag: flag value to indicate whether to follow symlink or not > + * and whether a decodable file handle is required. > * > * @handle->handle_size indicate the space available to store the > * variable part of the file handle in bytes. If there is not > @@ -96,17 +100,19 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name, > { > struct path path; > int lookup_flags; > + int fh_flags; > int err; > > - if ((flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0) > + if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID)) > return -EINVAL; > > lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0; > + fh_flags = (flag & AT_HANDLE_FID) ? EXPORT_FH_FID : 0; > if (flag & AT_EMPTY_PATH) > lookup_flags |= LOOKUP_EMPTY; > err = user_path_at(dfd, name, lookup_flags, &path); > if (!err) { > - err = do_sys_name_to_handle(&path, handle, mnt_id); > + err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags); > path_put(&path); > } > return err; > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h > index e8c07da58c9f..3091080db069 100644 > --- a/include/uapi/linux/fcntl.h > +++ b/include/uapi/linux/fcntl.h > @@ -112,4 +112,9 @@ > > #define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */ > > +/* Flags for name_to_handle_at(2) */ > +#define AT_HANDLE_FID 0x10000 /* file handle is needed to compare > + object indentity and may not be > + usable to open_by_handle_at(2) */ > + > #endif /* _UAPI_LINUX_FCNTL_H */ > -- > 2.34.1 >
On Wed, May 03, 2023 at 07:23:14PM +0200, Jan Kara wrote: > On Tue 02-05-23 15:48:16, Amir Goldstein wrote: > > Some userspace programs use st_ino as a unique object identifier, even > > though inode numbers may be recycable. > > > > This issue has been addressed for NFS export long ago using the exportfs > > file handle API and the unique file handle identifiers are also exported > > to userspace via name_to_handle_at(2). > > > > fanotify also uses file handles to identify objects in events, but only > > for filesystems that support NFS export. > > > > Relax the requirement for NFS export support and allow more filesystems > > to export a unique object identifier via name_to_handle_at(2) with the > > flag AT_HANDLE_FID. > > > > A file handle requested with the AT_HANDLE_FID flag, may or may not be > > usable as an argument to open_by_handle_at(2). > > > > To allow filesystems to opt-in to supporting AT_HANDLE_FID, a struct > > export_operations is required, but even an empty struct is sufficient > > for encoding FIDs. > > Christian (or Al), are you OK with sparing one AT_ flag for this > functionality? Otherwise the patch series looks fine to me so I'd like to > queue it into my tree. Thanks! At first it looked like there are reasons to complain about this on the grounds that this seems like a flag only for a single system call. But another look at include/uapi/linux/fcntl.h reveals that oh well, the AT_* flag namespace already contains system call specific flags. The overloading of AT_EACCESS and AT_REMOVEDIR as 0x200 is especially creative since it doesn't even use an infix like the statx specific flags. Long story short, since there's already overloading of the flag namespace happening it wouldn't be unprecedent or in any way wrong if this patch just reused the 0x200 value as well. In fact, it might come in handy since it would mean that we have the bit you're using right now free for a flag that is meaningful for multiple system calls. So something to consider but you can just change that in-tree as far as I'm concerned. All this amounts to a long-winded, Acked-by: Christian Brauner <brauner@kernel.org>
On Thu, May 4, 2023 at 2:19 PM Christian Brauner <brauner@kernel.org> wrote: > > On Wed, May 03, 2023 at 07:23:14PM +0200, Jan Kara wrote: > > On Tue 02-05-23 15:48:16, Amir Goldstein wrote: > > > Some userspace programs use st_ino as a unique object identifier, even > > > though inode numbers may be recycable. > > > > > > This issue has been addressed for NFS export long ago using the exportfs > > > file handle API and the unique file handle identifiers are also exported > > > to userspace via name_to_handle_at(2). > > > > > > fanotify also uses file handles to identify objects in events, but only > > > for filesystems that support NFS export. > > > > > > Relax the requirement for NFS export support and allow more filesystems > > > to export a unique object identifier via name_to_handle_at(2) with the > > > flag AT_HANDLE_FID. > > > > > > A file handle requested with the AT_HANDLE_FID flag, may or may not be > > > usable as an argument to open_by_handle_at(2). > > > > > > To allow filesystems to opt-in to supporting AT_HANDLE_FID, a struct > > > export_operations is required, but even an empty struct is sufficient > > > for encoding FIDs. > > > > Christian (or Al), are you OK with sparing one AT_ flag for this > > functionality? Otherwise the patch series looks fine to me so I'd like to > > queue it into my tree. Thanks! > > At first it looked like there are reasons to complain about this on the > grounds that this seems like a flag only for a single system call. But > another look at include/uapi/linux/fcntl.h reveals that oh well, the > AT_* flag namespace already contains system call specific flags. > > The overloading of AT_EACCESS and AT_REMOVEDIR as 0x200 is especially > creative since it doesn't even use an infix like the statx specific > flags. > > Long story short, since there's already overloading of the flag > namespace happening it wouldn't be unprecedent or in any way wrong if > this patch just reused the 0x200 value as well. > I had considered this myself as well... Couldn't decide if this was ugly or not. Obviously, I do not mind which value the flag gets. > In fact, it might come in handy since it would mean that we have the bit > you're using right now free for a flag that is meaningful for multiple > system calls. So something to consider but you can just change that > in-tree as far as I'm concerned. > > All this amounts to a long-winded, > > Acked-by: Christian Brauner <brauner@kernel.org> Thanks, Amir.
diff --git a/fs/fhandle.c b/fs/fhandle.c index f2bc27d1975e..4a635cf787fc 100644 --- a/fs/fhandle.c +++ b/fs/fhandle.c @@ -16,7 +16,7 @@ static long do_sys_name_to_handle(const struct path *path, struct file_handle __user *ufh, - int __user *mnt_id) + int __user *mnt_id, int fh_flags) { long retval; struct file_handle f_handle; @@ -24,11 +24,14 @@ static long do_sys_name_to_handle(const struct path *path, struct file_handle *handle = NULL; /* - * We need to make sure whether the file system - * support decoding of the file handle + * We need to make sure whether the file system support decoding of + * the file handle if decodeable file handle was requested. + * Otherwise, even empty export_operations are sufficient to opt-in + * to encoding FIDs. */ if (!path->dentry->d_sb->s_export_op || - !path->dentry->d_sb->s_export_op->fh_to_dentry) + (!(fh_flags & EXPORT_FH_FID) && + !path->dentry->d_sb->s_export_op->fh_to_dentry)) return -EOPNOTSUPP; if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) @@ -45,10 +48,10 @@ static long do_sys_name_to_handle(const struct path *path, /* convert handle size to multiple of sizeof(u32) */ handle_dwords = f_handle.handle_bytes >> 2; - /* we ask for a non connected handle */ + /* we ask for a non connectable maybe decodeable file handle */ retval = exportfs_encode_fh(path->dentry, (struct fid *)handle->f_handle, - &handle_dwords, 0); + &handle_dwords, fh_flags); handle->handle_type = retval; /* convert handle size to bytes */ handle_bytes = handle_dwords * sizeof(u32); @@ -84,6 +87,7 @@ static long do_sys_name_to_handle(const struct path *path, * @handle: resulting file handle * @mnt_id: mount id of the file system containing the file * @flag: flag value to indicate whether to follow symlink or not + * and whether a decodable file handle is required. * * @handle->handle_size indicate the space available to store the * variable part of the file handle in bytes. If there is not @@ -96,17 +100,19 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name, { struct path path; int lookup_flags; + int fh_flags; int err; - if ((flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0) + if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID)) return -EINVAL; lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0; + fh_flags = (flag & AT_HANDLE_FID) ? EXPORT_FH_FID : 0; if (flag & AT_EMPTY_PATH) lookup_flags |= LOOKUP_EMPTY; err = user_path_at(dfd, name, lookup_flags, &path); if (!err) { - err = do_sys_name_to_handle(&path, handle, mnt_id); + err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags); path_put(&path); } return err; diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h index e8c07da58c9f..3091080db069 100644 --- a/include/uapi/linux/fcntl.h +++ b/include/uapi/linux/fcntl.h @@ -112,4 +112,9 @@ #define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */ +/* Flags for name_to_handle_at(2) */ +#define AT_HANDLE_FID 0x10000 /* file handle is needed to compare + object indentity and may not be + usable to open_by_handle_at(2) */ + #endif /* _UAPI_LINUX_FCNTL_H */