On Tue 29-05-18 12:03:21, Henry Wilson wrote:
> On 29/05/18 10:15, Jan Kara wrote:
> Thank you for taking the time to respond.
> > On a general note a lot of your reasoning below actually belongs to the
> > commit message itself so that it is available later when someone wonders:
> > "Why the hell this flag was added?"
>
> Okay, I shall include it in the next patch submission.
Yes, please.
> > > ---
> > > RATIONALE
> > >
> > > When using inotify_add_watch() a programmer will expect the watch
> > > descriptor to create a new watch descriptor referencing the inode the
> > > path is associated with. However while the word 'add' implies these
> > > semantics, the system call may either create or modify watches with this
> > > call. This would be fine if the interface has a mechanism by which to
> > > distinguish between these actions. If two files, file1, file2 the new
> > > path points to the same inode as an existing watch, for example with a
> > > hard-link, a dereferenced symbolic link or the original file being
> > > renamed. The programmer will be given a watch descriptor that they alrady
> > > have.
> >
> > I can understand this may be inconvenient but you could work around this
> > rather easily in your program, couldn't you? E.g. by having a data
> > structure with already known watch descriptors for given inotify instance
> > and checking against that. Is there some problem with that?
> >
> > Honza
>
> While I agree that keeping a searchable record of all known watch
> descriptors allows the program to successfully *detect* that the inode is
> already being watched, inotify_add_watch() is a modifying action.
>
> The assumption that a particular path will map to the same inode over
> multiple calls to inotify_add_watch() cannot be made.
> Thus the program cannot guarantee to be able to *revert* any changes made by
> the detection of the repeated watch descriptor. Further uses of
> inotify_add_watch() to attempt to rectify the original modification to the
> intended watch descriptor's flags may end up clobbering other watch
> descriptors' flags. In this situation the program may be in a catch-22. It
> can try clearing affected watch descriptors and start again, but there may
> be inodes which it was watching that have been moved to an unknown path. Or
> it can give up and forget about the affected inodes.
That are all good points and belong to the commit description as well.
Please add them for the next submission.
So now I agree with adding this functionality to inotify. But please also
provide corresponding patch for man-pages so that documentation is
uptodate. Also I don't like the IN_ONLY_CREATE name as that sounds more
like an event type than a flag (and inotify sadly combines these in a
single field). Can we call it like IN_EXCL_ADD to indicate we want the mask
addition to be exclusive?
If you have time, it would be good to also provide similar flag for
fanotify_mark(2) syscall as Amir suggested. If not, I can eventually handle
that.
Honza
>
> By preventing the situation from occurring in the first place, the API is
> simpler to use.
>
> Regards, Henry.
>
> > >
> > > IMPLICATIONS OF THE DEFECT
> > >
> > > The programmer may not realise the watch descriptor has been duplicated. Thus,
> > > two logical entities in the program will have the same watch descriptor and will
> > > compete for events from the queue. The watch descriptor will be deleted more
> > > times than neccessary in this case.
> > >
> > > The call to inotify_add_watch() may modify the watch mask for the original watch
> > > descriptor and the program may attempt to revert these changes. They may be
> > > unable to revert changes due to the path no-longer being associated with the
> > > same inode. The problem may be compounded if the path is assiciated with a
> > > different inode that is being watched. If IN_MASK_ADD was not used with the
> > > call, by the time the watch mask is reverted, important events may have been
> > > missed, without a mechanism to explicitly say that events were missed. (Unlike
> > > queue overflow)
> > >
> > > 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.
> > >
> > > ALTERNATIVES
> > >
> > > Create a new system call inotify_create_watch(fd, pathname, mask) which only
> > > creates watches. This would avoid creating a new flag-bit but would require some
> > > implementation refactoring. Adding a new flag-bit was chosen as currently 21
> > > IN_xxx flag-bits exist, an additional flag-bit leaves 10 remaining for future
> > > expansion. This leverages the existing implementation and as such is a minor
> > > implementation change.
> > >
> > > 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
> > >
> > > ADDITIONS TO MANPAGES
> > >
> > > inotify(7)
> > >
> > > IN_ONLY_CREATE
> > > Only watch pathname if it refers to an inode that is not already being watched
> > > by this inotify instance.
> > >
> > > inotify_add_watch(2)
> > >
> > > EEXIST
> > > pathname references an existing watch descriptor and IN_ONLY_CREATE was
> > > specified.
> > >
> > > CHANGELOG
> > >
> > > v2: updated inotify_user_init() to the increased size of INOTIFY_ALL_BITS
> > > ---
> > > fs/notify/inotify/inotify_user.c | 5 ++++-
> > > include/linux/inotify.h | 2 +-
> > > include/uapi/linux/inotify.h | 1 +
> > > 3 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> > > index ef32f3657958..94e1234b0d38 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_ONLY_CREATE);
> > > int ret;
> > > 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);
> > > @@ -802,7 +805,7 @@ static int __init inotify_user_setup(void)
> > > BUILD_BUG_ON(IN_ISDIR != FS_ISDIR);
> > > BUILD_BUG_ON(IN_ONESHOT != FS_IN_ONESHOT);
> > > - BUG_ON(hweight32(ALL_INOTIFY_BITS) != 21);
> > > + BUG_ON(hweight32(ALL_INOTIFY_BITS) != 22);
> > > inotify_inode_mark_cachep = KMEM_CACHE(inotify_inode_mark, SLAB_PANIC);
> > > diff --git a/include/linux/inotify.h b/include/linux/inotify.h
> > > index 44f9ffe72c87..1b0b76cfc55b 100644
> > > --- a/include/linux/inotify.h
> > > +++ b/include/linux/inotify.h
> > > @@ -18,6 +18,6 @@ extern struct ctl_table inotify_table[]; /* for sysctl */
> > > IN_DELETE_SELF | IN_MOVE_SELF | IN_UNMOUNT | \
> > > IN_Q_OVERFLOW | IN_IGNORED | IN_ONLYDIR | \
> > > IN_DONT_FOLLOW | IN_EXCL_UNLINK | IN_MASK_ADD | \
> > > - IN_ISDIR | IN_ONESHOT)
> > > + IN_ONLY_CREATE | IN_ISDIR | IN_ONESHOT)
> > > #endif /* _LINUX_INOTIFY_H */
> > > diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
> > > index 4800bf2a531d..bf6ac25244d7 100644
> > > --- a/include/uapi/linux/inotify.h
> > > +++ b/include/uapi/linux/inotify.h
> > > @@ -53,6 +53,7 @@ struct inotify_event {
> > > #define IN_ONLYDIR 0x01000000 /* only watch the path if it is a directory */
> > > #define IN_DONT_FOLLOW 0x02000000 /* don't follow a sym link */
> > > #define IN_EXCL_UNLINK 0x04000000 /* exclude events on unlinked objects */
> > > +#define IN_ONLY_CREATE 0x10000000 /* only create watches */
> > > #define IN_MASK_ADD 0x20000000 /* add to the mask of an already existing watch */
> > > #define IN_ISDIR 0x40000000 /* event occurred against dir */
> > > #define IN_ONESHOT 0x80000000 /* only send event once */
> > > --
> > > 2.17.0
> > >
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
