linux-kernel - Re: [GIT PULL] pipe: nonblocking rw for io

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHk-=wiaFUoHpztu6Zf_4pyzH-gzeJhdCU0MYNw9LzVg1-kx8g@mail.gmail.com>
Date:   Mon, 24 Apr 2023 20:16:29 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Jens Axboe <axboe@...nel.dk>
Cc:     Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] pipe: nonblocking rw for io_uring

On Mon, Apr 24, 2023 at 3:45 PM Jens Axboe <axboe@...nel.dk> wrote:
>
> Something like this. Not tested yet, but wanted to get your feedback
> early to avoid issues down the line.

Ok, that try_cmpxchg() loop looks odd, but I guess it's the right thing to do.

That said, you should move the

        old_fmode = READ_ONCE(file->f_mode);

to outside the loop, because try_cmpxchg() will then update
'old_fmode' to the proper value and it should *not* be done again.

I also suspect that the READ_ONCE() itself is entirely superfluous,
because that is very much a value that we should let the compiler
optimize to *not* have to access more than it needs to.

So if the compiler had an earlier copy of that value, it should just
use it, rather than us forcing it to read it again.

But I suspect in this case it makes no real difference to code
generation. There's nothing else around it that uses f_mode, afaik,
and the try_cmpxchg() will reload it properly to fix any possible
races up.

The READ_ONCE() would possibly make more sense if we actually expected
that FMODE_NOWAIT bit to change more than once, but then we'd
presuably have some ordering rule and it should be a
smp_load_acquire() or whatever.

As it is, if we ever see it clear, we don't care any more, and the
real value consistency guarantee is in the try_cmpxchg itself. There
are no possible races ot mis-readings that could matter.

So I think it could/should just be something like

    void pipe_clear_nowait(struct file *file)
    {
        fmode_t fmode = file->f_mode;
        do {
            if (!(fmode & FMODE_NOWAIT))
                break;
        } while (!try_cmpxchg(&file->f_mode, &fmode, fmode & ~FMODE_NOWAIT));
    }

which sadly generates that big constant just because FMODE_NOWAIT is
up in the high bits and with the 'try_cmpxchg()', the compiler has no
choice but to get the full 32-bit value anyway.

               Linus