| Message ID | 20181202113826.32133-9-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: add support for more event types | expand |
On Sun, Dec 2, 2018 at 1:38 PM Amir Goldstein <amir73il@gmail.com> wrote: > > When setting up an fanotify listener, user may request to get fid > information in event instead of an open file descriptor. > > The fid obtained with event on a watched object contains the file > handle returned by name_to_handle_at(2) and fsid returned by statfs(2). > > When setting a mark, we need to make sure that the filesystem > supports encoding file handles with name_to_handle_at(2) and that > statfs(2) encodes a non-zero fsid. > Jan, On a discussion with Matthew about tests he is writing for FAN_REPORT_TID, the issue of permission events came up. Since I am not aware of any specific benefit that FAN_REPORT_TID could bring to users of permission events, I think the best course of action is to limit the use of FAN_REPORT_TID to group with priority FAN_CLASS_NOTIF. That would simplify tests and man page and if we ever see a use case for anything else, we can add that in the future. If you agree, we should add something like this to this patch: --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -768,6 +768,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) return -EINVAL; } + if ((flags & FAN_REPORT_FID) && + (flags & FANOTIFY_CLASS_BITS) != FAN_CLASS_NOTIF) + return -EINVAL; + user = get_current_user(); if (atomic_read(&user->fanotify_listeners) > FANOTIFY_DEFAULT_MAX_LISTENERS) { free_uid(user); Thanks, Amir.
On Sat 08-12-18 11:26:38, Amir Goldstein wrote: > On Sun, Dec 2, 2018 at 1:38 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > When setting up an fanotify listener, user may request to get fid > > information in event instead of an open file descriptor. > > > > The fid obtained with event on a watched object contains the file > > handle returned by name_to_handle_at(2) and fsid returned by statfs(2). > > > > When setting a mark, we need to make sure that the filesystem > > supports encoding file handles with name_to_handle_at(2) and that > > statfs(2) encodes a non-zero fsid. > > > > Jan, > > On a discussion with Matthew about tests he is writing for FAN_REPORT_TID, > the issue of permission events came up. > Since I am not aware of any specific benefit that FAN_REPORT_TID could > bring to users of permission events, I think the best course of action is to > limit the use of FAN_REPORT_TID to group with priority FAN_CLASS_NOTIF. > That would simplify tests and man page and if we ever see a use case for > anything else, we can add that in the future. > > If you agree, we should add something like this to this patch: Yeah, that's a good point. Agreed. Honza > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -768,6 +768,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, > flags, unsigned int, event_f_flags) > return -EINVAL; > } > > + if ((flags & FAN_REPORT_FID) && > + (flags & FANOTIFY_CLASS_BITS) != FAN_CLASS_NOTIF) > + return -EINVAL; > + > user = get_current_user(); > if (atomic_read(&user->fanotify_listeners) > > FANOTIFY_DEFAULT_MAX_LISTENERS) { > free_uid(user); > > > Thanks, > Amir.
On Mon, Dec 10, 2018 at 05:20:38PM +0100, Jan Kara wrote: > On Sat 08-12-18 11:26:38, Amir Goldstein wrote: > > On Sun, Dec 2, 2018 at 1:38 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > When setting up an fanotify listener, user may request to get fid > > > information in event instead of an open file descriptor. > > > > > > The fid obtained with event on a watched object contains the file > > > handle returned by name_to_handle_at(2) and fsid returned by statfs(2). > > > > > > When setting a mark, we need to make sure that the filesystem > > > supports encoding file handles with name_to_handle_at(2) and that > > > statfs(2) encodes a non-zero fsid. > > > > > > > Jan, > > > > On a discussion with Matthew about tests he is writing for FAN_REPORT_TID, > > the issue of permission events came up. > > Since I am not aware of any specific benefit that FAN_REPORT_TID could > > bring to users of permission events, I think the best course of action is to > > limit the use of FAN_REPORT_TID to group with priority FAN_CLASS_NOTIF. > > That would simplify tests and man page and if we ever see a use case for > > anything else, we can add that in the future. > > > > If you agree, we should add something like this to this patch: > > Yeah, that's a good point. Agreed. OK, good to know. I will continue to write the FAN_REPORT_FID tests based on Amir's fanotify_dirent branch, which contains the amendment suggested below. Amir, presumably we should also have a separate test that covers the expected error result when FAN_REPORT_FID is supplied with invalid class bits? > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -768,6 +768,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, > > flags, unsigned int, event_f_flags) > > return -EINVAL; > > } > > > > + if ((flags & FAN_REPORT_FID) && > > + (flags & FANOTIFY_CLASS_BITS) != FAN_CLASS_NOTIF) > > + return -EINVAL; > > + > > user = get_current_user(); > > if (atomic_read(&user->fanotify_listeners) > > > FANOTIFY_DEFAULT_MAX_LISTENERS) { > > free_uid(user);
On Tue, Dec 11, 2018 at 8:12 AM Matthew Bobrowski <mbobrowski@mbobrowski.org> wrote: > > On Mon, Dec 10, 2018 at 05:20:38PM +0100, Jan Kara wrote: > > On Sat 08-12-18 11:26:38, Amir Goldstein wrote: > > > On Sun, Dec 2, 2018 at 1:38 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > > > When setting up an fanotify listener, user may request to get fid > > > > information in event instead of an open file descriptor. > > > > > > > > The fid obtained with event on a watched object contains the file > > > > handle returned by name_to_handle_at(2) and fsid returned by statfs(2). > > > > > > > > When setting a mark, we need to make sure that the filesystem > > > > supports encoding file handles with name_to_handle_at(2) and that > > > > statfs(2) encodes a non-zero fsid. > > > > > > > > > > Jan, > > > > > > On a discussion with Matthew about tests he is writing for FAN_REPORT_TID, > > > the issue of permission events came up. > > > Since I am not aware of any specific benefit that FAN_REPORT_TID could > > > bring to users of permission events, I think the best course of action is to > > > limit the use of FAN_REPORT_TID to group with priority FAN_CLASS_NOTIF. > > > That would simplify tests and man page and if we ever see a use case for > > > anything else, we can add that in the future. > > > > > > If you agree, we should add something like this to this patch: > > > > Yeah, that's a good point. Agreed. > > OK, good to know. I will continue to write the FAN_REPORT_FID tests based on > Amir's fanotify_dirent branch, which contains the amendment suggested below. > > Amir, presumably we should also have a separate test that covers the expected > error result when FAN_REPORT_FID is supplied with invalid class bits? > Sure, that sounds good. I also did not find any test coverage for expected error result when trying to set permission mark bits on a FAN_CLASS_NOTIF group, so the same test could be the home of this test case as well. Going further to dirent events, we would also want to add validation that dirent event cannot be set on mark for group without FAN_REPORT_FID and cannot be set on a mount mark. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 032a9a225a21..3b8d442e67cd 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -17,6 +17,8 @@ #include <linux/compat.h> #include <linux/sched/signal.h> #include <linux/memcontrol.h> +#include <linux/statfs.h> +#include <linux/exportfs.h> #include <asm/ioctls.h> @@ -850,6 +852,52 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) return fd; } +/* Check if filesystem can encode a unique fid */ +static int fanotify_test_fid(struct path *path) +{ + struct kstatfs stat, root_stat; + struct path root = { + .mnt = path->mnt, + .dentry = path->dentry->d_sb->s_root, + }; + int err; + + /* + * Make sure path is not in filesystem with zero fsid (e.g. tmpfs). + */ + err = vfs_statfs(path, &stat); + if (err) + return err; + + if (!stat.f_fsid.val[0] && !stat.f_fsid.val[1]) + return -ENODEV; + + /* + * Make sure path is not inside a filesystem subvolume (e.g. btrfs) + * which uses a different fsid than sb root. + */ + err = vfs_statfs(&root, &root_stat); + if (err) + return err; + + if (root_stat.f_fsid.val[0] != stat.f_fsid.val[0] || + root_stat.f_fsid.val[1] != stat.f_fsid.val[1]) + return -EXDEV; + + /* + * We need to make sure that the file system supports at least + * encoding a file handle so user can use name_to_handle_at() to + * compare fid returned with event to the file handle of watched + * objects. However, name_to_handle_at() requires that the + * filesystem also supports decoding file handles. + */ + if (!path->dentry->d_sb->s_export_op || + !path->dentry->d_sb->s_export_op->fh_to_dentry) + return -EOPNOTSUPP; + + return 0; +} + static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, int dfd, const char __user *pathname) { @@ -935,6 +983,12 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, if (ret) goto fput_and_out; + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { + ret = fanotify_test_fid(&path); + if (ret) + goto path_put_and_out; + } + /* inode held in place by reference to path; group by fget on fd */ if (mark_type == FAN_MARK_INODE) inode = path.dentry->d_inode; @@ -963,6 +1017,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, ret = -EINVAL; } +path_put_and_out: path_put(&path); fput_and_out: fdput(f); @@ -999,7 +1054,7 @@ COMPAT_SYSCALL_DEFINE6(fanotify_mark, */ static int __init fanotify_user_setup(void) { - BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 7); + BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 8); BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 9); fanotify_mark_cache = KMEM_CACHE(fsnotify_mark, diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h index 9e2142795335..f59be967f72b 100644 --- a/include/linux/fanotify.h +++ b/include/linux/fanotify.h @@ -19,7 +19,7 @@ FAN_CLASS_PRE_CONTENT) #define FANOTIFY_INIT_FLAGS (FANOTIFY_CLASS_BITS | \ - FAN_REPORT_TID | \ + FAN_REPORT_TID | FAN_REPORT_FID | \ FAN_CLOEXEC | FAN_NONBLOCK | \ FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
When setting up an fanotify listener, user may request to get fid information in event instead of an open file descriptor. The fid obtained with event on a watched object contains the file handle returned by name_to_handle_at(2) and fsid returned by statfs(2). When setting a mark, we need to make sure that the filesystem supports encoding file handles with name_to_handle_at(2) and that statfs(2) encodes a non-zero fsid. Cc: <linux-api@vger.kernel.org> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify_user.c | 57 +++++++++++++++++++++++++++++- include/linux/fanotify.h | 2 +- 2 files changed, 57 insertions(+), 2 deletions(-)