Re: [PATCH v2 4.17] From: Henry Wilson <henry.wilson@acentic.com>

[Jan Kara <jack@suse.cz>]

On Wed 16-05-18 10:44:30, henry.wilson@acentic.com wrote:
> From: Henry Wilson <henry.wilson@acentic.com>
> 
> inotify: Add flag IN_ONLY_CREATE for inotify_add_watch()
> 
> The flag IN_ONLY_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_ONLY_CREATE is specified, fail with an errno of EEXIST.
> 
> Signed-off-by: Henry Wilson <henry.wilson@acentic.com>

Thanks for the patch and sorry for not responding earlier. You've got my
email address subtly wrong (the domain is 'suse', not 'suze') and thus I've
seen your email only now when reading older list posts.

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?"

> ---
> 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

> 
> 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@suse.com>
SUSE Labs, CR