Re: [PATCH] fs: touch up predicts in putname()

[Al Viro <viro@zeniv.linux.org.uk>]

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...

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.