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

Henry Wilson <henry.wilson@xxxxxxxxxxx> · Tue, 29 May 2018 12:03:21 +0100

On 29/05/18 10:15, Jan Kara wrote:
On Wed 16-05-18 10:44:30, henry.wilson@xxxxxxxxxxx wrote:
From: Henry Wilson <henry.wilson@xxxxxxxxxxx>

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

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







