From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw0-f194.google.com ([209.85.161.194]:33785 "EHLO mail-yw0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754482AbeEaKis (ORCPT ); Thu, 31 May 2018 06:38:48 -0400 MIME-Version: 1.0 In-Reply-To: <20180531094303.3425-1-henry.wilson@acentic.com> References: <20180530102613.1194-1-henry.wilson@acentic.com> <20180531094303.3425-1-henry.wilson@acentic.com> From: Amir Goldstein Date: Thu, 31 May 2018 13:38:47 +0300 Message-ID: Subject: Re: [PATCH v4 4.17] inotify: Add flag IN_MASK_CREATE for inotify_add_watch() To: Henry Wilson Cc: Jan Kara , linux-fsdevel , linux-api@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, May 31, 2018 at 12:43 PM, wrote: > From: Henry Wilson > > 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 Reviewed-by: Amir Goldstein 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.