Message-ID: <CAOQ4uxhut2NHc+MY-XOJay5B-OKXU2X5Fe0-6-RCMKt584ft5A@mail.gmail.com>
Date:   Mon, 26 Jun 2023 09:11:53 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Ahelenia Ziemiańska 
        <nabijaczleweli@...ijaczleweli.xyz>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jan Kara <jack@...e.cz>
Subject: Re: splice(-> FIFO) never wakes up inotify IN_MODIFY?

On Mon, Jun 26, 2023 at 6:54 AM Ahelenia Ziemiańska
<nabijaczleweli@...ijaczleweli.xyz> wrote:
>
> Hi!
>
> Consider the following programs:
> -- >8 --
> ==> ino.c <==
> #define _GNU_SOURCE
> #include <stdio.h>
> #include <sys/inotify.h>
> #include <unistd.h>
> int main() {
>   int ino = inotify_init1(IN_CLOEXEC);
>   inotify_add_watch(ino, "/dev/fd/0", IN_MODIFY);
>
>   char buf[64 * 1024];
>   struct inotify_event ev;
>   while (read(ino, &ev, sizeof(ev)) > 0) {
>     fprintf(stderr, "%d: mask=%x, cook=%x, len=%x, name=%.*s\n", ev.wd, ev.mask,
>             ev.cookie, ev.len, (int)ev.len, ev.name);
>     fprintf(stderr, "rd=%zd\n", read(0, buf, sizeof(buf)));
>   }
> }
>

That's a very odd (and wrong) way to implement poll(2).
This is not a documented way to use pipes, so it may
happen to work with sendfile(2), but there is no guarantee.

> ==> se.c <==
> #define _GNU_SOURCE
> #include <stdio.h>
> #include <sys/sendfile.h>
> int main() {
>   ssize_t rd, acc = 0;
>   while ((rd = sendfile(1, 0, 0, 128 * 1024 * 1024)) > 0)
>     acc += rd;
>   fprintf(stderr, "se=%zd: %m\n", acc);
> }
>
> ==> sp.c <==
> #define _GNU_SOURCE
> #include <fcntl.h>
> #include <stdio.h>
> int main() {
>   ssize_t rd, acc = 0;
>   while ((rd = splice(0, 0, 1, 0, 128 * 1024 * 1024, 0)) > 0)
>     acc += rd;
>   fprintf(stderr, "sp=%zd: %m\n", acc);
> }
> -- >8 --
>
> By all means, ./sp | ./ino and ./se | ./ino should be equivalent,
> right?
>

Maybe it should, but it's not.

> -- >8 --
> $ make se sp ino
> $ mkfifo fifo
> $ ./ino < fifo &
> [1] 230
> $ echo a > fifo
> $ echo a > fifo
> 1: mask=2, cook=0, len=0, name=
> rd=4
> $ echo c > fifo
> 1: mask=2, cook=0, len=0, name=
> rd=2
> $ ./se > fifo
> abcdef
> 1: mask=2, cook=0, len=0, name=
> asd
> ^D
> se=11: Success
> rd=11
> 1: mask=2, cook=0, len=0, name=
> rd=0
> $ ./sp > fifo
> abcdefg
> asd
> dsasdadadad
> sp=24: Success
> $ < sp ./sp > fifo
> sp=25856: Success
> $ < sp ./sp > fifo
> ^C
> $ echo sp > fifo
> ^C
> -- >8 --
>
> Note how in all ./sp > fifo cases, ./ino doesn't wake up!
> Note also how, thus, we've managed to fill the pipe buffer with ./sp
> (when it transferred 25856), and now we can't /ever/ write there again
> (both splicing and normal writes block, since there's no space left in
>  the pipe; ./ino hasn't seen this and will never wake up or service the
>  pipe):
> so we've effectively "denied service" by slickily using a different
> syscall to do the write, right?
>

Only applications that do not check for availability
of input in the pipe correctly will get "denied service".

> I consider this to be unexpected behaviour because (a) obviously and
> (b) sendfile() sends the inotify event.
>

The fact is that relying on inotify IN_MODIFY and IN_ACCESS events
for pipes is not a good idea.

splice(2) differentiates three different cases:
        if (ipipe && opipe) {
...
        if (ipipe) {
...
        if (opipe) {
...

IN_ACCESS will only be generated for non-pipe input
IN_MODIFY will only be generated for non-pipe output

Similarly FAN_ACCESS_PERM fanotify permission events
will only be generated for non-pipe input.

sendfile(2) OTOH does not special cases the pipe input
case at all and it generates IN_MODIFY for the pipe output
case as well.

If you would insist on fixing this inconsistency, I would be
willing to consider a patch that matches sendfile(2) behavior
to that of splice(2) and not the other way around.

My general opinion about IN_ACCESS/IN_MODIFY
(as well as FAN_ACCESS_PERM) is that they are not
very practical, not well defined for pipes and anyway do
not cover all the ways that a file can be modified/accessed
(i.e. mmap). Therefore, IMO, there is no incentive to fix
something that has been broken for decades unless
you have a very real use case - not a made up one.

Incidentally, I am working on a new set of fanotify
permission events (FAN_PRE_ACCESS/MODIFY)
that will have better defined semantics - those are not
going to be applicable to pipes though.

Thanks,
Amir.

Powered by blists - more mailing lists