* [PATCH 1/3] inotify: move wd lookup out of update_existing_watch()
2015-09-03 15:10 [PATCH 0/3] Introduce inotify_update_watch(2) David Herrmann
@ 2015-09-03 15:10 ` David Herrmann
2015-09-03 15:10 ` [PATCH 2/3] inotify: add inotify_update_watch() syscall David Herrmann
2015-09-03 15:10 ` [PATCH 3/3] kselftest/inotify: add inotify_update_watch(2) test-cases David Herrmann
2 siblings, 0 replies; 5+ messages in thread
From: David Herrmann @ 2015-09-03 15:10 UTC (permalink / raw)
To: linux-kernel
Cc: John McCutchan, Robert Love, Eric Paris, Andrew Morton, Al Viro,
linux-api, Shuah Khan, David Herrmann
Currently, inotify_update_existing_watch() first looks up the requested
wd before modifying it. This makes the function unsuitable for cases
where we already know the wd. Change the behavior to directly accept wd
as input and make the only caller do the lookup themself.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
fs/notify/inotify/inotify_user.c | 32 ++++++++++++--------------------
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 5b1e2a4..5a39ae8 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -513,23 +513,16 @@ static void inotify_free_mark(struct fsnotify_mark *fsn_mark)
kmem_cache_free(inotify_inode_mark_cachep, i_mark);
}
-static int inotify_update_existing_watch(struct fsnotify_group *group,
- struct inode *inode,
+static int inotify_update_existing_watch(struct fsnotify_mark *fsn_mark,
u32 arg)
{
- struct fsnotify_mark *fsn_mark;
+ struct inode *inode = fsn_mark->inode;
struct inotify_inode_mark *i_mark;
__u32 old_mask, new_mask;
__u32 mask;
int add = (arg & IN_MASK_ADD);
- int ret;
mask = inotify_arg_to_mask(arg);
-
- fsn_mark = fsnotify_find_inode_mark(group, inode);
- if (!fsn_mark)
- return -ENOENT;
-
i_mark = container_of(fsn_mark, struct inotify_inode_mark, fsn_mark);
spin_lock(&fsn_mark->lock);
@@ -555,13 +548,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
}
- /* return the wd */
- ret = i_mark->wd;
-
- /* match the get from fsnotify_find_mark() */
- fsnotify_put_mark(fsn_mark);
-
- return ret;
+ return i_mark->wd;
}
static int inotify_new_watch(struct fsnotify_group *group,
@@ -616,14 +603,19 @@ out_err:
static int inotify_update_watch(struct fsnotify_group *group, struct inode *inode, u32 arg)
{
+ struct fsnotify_mark *fsn_mark;
int ret = 0;
mutex_lock(&group->mark_mutex);
- /* try to update and existing watch with the new arg */
- ret = inotify_update_existing_watch(group, inode, arg);
- /* no mark present, try to add a new one */
- if (ret == -ENOENT)
+ fsn_mark = fsnotify_find_inode_mark(group, inode);
+ if (fsn_mark) {
+ /* update an existing watch with the new arg */
+ ret = inotify_update_existing_watch(fsn_mark, arg);
+ fsnotify_put_mark(fsn_mark);
+ } else {
+ /* no mark present, try to add a new one */
ret = inotify_new_watch(group, inode, arg);
+ }
mutex_unlock(&group->mark_mutex);
return ret;
--
2.5.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/3] inotify: add inotify_update_watch() syscall
2015-09-03 15:10 [PATCH 0/3] Introduce inotify_update_watch(2) David Herrmann
2015-09-03 15:10 ` [PATCH 1/3] inotify: move wd lookup out of update_existing_watch() David Herrmann
@ 2015-09-03 15:10 ` David Herrmann
2015-09-03 15:10 ` [PATCH 3/3] kselftest/inotify: add inotify_update_watch(2) test-cases David Herrmann
2 siblings, 0 replies; 5+ messages in thread
From: David Herrmann @ 2015-09-03 15:10 UTC (permalink / raw)
To: linux-kernel
Cc: John McCutchan, Robert Love, Eric Paris, Andrew Morton, Al Viro,
linux-api, Shuah Khan, David Herrmann
The current inotify API only provides a single function to add *and*
modify watch descriptors. There is no way to perform either operation
explicitly, but the kernel always automatically chooses what to do. If
the watch-descriptor exists, it is updated, otherwise a new descriptor is
allocated. This has quite nasty side-effects:
Imagine the case where an application monitors two independent files A
and B with two independent watch descriptors. If you now want to *change*
the watch-mask of A, you have to use inotify_add_watch(fd, "A", new_mask).
However, this might race with a file-system operation that links B over A,
thus this call to inotify_add_watch() will affect the watch-descriptor of
B. However, this is usually not what the caller wants, as the watch-masks
of A and B can be disjoint, and as such an unwanted update of B might
cause event loss. Hence, a call like inotify_update_watch() is needed,
which explicitly takes the watch-descriptor to modify. In this case, it
would still only update the watch-descriptor of A, even though the path
to A changed.
The underlying issue here is the automatism of inotify_add_watch(), which
does not allow the caller to distinguish an update operation from an ADD
operation. This race could be solved with a simple IN_EXCL (or IN_CREATE)
flag, which would cause inotify_add_watch() to *never* update existing
watch-descriptors, but fail with EEXIST instead. However, this still
prevents the caller from *updating* the flags of an explicitly passed
watch-descriptor. Furthermore, the fact that inotify objects identify
*INODES*, but the API takes *PATHS* calls for races. Therefore, we really
need an explicit update operation to allow user-space to modify watch
descriptors without having to re-create them and thus invalidating their
cache.
This patch implements inotify_update_watch() to extend the inotify API
with a way to explicity modify watch-descriptors, instead of going via
the file-system path-API of inotify_add_watch().
SYNOPSIS
#include <sys/inotify.h>
int inotify_update_watch(int fd, __u32 wd, __u32 mask);
DESCRIPTION
inotify_update_watch() modifies an existing inotify watch descriptor,
specified by 'wd', which was previously added via
inotify_add_watch(2). It updates the mask of events to be monitored
via this watch descriptor according to the event mask specified by
'mask'. If IN_MASK_ADD is passed, 'mask' is added to the existing set
of flags on this watch descriptor, otherwise the existing mask is
replaced by the new mask. See inotify(7) for a description of the
further bits allowed in 'mask'.
Flags that modify the file lookup behavior of inotify_add_watch(2)
(IN_ONLYDIR, IN_DONT_FOLLOW) cannot be passed to
inotify_update_watch(). They will be rejected with EINVAL.
RETURN VALUE
On success, 0 is returned. On error, -1 is returned, and errno is
set appropriately.
ERRORS
EBADF 'fd' is not a valid file descriptor.
EINVAL 'fd' is not an inotify file descriptor; or 'mask' contains
invalid or unsupported flags.
ENXIO 'wd' is not a valid watch descriptor on this inotify
instance.
CONFORMING TO
This system call is Linux-specific.
SEE ALSO
inotify(7), inotify_init(2), inotify_add_watch(2), inotify_rm_watch(2)
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
fs/notify/inotify/inotify_user.c | 37 ++++++++++++++++++++++++++++++++++
include/linux/syscalls.h | 1 +
kernel/sys_ni.c | 1 +
5 files changed, 41 insertions(+)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index ef8187f..598b6cc 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -365,3 +365,4 @@
356 i386 memfd_create sys_memfd_create
357 i386 bpf sys_bpf
358 i386 execveat sys_execveat stub32_execveat
+359 i386 inotify_update_watch sys_inotify_update_watch
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 9ef32d5..883a02e 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -329,6 +329,7 @@
320 common kexec_file_load sys_kexec_file_load
321 common bpf sys_bpf
322 64 execveat stub_execveat
+323 common inotify_update_watch sys_inotify_update_watch
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 5a39ae8..1df7312 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -733,6 +733,43 @@ fput_and_out:
return ret;
}
+SYSCALL_DEFINE3(inotify_update_watch, int, fd, __s32, wd, __u32, mask)
+{
+ struct inotify_inode_mark *mark;
+ struct fsnotify_group *group;
+ struct fd f;
+ int ret = 0;
+
+ /* disallow unknown flags and flags specific to inode lookup */
+ if (unlikely(mask & (IN_DONT_FOLLOW |
+ IN_ONLYDIR |
+ ~ALL_INOTIFY_BITS)))
+ return -EINVAL;
+
+ f = fdget(fd);
+ if (unlikely(!f.file))
+ return -EBADF;
+ if (unlikely(f.file->f_op != &inotify_fops)) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ group = f.file->private_data;
+ mark = inotify_idr_find(group, wd);
+ if (unlikely(!mark)) {
+ ret = -ENXIO;
+ goto exit;
+ }
+
+ mutex_lock(&group->mark_mutex);
+ inotify_update_existing_watch(&mark->fsn_mark, mask);
+ mutex_unlock(&group->mark_mutex);
+
+exit:
+ fdput(f);
+ return ret;
+}
+
SYSCALL_DEFINE2(inotify_rm_watch, int, fd, __s32, wd)
{
struct fsnotify_group *group;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index b45c45b..40701b0 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -744,6 +744,7 @@ asmlinkage long sys_inotify_init(void);
asmlinkage long sys_inotify_init1(int flags);
asmlinkage long sys_inotify_add_watch(int fd, const char __user *path,
u32 mask);
+asmlinkage long sys_inotify_update_watch(int fd, __s32 wd, __u32 mask);
asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd);
asmlinkage long sys_spu_run(int fd, __u32 __user *unpc,
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 7995ef5..b556a33d 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -114,6 +114,7 @@ cond_syscall(compat_sys_socketcall);
cond_syscall(sys_inotify_init);
cond_syscall(sys_inotify_init1);
cond_syscall(sys_inotify_add_watch);
+cond_syscall(sys_inotify_update_watch);
cond_syscall(sys_inotify_rm_watch);
cond_syscall(sys_migrate_pages);
cond_syscall(sys_move_pages);
--
2.5.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/3] kselftest/inotify: add inotify_update_watch(2) test-cases
2015-09-03 15:10 [PATCH 0/3] Introduce inotify_update_watch(2) David Herrmann
2015-09-03 15:10 ` [PATCH 1/3] inotify: move wd lookup out of update_existing_watch() David Herrmann
2015-09-03 15:10 ` [PATCH 2/3] inotify: add inotify_update_watch() syscall David Herrmann
@ 2015-09-03 15:10 ` David Herrmann
2015-09-04 6:42 ` Michael Ellerman
2 siblings, 1 reply; 5+ messages in thread
From: David Herrmann @ 2015-09-03 15:10 UTC (permalink / raw)
To: linux-kernel
Cc: John McCutchan, Robert Love, Eric Paris, Andrew Morton, Al Viro,
linux-api, Shuah Khan, David Herrmann
This adds inotify/ to the kselftests suite. It currently only includes a
test for the new inotify_update_watch(2) call, to make sure it actually
behaves like it should.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/inotify/.gitignore | 2 +
tools/testing/selftests/inotify/Makefile | 14 ++++
tools/testing/selftests/inotify/test_inotify.c | 105 +++++++++++++++++++++++++
4 files changed, 122 insertions(+)
create mode 100644 tools/testing/selftests/inotify/.gitignore
create mode 100644 tools/testing/selftests/inotify/Makefile
create mode 100644 tools/testing/selftests/inotify/test_inotify.c
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 24ae9e8..9e9b9cf 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -5,6 +5,7 @@ TARGETS += exec
TARGETS += firmware
TARGETS += ftrace
TARGETS += futex
+TARGETS += inotify
TARGETS += kcmp
TARGETS += memfd
TARGETS += memory-hotplug
diff --git a/tools/testing/selftests/inotify/.gitignore b/tools/testing/selftests/inotify/.gitignore
new file mode 100644
index 0000000..ab7299d
--- /dev/null
+++ b/tools/testing/selftests/inotify/.gitignore
@@ -0,0 +1,2 @@
+test_inotify
+test_file
diff --git a/tools/testing/selftests/inotify/Makefile b/tools/testing/selftests/inotify/Makefile
new file mode 100644
index 0000000..c205abb
--- /dev/null
+++ b/tools/testing/selftests/inotify/Makefile
@@ -0,0 +1,14 @@
+CC = $(CROSS_COMPILE)gcc
+CFLAGS += -I../../../../include/uapi/
+CFLAGS += -I../../../../include/
+CFLAGS += -I../../../../usr/include/
+
+all:
+ $(CC) $(CFLAGS) test_inotify.c -o test_inotify
+
+TEST_PROGS := test_inotify
+
+include ../lib.mk
+
+clean:
+ $(RM) test_inotify test_file
diff --git a/tools/testing/selftests/inotify/test_inotify.c b/tools/testing/selftests/inotify/test_inotify.c
new file mode 100644
index 0000000..094420a
--- /dev/null
+++ b/tools/testing/selftests/inotify/test_inotify.c
@@ -0,0 +1,105 @@
+/*
+ * inotify tests
+ */
+
+#define _GNU_SOURCE
+#define __EXPORTED_HEADERS__
+
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <limits.h>
+#include <linux/unistd.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/inotify.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+
+#define TEST_FILE "./test_file"
+
+static int sys_inotify_update_watch(int fd, uint32_t wd, uint32_t mask)
+{
+ return syscall(__NR_inotify_update_watch, fd, wd, mask);
+}
+
+static int read_inotify(int fd, struct inotify_event **out)
+{
+ union {
+ struct inotify_event event;
+ char buffer[sizeof(struct inotify_event) + NAME_MAX + 1];
+ } buf;
+ int r;
+
+ r = read(fd, &buf, sizeof(buf));
+ if (r < 0)
+ return r;
+
+ assert(r >= sizeof(struct inotify_event) && r <= sizeof(buf));
+ *out = &buf.event;
+ return r;
+}
+
+/* test inotify_update_watch(2) */
+static void test_update(void)
+{
+ struct inotify_event *event;
+ int ifd, wd, fd, r;
+
+ ifd = inotify_init1(IN_CLOEXEC | IN_NONBLOCK);
+ assert(ifd >= 0);
+
+ unlink(TEST_FILE);
+
+ /* try adding watch on non-existant file */
+ wd = inotify_add_watch(ifd, TEST_FILE, IN_IGNORED);
+ assert(wd < 0 && errno == ENOENT);
+
+ /* create file */
+ fd = open(TEST_FILE, O_CREAT | O_CLOEXEC | O_EXCL, S_IRUSR);
+ assert(fd >= 0);
+
+ /* add watch descriptor without IN_CLOSE */
+ wd = inotify_add_watch(ifd, TEST_FILE, IN_IGNORED);
+ assert(wd > 0);
+
+ /* close file */
+ close(fd);
+
+ /* should not have gotten any event */
+ r = read_inotify(ifd, &event);
+ assert(r < 0 && errno == EAGAIN);
+
+ /* reopen file */
+ fd = open(TEST_FILE, O_CLOEXEC | O_EXCL);
+ assert(fd >= 0);
+
+ /* invalid flags must result in EINVAL */
+ r = sys_inotify_update_watch(ifd, wd, -1);
+ assert(r < 0 && errno == EINVAL);
+
+ /* update watch descriptor with IN_CLOSE */
+ r = sys_inotify_update_watch(ifd, wd, IN_MASK_ADD | IN_CLOSE);
+ assert(r >= 0);
+
+ /* close file */
+ close(fd);
+
+ /* now we should have seen the event */
+ r = read_inotify(ifd, &event);
+ assert(r >= 0);
+ assert(event->wd == wd);
+ assert(event->mask & IN_CLOSE);
+
+ unlink(TEST_FILE);
+ close(ifd);
+}
+
+int main(int argc, char **argv)
+{
+ test_update();
+ return 0;
+}
--
2.5.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 3/3] kselftest/inotify: add inotify_update_watch(2) test-cases
2015-09-03 15:10 ` [PATCH 3/3] kselftest/inotify: add inotify_update_watch(2) test-cases David Herrmann
@ 2015-09-04 6:42 ` Michael Ellerman
0 siblings, 0 replies; 5+ messages in thread
From: Michael Ellerman @ 2015-09-04 6:42 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, John McCutchan, Robert Love, Eric Paris,
Andrew Morton, Al Viro, linux-api, Shuah Khan
On Thu, 2015-09-03 at 17:10 +0200, David Herrmann wrote:
> diff --git a/tools/testing/selftests/inotify/Makefile b/tools/testing/selftests/inotify/Makefile
> new file mode 100644
> index 0000000..c205abb
> --- /dev/null
> +++ b/tools/testing/selftests/inotify/Makefile
> @@ -0,0 +1,14 @@
> +CC = $(CROSS_COMPILE)gcc
You don't need this, it's in lib.mk already.
> +CFLAGS += -I../../../../include/uapi/
> +CFLAGS += -I../../../../include/
Please don't include the unexported kernel headers in userspace programs.
It will sometimes work, but often not.
> +CFLAGS += -I../../../../usr/include/
This is good, it allows the test to build against the exported kernel headers, in
their default install location (usr/include). To install them run 'make
headers_install' in the top-level of the kernel tree.
It is sufficient to get the new syscall numbers you need, and is usually safe.
> +all:
> + $(CC) $(CFLAGS) test_inotify.c -o test_inotify
> +
> +TEST_PROGS := test_inotify
You don't need to spell out the all rule, this is sufficient:
TEST_PROGS := test_inotify
all: $(TEST_PROGS)
> +
> +include ../lib.mk
> +
> +clean:
> + $(RM) test_inotify test_file
You should either add a leading "-", or make this rm -f, so that when the files
don't exist rm doesn't give you an error.
> diff --git a/tools/testing/selftests/inotify/test_inotify.c b/tools/testing/selftests/inotify/test_inotify.c
> new file mode 100644
> index 0000000..094420a
> --- /dev/null
> +++ b/tools/testing/selftests/inotify/test_inotify.c
> @@ -0,0 +1,105 @@
> +/*
> + * inotify tests
> + */
> +
> +#define _GNU_SOURCE
> +#define __EXPORTED_HEADERS__
Please don't do that.
> +
> +#include <assert.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <limits.h>
> +#include <linux/unistd.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/inotify.h>
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +
> +#define TEST_FILE "./test_file"
> +
> +static int sys_inotify_update_watch(int fd, uint32_t wd, uint32_t mask)
> +{
> + return syscall(__NR_inotify_update_watch, fd, wd, mask);
> +}
> +
> +static int read_inotify(int fd, struct inotify_event **out)
> +{
> + union {
> + struct inotify_event event;
> + char buffer[sizeof(struct inotify_event) + NAME_MAX + 1];
> + } buf;
> + int r;
> +
> + r = read(fd, &buf, sizeof(buf));
> + if (r < 0)
> + return r;
> +
> + assert(r >= sizeof(struct inotify_event) && r <= sizeof(buf));
> + *out = &buf.event;
> + return r;
> +}
> +
> +/* test inotify_update_watch(2) */
> +static void test_update(void)
> +{
> + struct inotify_event *event;
> + int ifd, wd, fd, r;
> +
> + ifd = inotify_init1(IN_CLOEXEC | IN_NONBLOCK);
> + assert(ifd >= 0);
> +
> + unlink(TEST_FILE);
> +
> + /* try adding watch on non-existant file */
> + wd = inotify_add_watch(ifd, TEST_FILE, IN_IGNORED);
> + assert(wd < 0 && errno == ENOENT);
> +
> + /* create file */
> + fd = open(TEST_FILE, O_CREAT | O_CLOEXEC | O_EXCL, S_IRUSR);
> + assert(fd >= 0);
> +
> + /* add watch descriptor without IN_CLOSE */
> + wd = inotify_add_watch(ifd, TEST_FILE, IN_IGNORED);
> + assert(wd > 0);
> +
> + /* close file */
> + close(fd);
> +
> + /* should not have gotten any event */
> + r = read_inotify(ifd, &event);
> + assert(r < 0 && errno == EAGAIN);
> +
> + /* reopen file */
> + fd = open(TEST_FILE, O_CLOEXEC | O_EXCL);
> + assert(fd >= 0);
> +
> + /* invalid flags must result in EINVAL */
> + r = sys_inotify_update_watch(ifd, wd, -1);
> + assert(r < 0 && errno == EINVAL);
> +
> + /* update watch descriptor with IN_CLOSE */
> + r = sys_inotify_update_watch(ifd, wd, IN_MASK_ADD | IN_CLOSE);
> + assert(r >= 0);
> +
> + /* close file */
> + close(fd);
> +
> + /* now we should have seen the event */
> + r = read_inotify(ifd, &event);
> + assert(r >= 0);
> + assert(event->wd == wd);
> + assert(event->mask & IN_CLOSE);
> +
> + unlink(TEST_FILE);
> + close(ifd);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + test_update();
> + return 0;
> +}
You may as well just drop test_update() and put the whole test in main() ?
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread