Re: [PATCH v4 4.17] inotify: Add flag IN_MASK_CREATE for inotify_add_watch()

[Amir Goldstein <amir73il@gmail.com>]

On Thu, May 31, 2018 at 12:43 PM,  <henry.wilson@acentic.com> wrote:
> From: Henry Wilson <henry.wilson@acentic.com>
>
> inotify: Add flag IN_MASK_CREATE for inotify_add_watch()
>
> The flag IN_MASK_CREATE is introduced as a flag for inotiy_add_watch() which
> prevents inotify from modifying any existing watches when invoked. If the
> pathname specified in the call has a watched inode associated with it and
> IN_MASK_CREATE is specified, fail with an errno of EEXIST.
>
> Use of IN_MASK_CREATE with IN_MASK_ADD is reserved for future use and will return
> EINVAL.
>
> RATIONALE
>
> In the current implementation, there is no way to prevent inotify_add_watch()
> from modifying existing watch descriptors. Even if the caller keeps a record of
> all watch descriptors collected, this is only sufficient to detect that an
> existing watch descriptor may have been modified.
>
> The assumption that a particular path will map to the same inode over multiple
> calls to inotify_add_watch() cannot be made as files can be renamed or deleted.
> It is also not possible to assume that two distinct paths do no map to the same
> inode, due to hard-links or a dereferenced symbolic link. Further uses of
> inotify_add_watch() to revert the change may cause other watch descriptors to
> be modified or created, merely compunding the problem. There is currently no
> system call such as inotify_modify_watch() to explicity modify a watch
> descriptor, which would be able to revert unwanted changes. Thus the caller
> cannot guarantee to be able to revert any changes to existing watch decriptors.
>
> Additionally the caller cannot assume that the events that are associated with a
> watch descriptor are within the set requested, as any future calls to
> inotify_add_watch() may unintentionally modify a watch descriptor's mask. Thus
> it cannot currently be guaranteed that a watch descriptor will only generate
> events which have been requested. The program must filter events which come
> through its watch descriptor to within its expected range.
>
> Signed-off-by: Henry Wilson <henry.wilson@acentic.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

See one suggestions below.

> ---
>
> EXTENSIONS
>
> A new system call inotify_modify_watch(fd, wd, mask) would be useful to modify
> an existing watch directly to avoid similar problems when modifying a watch
> descriptor's mask
>
> CHANGELOG
>
> v2: updated inotify_user_init() to the increased size of INOTIFY_ALL_BITS
> v3: renamed IN_ONLY_CREATE to IN_EXCL_ADD
>     additional rational for the change introduced to commit
> v4: renamed IN_EXCL_ADD to IN_MASK_CREATE
>     EINVAL returned when attempting to use IN_MASK_CREATE and IN_MASK_ADD
> ---
>  fs/notify/inotify/inotify_user.c | 9 ++++++++-
>  include/linux/inotify.h          | 2 +-
>  include/uapi/linux/inotify.h     | 1 +
>  3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index ef32f3657958..134e9c5308c8 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -506,6 +506,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>         __u32 old_mask, new_mask;
>         __u32 mask;
>         int add = (arg & IN_MASK_ADD);
> +       int create = (arg & IN_MASK_CREATE);
>         int ret;
>

Exclusive flag validation would look nicer here...

   if (add && create)
      return -EINVAL;

>         mask = inotify_arg_to_mask(arg);
> @@ -513,6 +514,8 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>         fsn_mark = fsnotify_find_mark(&inode->i_fsnotify_marks, group);
>         if (!fsn_mark)
>                 return -ENOENT;
> +       else if (create)
> +               return -EEXIST;
>
>         i_mark = container_of(fsn_mark, struct inotify_inode_mark, fsn_mark);
>
> @@ -714,6 +717,10 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname,
>         if (unlikely(!f.file))
>                 return -EBADF;
>
> +       /* IN_MASK_ADD and IN_MASK_CREATE don't make sense together */
> +       if (unlikely((mask & IN_MASK_ADD) && (mask & IN_MASK_CREATE)))
> +               return -EINVAL;
> +

..., but it may be safer practice
to keep it in the outer syscall interface, so not sure.

Thanks,
Amir.