Message-ID: <CAGudoHGP+x0VPpJnn=zWG6NLTkN8t+TvKDwErfWVvzZ7CEa+=Q@mail.gmail.com>
Date: Mon, 3 Nov 2025 17:44:07 +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 Mon, Nov 3, 2025 at 5:45 AM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Sun, Nov 02, 2025 at 11:42:03PM +0100, Mateusz Guzik wrote:
>
> > Even ignoring the fact that there is a refcount and people may be
> > inclined to refname(name) + take_filename(name), the following already
> > breaks:
>
> Er...  refname() doesn't need to be seen for anyone other than auditsc.c
> and core part of filename handling in fs/namei.c (I'd like to move it
> to fs/filename.c someday)...
>
> > 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.
>
> Breaks in which case, exactly?  If baz() consumes its argument, we are
> fine, if it does we have a leak...
>

My point is currently the idiomatic handling of struct filename is to
getname(), pass it around and then unconditionally call putname() on
it, which already branches on IS_ERR_OR_NULL. With the previous
proposed design it would be a matter of time before someone does that
and take_filename somewhere down the callstack, resulting in a bug.

The new alien_filename struct mostly sorts it out, but I have some notes on it.

> I agree that 'take_filename' is inviting wrong connotations, though.
>
> Hell knows - it might be worth thinking of that as claiming ownership.
> Or, perhaps, transformation of the original object, if we separate
> the notion of 'active filename' (absolutely tied to one thread, not
> allowed to be reachable from any data structures shared with other
> threads, etc.) from 'embryonic filename' (no refcounting whatsoever,
> no copying of references, etc., consumed on transformation into
> 'active filename').  Then getname_alien() would create an embryo,
> to be consumed before doing actual work.  That could be expressed
> in C type system...  Need to think about that.
>
> One possibility would be something like
>
> struct alien_filename {
>         struct filename *__dont_touch_that;
> };
>
> int getname_alien(struct alien_filename *v, const char __user *string)
> {
>         struct filename *res;
>         if (WARN_ON(v->__dont_touch_that))
>                 return -EINVAL;
>         res = getname_flags(string, GETNAME_NOAUDIT);
>         if (IS_ERR(res))
>                 return PTR_ERR(res);
>         v->__done_touch_that = res;
>         return 0;
> }
>
> void destroy_alien_filename(struct alient_filename *v)
> {
>         putname(no_free_ptr(v->__dont_touch_that));
> }
>
> struct filename *claim_filename(struct alien_filename *v)
> {
>         struct filename *res = no_free_ptr(v->__dont_touch_that);
>         if (!IS_ERR(res))
>                 audit_getname(res);
>         return res;
> }
>
> and e.g.
>
> struct io_rename {
>         struct file                     *file;
>         int                             old_dfd;
>         int                             new_dfd;
>         struct alien_filename           oldpath;
>         struct alien_filename           newpath;
>         int                             flags;
> };
>
> ...
>         err = getname_alien(&ren->oldpath);
>         if (unlikely(err))
>                 return err;
>         err = getname_alien(&ren->newpath);
>         if (unlikely(err)) {
>                 destroy_alien_filename(&ren->oldpath);
>                 return err;
>         }
>
> ...
>         /* note that do_renameat2() consumes filename references */
>         ret = do_renameat2(ren->old_dfd, claim_filename(&ren->oldpath),
>                            ren->new_dfd, claim_filename(&ren->newpath),
>                            ren->flags);
> ...
>
> void io_renameat_cleanup(struct io_kiocb *req)
> {
>         struct io_rename *ren = io_kiocb_to_cmd(req, struct io_rename);
>
>         destroy_alien_filename(&ren->oldpath);
>         destroy_alien_filename(&ren->newpath);
> }
>
> Might work...  Anyone found adding any instances of __dont_touch_that anywhere in
> the kernel would be obviously doing something fishy (and if they are playing silly
> buggers with obfuscating that, s/doing something fishy/malicious/), so C typechecking
> + git grep once in a while should suffice for safety.

I think this still wants some error-proofing to catch bad usage. Per
the above, the new thing deviates from the idiom claiming you can
always putname().

Perhaps like this:
struct alien_filename {
        struct filename *__dont_touch_that;
        struct task_struct *__who_can_free;
        bool is_delegated;
};

It would start with __who_can_free == NULL and would need to be
populated by a helper before destroy_alien_name is legally callable.

The consumer would denote it does not intend to free the obj by
calling delegate_alien_name(), after which some other thread needs to
take ownership.

a sketch:
/* called by the thread which allocated the name if it decides to go
through with it */
delegate_alien_name(name) {
    VFS_BUG_ON(name->delegated);
    name->delegated = true;
}

/* called by the thread using the name */
claim_alien_name(name) {
    VFS_BUG_ON(!name->delegated);
    VFS_BUG_ON(name->__who_can_free != NULL);
    name->__who_can_free = current;
}

destroy_alien_name(name) {
    if (name->delegated) {
        VFS_BUG_ON(name->__who_can_free == NULL);
        VFS_BUG_ON(name->__who_can_free != current);
    }
    putname(..);
}

So a sample correct consumer looks like this:
err = getname_alien(&name);
....
err = other_prep();
if (!err)
    actual_work(delegate_alien_name(name));
else
    destroy_alien_name(name);

the *other* thread which eventually works on the name:
claim_alien_name(name);
/* hard work goes here */
destroy_alien_name(name);

Sample buggy consumer which both delegated the free *and* decided free
anyway is caught.

Powered by blists - more mailing lists