| Message ID | 20230611122429.1499617-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | fsnotify: move fsnotify_open() hook into do_dentry_open() | expand |
On Sun, Jun 11, 2023 at 03:24:29PM +0300, Amir Goldstein wrote: > fsnotify_open() hook is called only from high level system calls > context and not called for the very many helpers to open files. > > This may makes sense for many of the special file open cases, but it is > inconsistent with fsnotify_close() hook that is called for every last > fput() of on a file object with FMODE_OPENED. > > As a result, it is possible to observe ACCESS, MODIFY and CLOSE events > without ever observing an OPEN event. > > Fix this inconsistency by replacing all the fsnotify_open() hooks with > a single hook inside do_dentry_open(). > > If there are special cases that would like to opt-out of the possible > overhead of fsnotify() call in fsnotify_open(), they would probably also > want to avoid the overhead of fsnotify() call in the rest of the fsnotify > hooks, so they should be opening that file with the __FMODE_NONOTIFY flag. > > However, in the majority of those cases, the s_fsnotify_connectors > optimization in fsnotify_parent() would be sufficient to avoid the > overhead of fsnotify() call anyway. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Jan, > > I found out about this problem when I tested the work to remove > FMODE_NONOTIFY from overlayfs internal files - even after I enabled > notifications on the underlying fs, the LTS tests [2] did not observe > the OPEN events. > > Because this change is independent of the ovl work and has implications > on other subsystems as well (e.g. cachefiles), I think it is better > if the change came through your tree. > > This change has a potential to regress some micro-benchmarks, so if > you could queue it up for soaking in linux-next that would be great. > > Thanks, > Amir. > > > [1] https://lore.kernel.org/linux-fsdevel/20230609073239.957184-1-amir73il@gmail.com/ > [2] https://github.com/amir73il/ltp/commits/ovl_encode_fid > > fs/exec.c | 5 ----- > fs/fhandle.c | 1 - > fs/open.c | 6 +++++- > io_uring/openclose.c | 1 - > 4 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index a466e797c8e2..238473de1ec5 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -152,8 +152,6 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > path_noexec(&file->f_path))) > goto exit; > > - fsnotify_open(file); > - > error = -ENOEXEC; > > read_lock(&binfmt_lock); > @@ -934,9 +932,6 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) > if (err) > goto exit; > > - if (name->name[0] != '\0') > - fsnotify_open(file); > - > out: > return file; > > diff --git a/fs/fhandle.c b/fs/fhandle.c > index fd0d6a3b3699..6ea8d35a9382 100644 > --- a/fs/fhandle.c > +++ b/fs/fhandle.c > @@ -242,7 +242,6 @@ static long do_handle_open(int mountdirfd, struct file_handle __user *ufh, > retval = PTR_ERR(file); > } else { > retval = fd; > - fsnotify_open(file); > fd_install(fd, file); > } > path_put(&path); > diff --git a/fs/open.c b/fs/open.c > index 4478adcc4f3a..81444ebf6091 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -969,6 +969,11 @@ static int do_dentry_open(struct file *f, > } > } > > + /* > + * Once we return a file with FMODE_OPENED, __fput() will call > + * fsnotify_close(), so we need fsnotify_open() here for symetry. s/symetry/symmetry/
On Sun 11-06-23 15:24:29, Amir Goldstein wrote: > fsnotify_open() hook is called only from high level system calls > context and not called for the very many helpers to open files. > > This may makes sense for many of the special file open cases, but it is > inconsistent with fsnotify_close() hook that is called for every last > fput() of on a file object with FMODE_OPENED. > > As a result, it is possible to observe ACCESS, MODIFY and CLOSE events > without ever observing an OPEN event. > > Fix this inconsistency by replacing all the fsnotify_open() hooks with > a single hook inside do_dentry_open(). > > If there are special cases that would like to opt-out of the possible > overhead of fsnotify() call in fsnotify_open(), they would probably also > want to avoid the overhead of fsnotify() call in the rest of the fsnotify > hooks, so they should be opening that file with the __FMODE_NONOTIFY flag. > > However, in the majority of those cases, the s_fsnotify_connectors > optimization in fsnotify_parent() would be sufficient to avoid the > overhead of fsnotify() call anyway. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Thanks! The cleanup looks nice so I've applied it with the typo fixup from Christian. I have a slight worry this might break something subtle somewhere but after searching for a while I didn't find anything and the machine boots and ltp tests pass so it's worth a try :) Honza > --- > > Jan, > > I found out about this problem when I tested the work to remove > FMODE_NONOTIFY from overlayfs internal files - even after I enabled > notifications on the underlying fs, the LTS tests [2] did not observe > the OPEN events. > > Because this change is independent of the ovl work and has implications > on other subsystems as well (e.g. cachefiles), I think it is better > if the change came through your tree. > > This change has a potential to regress some micro-benchmarks, so if > you could queue it up for soaking in linux-next that would be great. > > Thanks, > Amir. > > > [1] https://lore.kernel.org/linux-fsdevel/20230609073239.957184-1-amir73il@gmail.com/ > [2] https://github.com/amir73il/ltp/commits/ovl_encode_fid > > fs/exec.c | 5 ----- > fs/fhandle.c | 1 - > fs/open.c | 6 +++++- > io_uring/openclose.c | 1 - > 4 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index a466e797c8e2..238473de1ec5 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -152,8 +152,6 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > path_noexec(&file->f_path))) > goto exit; > > - fsnotify_open(file); > - > error = -ENOEXEC; > > read_lock(&binfmt_lock); > @@ -934,9 +932,6 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) > if (err) > goto exit; > > - if (name->name[0] != '\0') > - fsnotify_open(file); > - > out: > return file; > > diff --git a/fs/fhandle.c b/fs/fhandle.c > index fd0d6a3b3699..6ea8d35a9382 100644 > --- a/fs/fhandle.c > +++ b/fs/fhandle.c > @@ -242,7 +242,6 @@ static long do_handle_open(int mountdirfd, struct file_handle __user *ufh, > retval = PTR_ERR(file); > } else { > retval = fd; > - fsnotify_open(file); > fd_install(fd, file); > } > path_put(&path); > diff --git a/fs/open.c b/fs/open.c > index 4478adcc4f3a..81444ebf6091 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -969,6 +969,11 @@ static int do_dentry_open(struct file *f, > } > } > > + /* > + * Once we return a file with FMODE_OPENED, __fput() will call > + * fsnotify_close(), so we need fsnotify_open() here for symetry. > + */ > + fsnotify_open(f); > return 0; > > cleanup_all: > @@ -1358,7 +1363,6 @@ static long do_sys_openat2(int dfd, const char __user *filename, > put_unused_fd(fd); > fd = PTR_ERR(f); > } else { > - fsnotify_open(f); > fd_install(fd, f); > } > } > diff --git a/io_uring/openclose.c b/io_uring/openclose.c > index a1b98c81a52d..10ca57f5bd24 100644 > --- a/io_uring/openclose.c > +++ b/io_uring/openclose.c > @@ -150,7 +150,6 @@ int io_openat2(struct io_kiocb *req, unsigned int issue_flags) > > if ((issue_flags & IO_URING_F_NONBLOCK) && !nonblock_set) > file->f_flags &= ~O_NONBLOCK; > - fsnotify_open(file); > > if (!fixed) > fd_install(ret, file); > -- > 2.34.1 >
On Mon, Jun 12, 2023 at 10:54:31AM +0200, Jan Kara wrote: > On Sun 11-06-23 15:24:29, Amir Goldstein wrote: > > fsnotify_open() hook is called only from high level system calls > > context and not called for the very many helpers to open files. > > > > This may makes sense for many of the special file open cases, but it is > > inconsistent with fsnotify_close() hook that is called for every last > > fput() of on a file object with FMODE_OPENED. > > > > As a result, it is possible to observe ACCESS, MODIFY and CLOSE events > > without ever observing an OPEN event. > > > > Fix this inconsistency by replacing all the fsnotify_open() hooks with > > a single hook inside do_dentry_open(). > > > > If there are special cases that would like to opt-out of the possible > > overhead of fsnotify() call in fsnotify_open(), they would probably also > > want to avoid the overhead of fsnotify() call in the rest of the fsnotify > > hooks, so they should be opening that file with the __FMODE_NONOTIFY flag. > > > > However, in the majority of those cases, the s_fsnotify_connectors > > optimization in fsnotify_parent() would be sufficient to avoid the > > overhead of fsnotify() call anyway. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Thanks! The cleanup looks nice so I've applied it with the typo fixup from > Christian. I have a slight worry this might break something subtle > somewhere but after searching for a while I didn't find anything and the > machine boots and ltp tests pass so it's worth a try :) Yep, I agree. If we can reduce cluttering multiple places with fsnotify_open() and instead move it to a central location it's a maintenance win in the long term.
diff --git a/fs/exec.c b/fs/exec.c index a466e797c8e2..238473de1ec5 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -152,8 +152,6 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) path_noexec(&file->f_path))) goto exit; - fsnotify_open(file); - error = -ENOEXEC; read_lock(&binfmt_lock); @@ -934,9 +932,6 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) if (err) goto exit; - if (name->name[0] != '\0') - fsnotify_open(file); - out: return file; diff --git a/fs/fhandle.c b/fs/fhandle.c index fd0d6a3b3699..6ea8d35a9382 100644 --- a/fs/fhandle.c +++ b/fs/fhandle.c @@ -242,7 +242,6 @@ static long do_handle_open(int mountdirfd, struct file_handle __user *ufh, retval = PTR_ERR(file); } else { retval = fd; - fsnotify_open(file); fd_install(fd, file); } path_put(&path); diff --git a/fs/open.c b/fs/open.c index 4478adcc4f3a..81444ebf6091 100644 --- a/fs/open.c +++ b/fs/open.c @@ -969,6 +969,11 @@ static int do_dentry_open(struct file *f, } } + /* + * Once we return a file with FMODE_OPENED, __fput() will call + * fsnotify_close(), so we need fsnotify_open() here for symetry. + */ + fsnotify_open(f); return 0; cleanup_all: @@ -1358,7 +1363,6 @@ static long do_sys_openat2(int dfd, const char __user *filename, put_unused_fd(fd); fd = PTR_ERR(f); } else { - fsnotify_open(f); fd_install(fd, f); } } diff --git a/io_uring/openclose.c b/io_uring/openclose.c index a1b98c81a52d..10ca57f5bd24 100644 --- a/io_uring/openclose.c +++ b/io_uring/openclose.c @@ -150,7 +150,6 @@ int io_openat2(struct io_kiocb *req, unsigned int issue_flags) if ((issue_flags & IO_URING_F_NONBLOCK) && !nonblock_set) file->f_flags &= ~O_NONBLOCK; - fsnotify_open(file); if (!fixed) fd_install(ret, file);
fsnotify_open() hook is called only from high level system calls context and not called for the very many helpers to open files. This may makes sense for many of the special file open cases, but it is inconsistent with fsnotify_close() hook that is called for every last fput() of on a file object with FMODE_OPENED. As a result, it is possible to observe ACCESS, MODIFY and CLOSE events without ever observing an OPEN event. Fix this inconsistency by replacing all the fsnotify_open() hooks with a single hook inside do_dentry_open(). If there are special cases that would like to opt-out of the possible overhead of fsnotify() call in fsnotify_open(), they would probably also want to avoid the overhead of fsnotify() call in the rest of the fsnotify hooks, so they should be opening that file with the __FMODE_NONOTIFY flag. However, in the majority of those cases, the s_fsnotify_connectors optimization in fsnotify_parent() would be sufficient to avoid the overhead of fsnotify() call anyway. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Jan, I found out about this problem when I tested the work to remove FMODE_NONOTIFY from overlayfs internal files - even after I enabled notifications on the underlying fs, the LTS tests [2] did not observe the OPEN events. Because this change is independent of the ovl work and has implications on other subsystems as well (e.g. cachefiles), I think it is better if the change came through your tree. This change has a potential to regress some micro-benchmarks, so if you could queue it up for soaking in linux-next that would be great. Thanks, Amir. [1] https://lore.kernel.org/linux-fsdevel/20230609073239.957184-1-amir73il@gmail.com/ [2] https://github.com/amir73il/ltp/commits/ovl_encode_fid fs/exec.c | 5 ----- fs/fhandle.c | 1 - fs/open.c | 6 +++++- io_uring/openclose.c | 1 - 4 files changed, 5 insertions(+), 8 deletions(-)