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

[Jan Kara <jack@suse.cz>]

On Thu 31-05-18 13:38:47, Amir Goldstein wrote:
> 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.

Yeah, I think the outer interface is better for clarity. I'll take the
patch as is. Thanks for your review.

							Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR