Previously: https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u
In short:
* most read/write APIs generate ACCESS/MODIFY for the read/written file(s)
* except the [vm]splice/tee family
(actually, since 6.4, splice itself /does/ generate events but only
for the non-pipes being spliced from/to; this commit is Fixes:ed)
* userspace that registers (i|fa)notify on pipes usually relies on it
actually working (coreutils tail -f is the primo example)
* it's sub-optimal when someone with a magic syscall can fill up a
pipe simultaneously ensuring it will never get serviced
Thus: actually generate ACCESS/MODIFY for all the
[vm]spliced/teed-from/to files.
LTP tests are staged in
https://git.sr.ht/~nabijaczleweli/ltp/commit/v4
("inotify13: new test for fs/splice.c functions vs pipes vs inotify"),
validating that one A and/or one M event per [vm]splice(), tee(),
and sendfile() is generated ‒
without this patchset, this only holds for sendfile().
Amir has identified a potential performance impact caused by
correctly generating events, and has prepared patches at
https://github.com/amir73il/linux/commits/fsnotify_pipe
that optimise the most common cases more aggressively.
Please review, and please consider taking these through the vfs
tree for 6.6.
Thanks,
Ahelenia Ziemiańska (3):
splice: always fsnotify_access(in), fsnotify_modify(out) on success
splice: fsnotify_access(fd)/fsnotify_modify(fd) in vmsplice
splice: fsnotify_access(in), fsnotify_modify(out) on success in tee
fs/splice.c | 43 +++++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 18 deletions(-)
--
2.39.2
Same logic applies here: this can fill up the pipe and pollers that rely
on getting IN_MODIFY notifications never wake up.
Fixes: 983652c69199 ("splice: report related fsnotify events")
Link: https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u
Link: https://bugs.debian.org/1039488
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: Amir Goldstein <[email protected]>
Acked-by: Jan Kara <[email protected]>
---
fs/splice.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/splice.c b/fs/splice.c
index 6ae6da52eba9..5deb12d743b1 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1346,6 +1346,9 @@ static long vmsplice_to_user(struct file *file, struct iov_iter *iter,
pipe_unlock(pipe);
}
+ if (ret > 0)
+ fsnotify_access(file);
+
return ret;
}
@@ -1375,8 +1378,10 @@ static long vmsplice_to_pipe(struct file *file, struct iov_iter *iter,
if (!ret)
ret = iter_to_pipe(iter, pipe, buf_flag);
pipe_unlock(pipe);
- if (ret > 0)
+ if (ret > 0) {
wakeup_pipe_readers(pipe);
+ fsnotify_modify(file);
+ }
return ret;
}
--
2.39.2
Same logic applies here: this can fill up the pipe, and pollers that rely
on getting IN_MODIFY notifications never wake up.
Fixes: 983652c69199 ("splice: report related fsnotify events")
Link: https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u
Link: https://bugs.debian.org/1039488
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: Amir Goldstein <[email protected]>
Acked-by: Jan Kara <[email protected]>
---
fs/splice.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/splice.c b/fs/splice.c
index 5deb12d743b1..c49909dbf3c5 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1815,6 +1815,11 @@ long do_tee(struct file *in, struct file *out, size_t len, unsigned int flags)
}
}
+ if (ret > 0) {
+ fsnotify_access(in);
+ fsnotify_modify(out);
+ }
+
return ret;
}
--
2.39.2
The current behaviour caused an asymmetry where some write APIs
(write, sendfile) would notify the written-to/read-from objects,
but splice wouldn't.
This affected userspace which uses inotify, most notably coreutils
tail -f, to monitor pipes.
If the pipe buffer had been filled by a splice-family function:
* tail wouldn't know and thus wouldn't service the pipe, and
* all writes to the pipe would block because it's full,
thus service was denied.
(For the particular case of tail -f this could be worked around
with ---disable-inotify.)
Fixes: 983652c69199 ("splice: report related fsnotify events")
Link: https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u
Link: https://bugs.debian.org/1039488
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: Amir Goldstein <[email protected]>
Acked-by: Jan Kara <[email protected]>
---
fs/splice.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/fs/splice.c b/fs/splice.c
index 3e06611d19ae..6ae6da52eba9 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1154,10 +1154,8 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
if ((in->f_flags | out->f_flags) & O_NONBLOCK)
flags |= SPLICE_F_NONBLOCK;
- return splice_pipe_to_pipe(ipipe, opipe, len, flags);
- }
-
- if (ipipe) {
+ ret = splice_pipe_to_pipe(ipipe, opipe, len, flags);
+ } else if (ipipe) {
if (off_in)
return -ESPIPE;
if (off_out) {
@@ -1182,18 +1180,11 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
ret = do_splice_from(ipipe, out, &offset, len, flags);
file_end_write(out);
- if (ret > 0)
- fsnotify_modify(out);
-
if (!off_out)
out->f_pos = offset;
else
*off_out = offset;
-
- return ret;
- }
-
- if (opipe) {
+ } else if (opipe) {
if (off_out)
return -ESPIPE;
if (off_in) {
@@ -1209,18 +1200,24 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
ret = splice_file_to_pipe(in, opipe, &offset, len, flags);
- if (ret > 0)
- fsnotify_access(in);
-
if (!off_in)
in->f_pos = offset;
else
*off_in = offset;
+ } else
+ return -EINVAL;
- return ret;
+ if (ret > 0) {
+ /*
+ * Generate modify out before access in:
+ * do_splice_from() may've already sent modify out,
+ * and this ensures the events get merged.
+ */
+ fsnotify_modify(out);
+ fsnotify_access(in);
}
- return -EINVAL;
+ return ret;
}
static long __do_splice(struct file *in, loff_t __user *off_in,
--
2.39.2
On Mon, Jul 3, 2023 at 5:42 PM Ahelenia Ziemiańska
<[email protected]> wrote:
>
> The current behaviour caused an asymmetry where some write APIs
> (write, sendfile) would notify the written-to/read-from objects,
> but splice wouldn't.
>
> This affected userspace which uses inotify, most notably coreutils
> tail -f, to monitor pipes.
> If the pipe buffer had been filled by a splice-family function:
> * tail wouldn't know and thus wouldn't service the pipe, and
> * all writes to the pipe would block because it's full,
> thus service was denied.
> (For the particular case of tail -f this could be worked around
> with ---disable-inotify.)
>
> Fixes: 983652c69199 ("splice: report related fsnotify events")
> Link: https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u
> Link: https://bugs.debian.org/1039488
> Signed-off-by: Ahelenia Ziemiańska <[email protected]>
> Reviewed-by: Amir Goldstein <[email protected]>
> Acked-by: Jan Kara <[email protected]>
> ---
> fs/splice.c | 31 ++++++++++++++-----------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/fs/splice.c b/fs/splice.c
> index 3e06611d19ae..6ae6da52eba9 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1154,10 +1154,8 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
> if ((in->f_flags | out->f_flags) & O_NONBLOCK)
> flags |= SPLICE_F_NONBLOCK;
>
> - return splice_pipe_to_pipe(ipipe, opipe, len, flags);
> - }
> -
> - if (ipipe) {
> + ret = splice_pipe_to_pipe(ipipe, opipe, len, flags);
> + } else if (ipipe) {
> if (off_in)
> return -ESPIPE;
> if (off_out) {
> @@ -1182,18 +1180,11 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
> ret = do_splice_from(ipipe, out, &offset, len, flags);
> file_end_write(out);
>
> - if (ret > 0)
> - fsnotify_modify(out);
> -
> if (!off_out)
> out->f_pos = offset;
> else
> *off_out = offset;
> -
> - return ret;
> - }
> -
> - if (opipe) {
> + } else if (opipe) {
> if (off_out)
> return -ESPIPE;
> if (off_in) {
> @@ -1209,18 +1200,24 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
>
> ret = splice_file_to_pipe(in, opipe, &offset, len, flags);
>
> - if (ret > 0)
> - fsnotify_access(in);
> -
> if (!off_in)
> in->f_pos = offset;
> else
> *off_in = offset;
> + } else
> + return -EINVAL;
I must have missed this in previous reviews -
mixing if {} else (without {}) is an anti pattern that causes bugs.
No need to send v6 just for this - it can be fixed up on commit.
>
> - return ret;
> + if (ret > 0) {
> + /*
> + * Generate modify out before access in:
> + * do_splice_from() may've already sent modify out,
> + * and this ensures the events get merged.
> + */
> + fsnotify_modify(out);
> + fsnotify_access(in);
> }
>
> - return -EINVAL;
> + return ret;
> }
>
> static long __do_splice(struct file *in, loff_t __user *off_in,
> --
> 2.39.2
>
On Mon, 03 Jul 2023 16:42:05 +0200, Ahelenia Ziemiańska wrote:
> Previously: https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u
>
> In short:
> * most read/write APIs generate ACCESS/MODIFY for the read/written file(s)
> * except the [vm]splice/tee family
> (actually, since 6.4, splice itself /does/ generate events but only
> for the non-pipes being spliced from/to; this commit is Fixes:ed)
> * userspace that registers (i|fa)notify on pipes usually relies on it
> actually working (coreutils tail -f is the primo example)
> * it's sub-optimal when someone with a magic syscall can fill up a
> pipe simultaneously ensuring it will never get serviced
>
> [...]
Fixed the missing single-line-{} after multi-line-{} style problem that
Amir mentioned.
---
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[1/3] splice: always fsnotify_access(in), fsnotify_modify(out) on success
https://git.kernel.org/vfs/vfs/c/cade9d70ce70
[2/3] splice: fsnotify_access(fd)/fsnotify_modify(fd) in vmsplice
https://git.kernel.org/vfs/vfs/c/6aa55b7b85b5
[3/3] splice: fsnotify_access(in), fsnotify_modify(out) on success in tee
https://git.kernel.org/vfs/vfs/c/6e7556086b19