linux-kernel - Re: [PATCH] fs: support filename refcount without atomics

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: <CAGudoHE+VQUtxqtc3v38XFGVojTLqiYXoBU==PFvj=A5kmMMHw@mail.gmail.com>
Date: Fri, 7 Mar 2025 23:58:50 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: brauner@...nel.org, jack@...e.cz, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, io-uring@...r.kernel.org, 
	audit@...r.kernel.org, axboe@...nel.dk
Subject: Re: [PATCH] fs: support filename refcount without atomics

On Fri, Mar 7, 2025 at 5:44 PM Mateusz Guzik <mjguzik@...il.com> wrote:
>
> On Fri, Mar 7, 2025 at 5:42 PM Al Viro <viro@...iv.linux.org.uk> wrote:
> > Not a good way to handle that, IMO.
> >
> > Atomics do hurt there, but they are only plastering over the real
> > problem - names formed in one thread, inserted into audit context
> > there and operation involving them happening in a different thread.
> >
> > Refcounting avoids an instant memory corruption, but the real PITA
> > is in audit users of that stuff.
> >
> > IMO we should *NOT* grab an audit names slot at getname() time -
> > that ought to be done explicitly at later points.
> >

I was looking at doing that, but the code is kind of a mess and I bailed.

> > The obstacle is that currently there still are several retry loop
> > with getname() done in it; I've most of that dealt with, need to
> > finish that series.
> >
> > And yes, refcount becomes non-atomic as the result.
>
> Well yes, it was audit which caused the appearance of atomics in the
> first place. I was looking for an easy way out.
>
> If you have something which gets rid of the underlying problem and it
> is going to land in the foreseeable future, I wont be defending this
> approach.
>

It is unclear to me if you are NAKing the patch, or merely pointing
out this can be done in a better way (which I agree with)

Some time ago I posted a much simpler patch to merely dodge the last
decrement [1], which already accomplishes what I was looking for.

Christian did not like it and wanted something which only deals with
atomics when audit is enabled.

I should have done that patch slightly differently, but bottom line is
the following in putname():

        refcnt = atomic_read(&name->refcnt);
        if (refcnt != 1) {
                if (WARN_ON_ONCE(!refcnt))
                        return;

                if (!atomic_dec_and_test(&name->refcnt))
                        return;
        }

So if you are NAKing the regular -> atomic switch patch, how about the
above as a quick hack until the issue gets resolved? It is trivial to
reason about (refcnt == 1 means nobody can do anything) and guarantees
to dodge one atomic (which in case of no audit means all consumers). I
can repost touched up if you are OK with it (the original posting
issues atomic_read twice).

As for the bigger patch posted here, Jens wants the io_uring bits done
differently and offered to handle them in the upcoming week. I think a
clear statement if the patch is a no-go would be appreciated.

Link 1: https://lore.kernel.org/linux-fsdevel/20240604132448.101183-1-mjguzik@gmail.com/

-- 
Mateusz Guzik <mjguzik gmail.com>