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