linux-kernel - Re: [GIT PULL] vfs fixes

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-=whBXdLJ=QDpYmDEH-Tn71dXasGJSX4Jz4qMo8V4-7vYkg@mail.gmail.com>
Date:   Thu, 19 Oct 2023 09:37:24 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Christian Brauner <brauner@...nel.org>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] vfs fixes

On Thu, 19 Oct 2023 at 03:09, Christian Brauner <brauner@...nel.org> wrote:
>
> An openat() call from io_uring triggering an audit call can apparently
> cause the refcount of struct filename to be incremented from multiple
> threads concurrently during async execution, triggering a refcount
> underflow and hitting a BUG_ON(). That bug has been lurking around since
> at least v5.16 apparently.

Ouch. That filename ref by audit was always supposed to be
thread-local in a "for this system call" kind of sense.

But yes, looks like the io_uring stuff ended up making it no longer
thread-local.

That said, using atomics for reference counting is our default
behavior and should be normal, so the patch isn't wrong, it's just
annoying since getname/putname is very much in the critical path of
filename handling.

That said, the extra atomics are hopefully not really noticeable.

Some people might want to use the non-refcounted version (ie we have
getname/putname used by ksmbd too, for example), if they really care.

It already exists, as __getname/__putname.

But the normal open/stat/etc system call paths are obviously now going
to hit those extra atomics. Not lovely, but I guess it's the best we
can do.

> Switch to an atomic counter to fix that. The underflow check is
> downgraded from a BUG_ON() to a WARN_ON_ONCE() but we could easily
> remove that check altogether tbh and not waste an additional atomic. So
> if you feel that extra check isn't needed you could just remove in case
> you're pulling.

Well, the atomic *read* is cheap - the expensive part is the
atomic_dec_and_test() (and the atomic_inc in the audit code.

I'm not sure why you made it check just for zero in the WARN_ON_ONCE,
rather than <= 0 as it used to, but that check is racy regardless, so
it doesn't matter. It would miss two concurrent decrements coming in
with a count of 1.

We don't have the ternary test of atomic decrement results (positive,
zero or negative), so it is what it is.

                 Linus