Message-ID: <CAGudoHFDAPEYoC8RAPuPVkcsHsgpdJtQh91=8wRgMAozJyYf2w@mail.gmail.com>
Date: Sun, 2 Nov 2025 23:42:03 +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
Subject: Re: [PATCH] fs: touch up predicts in putname()

On Sun, Nov 2, 2025 at 7:14 AM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Sat, Nov 01, 2025 at 09:19:21AM +0100, Mateusz Guzik wrote:
> > On Sat, Nov 1, 2025 at 7:05 AM Al Viro <viro@...iv.linux.org.uk> wrote:
> > >
> > > On Fri, Oct 31, 2025 at 08:17:53PM +0000, Al Viro wrote:
> > >
> > > > 0) get rid of audit_reusename() and aname->uptr (I have that series,
> > > > massaging it for posting at the moment).  Basically, don't have
> > > > getname et.al. called in retry loops - there are few places doing
> > > > that, and they are not hard to fix.
> > >
> > > See #work.filename-uptr; I'll post individual patches tomorrow morning,
> > > hopefully along with getname_alien()/take_filename() followups, including
> > > the removal of atomic (still not settled on the calling conventions for
> > > getname_alien()).
> > >
> >
> > Ok, in that case I think it will be most expedient if my patch gets
> > dropped and you just fold the updated predicts into your patchset
> > somewhere. I don't need any credit.
>
> See #work.filename-refcnt.  I'm not entirely happy about the API, if you
> see a saner way to do it, I'd really like to hear it.  Stuff in the series:
>
>         * get rid of getname in retry loops.  Only 9 places like that left,
> massaged out of existence one by one.  (##1..9)
>         * drop audit_reusename() and filename->uptr (#10)
>         * get rid of mixing LOOKUP_EMPTY with the rest of the flags -
> very few places do that at this point and they are not hard to take
> care of (##11..15)
>         * take LOOKUP_EMPTY out of LOOKUP_... space entirely - make it
> GETNAME_EMPTY and have it passed only to getname_flags() (#16)
>         * add GETNAME_NOAUDIT for "don't call audit_getname() there" (#17).
> Helpers: getname_alien()/getname_uflags_alien() being wrappers for
> that; io-uring switched to those for filename import (in ->prep()).
> take_filename(): take a reference to struct filename, leaving NULL
> behind, feed it to audit_getname() and return to caller.   Used by
> io-uring ->issue() instances that feed an imported filename to
> do_{mkdir,mknod...}() - the stuff that does actual work, done in the
> thread that will do that work.
>         * make filename->refcnt non-atomic; now it can be done (#19,
> on top of merge from vfs-common/vfs-6.19.misc to bring your commit
> in).

I think the take_filename business invites misuse in the long run and
the API has no way of pointing out it happened.

Even ignoring the fact that there is a refcount and people may be
inclined to refname(name) + take_filename(name), the following already
breaks:

foo() {
    name = getname(...);
    if (!IS_ERR_OR_NULL(name))
        bar(name);
    putname(name);
}

bar(struct filename *name)
{
    baz(take_filename(&name));
}

While the code as proposed in the branch does not do it, it is a
matter of time before something which can be distilled to the above
shows up.

I think the core idea of having io_uring bugger off from freeing the
filename thing has legs. I *suspect* the way forward is to implement
audit_delegate_free() or similar which would assert refcount == 1 and
would denote with a flag that audit takes ownership of freeing. Then
the regular putname() yells the flag when compiled with
CONFIG_DEBUG_VFS, catching regular misuse. audit itself, when done
with the buffer, would clear the flag and calls putname().

This is from top of my head, I would need to dig into it to validate
the above is feasible.

Powered by blists - more mailing lists