linux-kernel - Re: Pending splice(file -> FIFO) excludes all other FIFO operations forever (was: ... always blocks read(FIFO), regardless of O

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wimmqG_wvSRtMiKPeGGDL816n65u=Mq2+H3-=uM2U6FmA@mail.gmail.com>
Date:   Sat, 8 Jul 2023 13:06:56 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Ahelenia Ziemiańska 
        <nabijaczleweli@...ijaczleweli.xyz>
Cc:     Christian Brauner <brauner@...nel.org>,
        Jens Axboe <axboe@...nel.dk>,
        David Howells <dhowells@...hat.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: Pending splice(file -> FIFO) excludes all other FIFO operations
 forever (was: ... always blocks read(FIFO), regardless of O_NONBLOCK on read side?)

On Fri, 7 Jul 2023 at 17:30, Ahelenia Ziemiańska
<nabijaczleweli@...ijaczleweli.xyz> wrote:
>
> Same reproducer, backtrace attached:
> $ scripts/faddr2line vmlinux splice_from_pipe_next+0x6e
> splice_from_pipe_next+0x6e/0x180:
> pipe_buf_confirm at include/linux/pipe_fs_i.h:233

Bah. I should have looked more closely at your case.

This is a buffer without an 'ops' pointer. So it looks like was
already released.

And the reason is that the pipe was readable because there were no
writers, and I had put the

        if (!pipe->writers)
                return 0;

check in splice_from_pipe_next() in the wrong place. It needs to be
*before* the eat_empty_buffer() call.

Duh.

Anyway, while I think that fixes your NULL pointer thing, having
looked at my original patch I realized that keeping the pointer to the
pipe buffer in copy_splice_read() across the dropping of the pipe lock
is completely broken.

I thought (because I didn't remember the code) that pipe resizing will
just copy the pipe buffer pointers around. That would have made it ok
to remember a pipe buffer pointer. But it is not so. It will actually
create new pipe buffers and copy the buffer contents around.

So while fixing your NULL pointer check should be trivial, I think
that first patch is actually fundamentally broken wrt pipe resizing,
and I see no really sane way to fix it. We could add a new lock just
for that, but I don't think it's worth it.

> You are, but, well, that's also the case when the pipe is full.
> As it stands, the pipe is /empty/ and yet /no-one can write to it/.
> This is the crux of the issue at hand.

No, I think you are mis-representing things. The pipe isn't empty.
It's full of things that just aren't finalized yet.

> Or, rather: splice() from a non-seekable (non-mmap()pable?)

Please stop with the non-seekable nonsense.

Any time I see a patch like this:

> +               if (!(in->f_mode & FMODE_LSEEK))
> +                       return -EINVAL;

I will just go "that person is not competent".

This has absolutely nothing to do with seekability.

But it is possible that we need to just bite the bullet and say
"copy_splice_read() needs to use a non-blocking kiocb for the IO".

Of course, that then doesn't work, because while doing this is trivial:

  --- a/fs/splice.c
  +++ b/fs/splice.c
  @@ -364,6 +364,7 @@ ssize_t copy_splice_read(struct file *in, loff_t *ppos,
        iov_iter_bvec(&to, ITER_DEST, bv, npages, len);
        init_sync_kiocb(&kiocb, in);
        kiocb.ki_pos = *ppos;
  +     kiocb.ki_flags |= IOCB_NOWAIT;
        ret = call_read_iter(in, &kiocb, &to);

        if (ret > 0) {

I suspectr you'll find that it makes no difference, because the tty
layer doesn't actually honor the IOCB_NOWAIT flag for various
historical reasons. In fact, the kiocb isn't even passed down to the
low-level routine, which only gets the 'struct file *', and instead it
looks at tty_io_nonblock(), which just does that legacy

        file->f_flags & O_NONBLOCK

test.

I guess combined with something like

        if (!(in->f_mode & FMODE_NOWAIT))
                return -EINVAL;

it might all work.

                Linus