Re: [PATCH] fs: support filename refcount without atomics

[Mateusz Guzik <mjguzik@gmail.com>]

On Fri, Mar 7, 2025 at 5:44 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Fri, Mar 7, 2025 at 5:42 PM Al Viro <viro@zeniv.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>