[PATCH 4.17] inotify: Add flag IN_ONLY_CREATE for inotify_add_watch()

[PATCH 4.17] inotify: Add flag IN_ONLY_CREATE for inotify_add_watch()

[Henry Wilson]

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

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.
---
 fs/notify/inotify/inotify_user.c | 3 +++
 include/linux/inotify.h          | 2 +-
 include/uapi/linux/inotify.h     | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index ef32f3657958..24cad6bf7579 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);
 
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

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

[Amir Goldstein]

On Thu, May 3, 2018 at 6:24 PM, Henry Wilson <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>
> ---
> 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.
>
> 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.

I'm not crazy about the flag name. No great ideas for name. Maybe I_NO_UPDATE.

If this API change is considered, I would like to see a similar change
to fanotify,
because I don't like fact there is missing functionality in fanotify
and would not
like that gap to grow.

Thanks,
Amir.

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

[Henry Wilson]

On 2018-05-03 19:42, Amir Goldstein <amir73il@gmail.com> wrote:
 > I'm not crazy about the flag name.

I'm not particularly attached to the name.
Here's a list of the names I can think of at this moment:

IN_ONLY_CREATE
IN_NO_UPDATE
IN_DONT_UPDATE
IN_NO_MODIFY
IN_DONT_MODIFY
IN_SAFE_CREATE
IN_ADD_WATCH_CREATE_ONLY
IN_CONSTRUCT

Regards, Henry.

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

[henry.wilson]

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

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

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

[Jan Kara]

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

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

[Henry Wilson]

On 29/05/18 10:15, Jan Kara wrote:
> 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.

My apologies for missing that and messing up the title of the previous 
email.
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.

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

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

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

[Jan Kara]

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

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

[Henry Wilson]

On 29/05/18 13:15, Jan Kara wrote:
> So now I agree with adding this functionality to inotify. But please also
> provide corresponding patch for man-pages so that documentation is
> uptodate.

I shall prepare a patch for man-pages later today.

Regards, Henry

[PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[henry.wilson]

From: Henry Wilson <henry.wilson@acentic.com>

inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

The flag IN_EXCL_ADD 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_EXCL_ADD is specified, fail with an errno of EEXIST.

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

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_EXCL_ADD
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_EXCL_ADD was
specified.

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
---
 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..bbe6fbc4fee6 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_EXCL_ADD);
 	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..ab423272540c 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_EXCL_ADD | IN_ISDIR | IN_ONESHOT)
 
 #endif	/* _LINUX_INOTIFY_H */
diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
index 4800bf2a531d..7601540aa38a 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_EXCL_ADD		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

Re: [PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[Jan Kara]

Hello,

On Wed 30-05-18 10:26:13, henry.wilson@acentic.com wrote:
> From: Henry Wilson <henry.wilson@acentic.com>
> 
> inotify: Add flag IN_EXCL_ADD for inotify_add_watch()
> 
> The flag IN_EXCL_ADD 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_EXCL_ADD is specified, fail with an errno of EEXIST.
> 
> 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>

Thanks. The patch looks good. I've added it to my tree. BTW, do you plan on
working on a similar addition to fanotify?

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

Re: [PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[Henry Wilson]

On 30/05/18 14:01, Jan Kara wrote:
> Thanks. The patch looks good. I've added it to my tree. BTW, do you plan on
> working on a similar addition to fanotify?
> 
> 								Honza
> 

Ah that's grand, I'm glad to have helped to improve things.
I'm not familiar with fanotify, however a quick look at fanotify_user.c 
suggests that a similar approach may be taken by modifying:

if(!fsn_mark) {
	...
}
else if (create) {
	return -EEXIST;
}

in both fanotify_add_vfsmount_mark() and fanotify_add_inode_mark()

Regards, Henry.

Re: [PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[Amir Goldstein]

On Wed, May 30, 2018 at 4:35 PM, Henry Wilson <henry.wilson@acentic.com> wrote:
> On 30/05/18 14:01, Jan Kara wrote:
>>
>> Thanks. The patch looks good. I've added it to my tree. BTW, do you plan
>> on
>> working on a similar addition to fanotify?
>>
>>                                                                 Honza
>>
>
> Ah that's grand, I'm glad to have helped to improve things.
> I'm not familiar with fanotify, however a quick look at fanotify_user.c
> suggests that a similar approach may be taken by modifying:
>
> if(!fsn_mark) {
>         ...
> }
> else if (create) {
>         return -EEXIST;
> }
>
> in both fanotify_add_vfsmount_mark() and fanotify_add_inode_mark()
>

I think that was a yes/no question and I interpret your answer as no??

Anyway, another yes/no question:
Can you write a simple LTP test to verify the new API?

I reccon Jan was also expecting an actual patch posted to man pages
maintainer (and linux-api, which was not cc'ed on the latest patch).

About the fanotify change, since fanotify API does have 'flags' separate
from 'mask', I am not sure if FAN_MARK_EXCL_ADD would be the
best flag name?? Perhaps FAN_MARK_CREATE? FAN_MARK_NEW?
not sure.

But also, I did not get a chance to comment about the chosen inotify
flag name that the lexical proximity to IN_EXCL_UNLINK is a bit odd
considering that _EXCL_ mean two completely different things.

Should we maybe re-consider the chosen flag name?
Maybe include linux-api in the discussion?

Thanks,
Amir.

Re: [PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[Jan Kara]

On Wed 30-05-18 18:40:27, Amir Goldstein wrote:
> On Wed, May 30, 2018 at 4:35 PM, Henry Wilson <henry.wilson@acentic.com> wrote:
> > On 30/05/18 14:01, Jan Kara wrote:
> >>
> >> Thanks. The patch looks good. I've added it to my tree. BTW, do you plan
> >> on
> >> working on a similar addition to fanotify?
> >>
> >>                                                                 Honza
> >>
> >
> > Ah that's grand, I'm glad to have helped to improve things.
> > I'm not familiar with fanotify, however a quick look at fanotify_user.c
> > suggests that a similar approach may be taken by modifying:
> >
> > if(!fsn_mark) {
> >         ...
> > }
> > else if (create) {
> >         return -EEXIST;
> > }
> >
> > in both fanotify_add_vfsmount_mark() and fanotify_add_inode_mark()
> >
> 
> I think that was a yes/no question and I interpret your answer as no??
> 
> Anyway, another yes/no question:
> Can you write a simple LTP test to verify the new API?
> 
> I reccon Jan was also expecting an actual patch posted to man pages
> maintainer (and linux-api, which was not cc'ed on the latest patch).

Yes, and I think Henry is about to post it, just didn't get to it yet.

> About the fanotify change, since fanotify API does have 'flags' separate
> from 'mask', I am not sure if FAN_MARK_EXCL_ADD would be the
> best flag name?? Perhaps FAN_MARK_CREATE? FAN_MARK_NEW?
> not sure.

Yes, for fanotify we could choose a different name.

> But also, I did not get a chance to comment about the chosen inotify
> flag name that the lexical proximity to IN_EXCL_UNLINK is a bit odd
> considering that _EXCL_ mean two completely different things.
> 
> Should we maybe re-consider the chosen flag name?

I'm open to that, I have the patch just sitting in an internal branch for
now. Do you have a better suggestion? Maybe since we already have
IN_MASK_ADD, we could call it IN_MASK_CREATE? And then FAN_MARK_CREATE for
fanotify_mark(2)?

> Maybe include linux-api in the discussion?

Probably we should, added.

								Honza

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

Re: [PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[Amir Goldstein]

On Wed, May 30, 2018 at 7:04 PM, Jan Kara <jack@suse.cz> wrote:
> On Wed 30-05-18 18:40:27, Amir Goldstein wrote:
>> On Wed, May 30, 2018 at 4:35 PM, Henry Wilson <henry.wilson@acentic.com> wrote:
>> > On 30/05/18 14:01, Jan Kara wrote:
>> >>
>> >> Thanks. The patch looks good. I've added it to my tree. BTW, do you plan
>> >> on
>> >> working on a similar addition to fanotify?
>> >>
>> >>                                                                 Honza
>> >>
>> >
>> > Ah that's grand, I'm glad to have helped to improve things.
>> > I'm not familiar with fanotify, however a quick look at fanotify_user.c
>> > suggests that a similar approach may be taken by modifying:
>> >
>> > if(!fsn_mark) {
>> >         ...
>> > }
>> > else if (create) {
>> >         return -EEXIST;
>> > }
>> >
>> > in both fanotify_add_vfsmount_mark() and fanotify_add_inode_mark()
>> >
>>
>> I think that was a yes/no question and I interpret your answer as no??
>>
>> Anyway, another yes/no question:
>> Can you write a simple LTP test to verify the new API?
>>
>> I reccon Jan was also expecting an actual patch posted to man pages
>> maintainer (and linux-api, which was not cc'ed on the latest patch).
>
> Yes, and I think Henry is about to post it, just didn't get to it yet.
>
>> About the fanotify change, since fanotify API does have 'flags' separate
>> from 'mask', I am not sure if FAN_MARK_EXCL_ADD would be the
>> best flag name?? Perhaps FAN_MARK_CREATE? FAN_MARK_NEW?
>> not sure.
>
> Yes, for fanotify we could choose a different name.
>
>> But also, I did not get a chance to comment about the chosen inotify
>> flag name that the lexical proximity to IN_EXCL_UNLINK is a bit odd
>> considering that _EXCL_ mean two completely different things.
>>
>> Should we maybe re-consider the chosen flag name?
>
> I'm open to that, I have the patch just sitting in an internal branch for
> now. Do you have a better suggestion? Maybe since we already have
> IN_MASK_ADD, we could call it IN_MASK_CREATE? And then FAN_MARK_CREATE for
> fanotify_mark(2)?
>

I like that naming.

Thinking forward, since we don't have many reserved bits left in inotify mask,
we should return -EINVAL for both IN_MASK_ADD and IN_MASK_CREATE
(they don't make sense together anyway), so we may re purpose the flag
combination in the future for something else.

Thanks,
Amir.

Re: [PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[Jan Kara]

On Wed 30-05-18 22:03:35, Amir Goldstein wrote:
> On Wed, May 30, 2018 at 7:04 PM, Jan Kara <jack@suse.cz> wrote:
> > On Wed 30-05-18 18:40:27, Amir Goldstein wrote:
> >> On Wed, May 30, 2018 at 4:35 PM, Henry Wilson <henry.wilson@acentic.com> wrote:
> >> > On 30/05/18 14:01, Jan Kara wrote:
> >> >>
> >> >> Thanks. The patch looks good. I've added it to my tree. BTW, do you plan
> >> >> on
> >> >> working on a similar addition to fanotify?
> >> >>
> >> >>                                                                 Honza
> >> >>
> >> >
> >> > Ah that's grand, I'm glad to have helped to improve things.
> >> > I'm not familiar with fanotify, however a quick look at fanotify_user.c
> >> > suggests that a similar approach may be taken by modifying:
> >> >
> >> > if(!fsn_mark) {
> >> >         ...
> >> > }
> >> > else if (create) {
> >> >         return -EEXIST;
> >> > }
> >> >
> >> > in both fanotify_add_vfsmount_mark() and fanotify_add_inode_mark()
> >> >
> >>
> >> I think that was a yes/no question and I interpret your answer as no??
> >>
> >> Anyway, another yes/no question:
> >> Can you write a simple LTP test to verify the new API?
> >>
> >> I reccon Jan was also expecting an actual patch posted to man pages
> >> maintainer (and linux-api, which was not cc'ed on the latest patch).
> >
> > Yes, and I think Henry is about to post it, just didn't get to it yet.
> >
> >> About the fanotify change, since fanotify API does have 'flags' separate
> >> from 'mask', I am not sure if FAN_MARK_EXCL_ADD would be the
> >> best flag name?? Perhaps FAN_MARK_CREATE? FAN_MARK_NEW?
> >> not sure.
> >
> > Yes, for fanotify we could choose a different name.
> >
> >> But also, I did not get a chance to comment about the chosen inotify
> >> flag name that the lexical proximity to IN_EXCL_UNLINK is a bit odd
> >> considering that _EXCL_ mean two completely different things.
> >>
> >> Should we maybe re-consider the chosen flag name?
> >
> > I'm open to that, I have the patch just sitting in an internal branch for
> > now. Do you have a better suggestion? Maybe since we already have
> > IN_MASK_ADD, we could call it IN_MASK_CREATE? And then FAN_MARK_CREATE for
> > fanotify_mark(2)?
> >
> 
> I like that naming.
> 
> Thinking forward, since we don't have many reserved bits left in inotify mask,
> we should return -EINVAL for both IN_MASK_ADD and IN_MASK_CREATE
> (they don't make sense together anyway), so we may re purpose the flag
> combination in the future for something else.

Good point, I'll update the patch.

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

Re: [PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[Henry Wilson]

On 30/05/18 17:04, Jan Kara wrote:
> On Wed 30-05-18 18:40:27, Amir Goldstein wrote:
>> On Wed, May 30, 2018 at 4:35 PM, Henry Wilson <henry.wilson@acentic.com> wrote:
>>> On 30/05/18 14:01, Jan Kara wrote:
>>>>
>>>> Thanks. The patch looks good. I've added it to my tree. BTW, do you plan
>>>> on
>>>> working on a similar addition to fanotify?
>>>>
>>>>                                                                  Honza
>>>>
>>>
>>> Ah that's grand, I'm glad to have helped to improve things.
>>> I'm not familiar with fanotify, however a quick look at fanotify_user.c
>>> suggests that a similar approach may be taken by modifying:
>>>
>>> if(!fsn_mark) {
>>>          ...
>>> }
>>> else if (create) {
>>>          return -EEXIST;
>>> }
>>>
>>> in both fanotify_add_vfsmount_mark() and fanotify_add_inode_mark()
>>>
>>
>> I think that was a yes/no question and I interpret your answer as no?? >>
>> Anyway, another yes/no question:
>> Can you write a simple LTP test to verify the new API?

I shall have a go at writing a test, yes.

>>
>> I reccon Jan was also expecting an actual patch posted to man pages
>> maintainer (and linux-api, which was not cc'ed on the latest patch).

Ah, I did not know linux-api needed to be cc'ed in.

> 
> Yes, and I think Henry is about to post it, just didn't get to it yet.

For reference here is an archive link to the thread on the linux-man archive
https://marc.info/?l=linux-man&m=152769572917930&w=2

> 
>> About the fanotify change, since fanotify API does have 'flags' separate
>> from 'mask', I am not sure if FAN_MARK_EXCL_ADD would be the
>> best flag name?? Perhaps FAN_MARK_CREATE? FAN_MARK_NEW?
>> not sure.
> 
> Yes, for fanotify we could choose a different name.
> 
>> But also, I did not get a chance to comment about the chosen inotify
>> flag name that the lexical proximity to IN_EXCL_UNLINK is a bit odd
>> considering that _EXCL_ mean two completely different things.
>>
>> Should we maybe re-consider the chosen flag name? >
> I'm open to that, I have the patch just sitting in an internal branch for
> now. Do you have a better suggestion? Maybe since we already have
> IN_MASK_ADD, we could call it IN_MASK_CREATE? And then FAN_MARK_CREATE for
> fanotify_mark(2)?
IN_MASK_CREATE seems the most logical to me too.
I'm happy to resubmit the patch if necessary.

> 
>> Maybe include linux-api in the discussion?
> 
> Probably we should, added.
> 
> 								Honza
> 

Re: [PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[Amir Goldstein]

On Thu, May 31, 2018 at 11:22 AM, Henry Wilson <henry.wilson@acentic.com> wrote:
>
>
> On 30/05/18 17:04, Jan Kara wrote:
>>
>> On Wed 30-05-18 18:40:27, Amir Goldstein wrote:
>>>
>>> On Wed, May 30, 2018 at 4:35 PM, Henry Wilson <henry.wilson@acentic.com>
>>> wrote:
>>>>
>>>> On 30/05/18 14:01, Jan Kara wrote:
>>>>>
>>>>>
>>>>> Thanks. The patch looks good. I've added it to my tree. BTW, do you
>>>>> plan
>>>>> on
>>>>> working on a similar addition to fanotify?
>>>>>
>>>>>                                                                  Honza
>>>>>
>>>>
>>>> Ah that's grand, I'm glad to have helped to improve things.
>>>> I'm not familiar with fanotify, however a quick look at fanotify_user.c
>>>> suggests that a similar approach may be taken by modifying:
>>>>
>>>> if(!fsn_mark) {
>>>>          ...
>>>> }
>>>> else if (create) {
>>>>          return -EEXIST;
>>>> }
>>>>
>>>> in both fanotify_add_vfsmount_mark() and fanotify_add_inode_mark()
>>>>
>>>
>>> I think that was a yes/no question and I interpret your answer as no?? >>
>>> Anyway, another yes/no question:
>>> Can you write a simple LTP test to verify the new API?
>
>
> I shall have a go at writing a test, yes.

Great.

I advice you start by forking testcases/kernel/syscalls/inotify/inotify01.c
I would verify that:
A. IN_MASK_CREATE creates a watch and events get generated
(minor flavor of inotify01.c)
B. IN_MASK_CREATE returns EEXISTS on second call for same file
C. existing mask is not modified by this attempt (events get generated
according to original mask)
D. IN_MASK_CREATE && IN_MASK_ADD return EINVAL

B-D. can be just extra steps in setup().
Do keep in mind to exit test with tst_brk(TCONF in case call
with IN_MASK_CREATE returns EINVAL


>
>>>
>>> I reccon Jan was also expecting an actual patch posted to man pages
>>> maintainer (and linux-api, which was not cc'ed on the latest patch).
>
>
> Ah, I did not know linux-api needed to be cc'ed in.
>
>>
>> Yes, and I think Henry is about to post it, just didn't get to it yet.
>
>
> For reference here is an archive link to the thread on the linux-man archive
> https://marc.info/?l=linux-man&m=152769572917930&w=2
>

OK, so I see now with the flag name change documentation patch is
out of date, but also the statement (since Linux 4.17) is incorrect

Honestly, I don't know how Michael coordinates man pages updates vs.
merged kernel patches, but the more info you provide to Michael
about were the kernel patch stands in the process is probably for the best.

Thanks,
Amir.

Re: [PATCH v3 4.17] inotify: Add flag IN_EXCL_ADD for inotify_add_watch()

[Randy Dunlap]

On 05/30/2018 08:40 AM, Amir Goldstein wrote:
> On Wed, May 30, 2018 at 4:35 PM, Henry Wilson <henry.wilson@acentic.com> wrote:
>> On 30/05/18 14:01, Jan Kara wrote:
>>>
>>> Thanks. The patch looks good. I've added it to my tree. BTW, do you plan
>>> on
>>> working on a similar addition to fanotify?
>>>
>>>                                                                 Honza
>>>
>>
>> Ah that's grand, I'm glad to have helped to improve things.
>> I'm not familiar with fanotify, however a quick look at fanotify_user.c
>> suggests that a similar approach may be taken by modifying:
>>
>> if(!fsn_mark) {
>>         ...
>> }
>> else if (create) {
>>         return -EEXIST;
>> }
>>
>> in both fanotify_add_vfsmount_mark() and fanotify_add_inode_mark()
>>
> 
> I think that was a yes/no question and I interpret your answer as no??
> 
> Anyway, another yes/no question:
> Can you write a simple LTP test to verify the new API?
> 
> I reccon Jan was also expecting an actual patch posted to man pages
> maintainer (and linux-api, which was not cc'ed on the latest patch).
> 
> About the fanotify change, since fanotify API does have 'flags' separate
> from 'mask', I am not sure if FAN_MARK_EXCL_ADD would be the
> best flag name?? Perhaps FAN_MARK_CREATE? FAN_MARK_NEW?
> not sure.
> 
> But also, I did not get a chance to comment about the chosen inotify
> flag name that the lexical proximity to IN_EXCL_UNLINK is a bit odd
> considering that _EXCL_ mean two completely different things.
> 
> Should we maybe re-consider the chosen flag name?
> Maybe include linux-api in the discussion?

Certainly (regarding linux-api).


-- 
~Randy

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

[henry.wilson]

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

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;
 
 	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;
+
 	/* verify that this is indeed an inotify instance */
 	if (unlikely(f.file->f_op != &inotify_fops)) {
 		ret = -EINVAL;
@@ -802,7 +809,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..6a24905f6e1e 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_MASK_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..884b4846b630 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_MASK_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

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

[Amir Goldstein]

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.

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

[Jan Kara]

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