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

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

[Mateusz Guzik]

1. we already expect the refcount is 1.
2. path creation predicts name == iname

I verified this straightens out the asm, no functional changes.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

random annoyance i noticed while profiling

 fs/namei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index ba29ec7b67c5..4692f25e7cd9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -282,7 +282,7 @@ void putname(struct filename *name)
 		return;
 
 	refcnt = atomic_read(&name->refcnt);
-	if (refcnt != 1) {
+	if (unlikely(refcnt != 1)) {
 		if (WARN_ON_ONCE(!refcnt))
 			return;
 
@@ -290,7 +290,7 @@ void putname(struct filename *name)
 			return;
 	}
 
-	if (name->name != name->iname) {
+	if (unlikely(name->name != name->iname)) {
 		__putname(name->name);
 		kfree(name);
 	} else
-- 
2.34.1

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

[Markus Elfring]

…
> I verified this straightens out the asm, no functional changes.

Would a corresponding imperative wording become helpful for an improved change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.18-rc3#n94


How do you think about to refer to predictions for condition checks?

Regards,
Markus

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

[Jan Kara]

On Wed 29-10-25 14:49:52, Mateusz Guzik wrote:
> 1. we already expect the refcount is 1.
> 2. path creation predicts name == iname
> 
> I verified this straightens out the asm, no functional changes.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Looks sensible. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> 
> random annoyance i noticed while profiling
> 
>  fs/namei.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index ba29ec7b67c5..4692f25e7cd9 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -282,7 +282,7 @@ void putname(struct filename *name)
>  		return;
>  
>  	refcnt = atomic_read(&name->refcnt);
> -	if (refcnt != 1) {
> +	if (unlikely(refcnt != 1)) {
>  		if (WARN_ON_ONCE(!refcnt))
>  			return;
>  
> @@ -290,7 +290,7 @@ void putname(struct filename *name)
>  			return;
>  	}
>  
> -	if (name->name != name->iname) {
> +	if (unlikely(name->name != name->iname)) {
>  		__putname(name->name);
>  		kfree(name);
>  	} else
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Christian Brauner]

On Wed, 29 Oct 2025 14:49:52 +0100, Mateusz Guzik wrote:
> 1. we already expect the refcount is 1.
> 2. path creation predicts name == iname
> 
> I verified this straightens out the asm, no functional changes.
> 
> 

Applied to the vfs-6.19.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.19.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.19.misc

[1/1] fs: touch up predicts in putname()
      https://git.kernel.org/vfs/vfs/c/20052f2ef08a

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

[Al Viro]

On Wed, Oct 29, 2025 at 02:49:52PM +0100, Mateusz Guzik wrote:
> 1. we already expect the refcount is 1.
> 2. path creation predicts name == iname
> 
> I verified this straightens out the asm, no functional changes.

FWIW, I think I know how to get rid of atomic there.  Doesn't
invalidate your patch...

Look:

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.

1) provide getname_alien(), differing from plain getname() only
in the lack of audit_getname() call.

2) have io_uring use it for references that might be handled in
a worker thread.

3) provide something like

struct filename *take_filename(struct filename **p)
{
	struct filename *res = no_free_ptr(*p);
	audit_getname(res);
	return res;
}

and have places like io_mkdirat() switch from
        ret = do_mkdirat(mkd->dfd, mkd->filename, mkd->mode);
	 
	req->flags &= ~REQ_F_NEED_CLEANUP;
to
	ret = do_mkdirat(mkd->dfd, take_filename(&mkd->filename), mkd->mode);

Voila - no need for atomic.  Prior to audit_getname() it's going to be 1;
after that only the thread that has called audit_getname() is going to see
the address of the object (and all accesses are going to be process-synchronous).
IOW, it becomes a plain int refcount.  Sure, we still want that prediction there,
but the atomicity cost is no more...

I'll post the ->uptr removal series tonight or tomorrow; figuring out the right
calling conventions for getname_alien() is the main obstacle for (1--3) ATM...

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

[Al Viro]

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

> I'll post the ->uptr removal series tonight or tomorrow; figuring out the right
> calling conventions for getname_alien() is the main obstacle for (1--3) ATM...

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

[Mateusz Guzik]

On Sat, Nov 1, 2025 at 7:05 AM Al Viro <viro@zeniv.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.

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

[Al Viro]

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@zeniv.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).

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

[Mateusz Guzik]

On Sun, Nov 2, 2025 at 7:14 AM Al Viro <viro@zeniv.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@zeniv.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.

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

[Al Viro]

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.

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

[Mateusz Guzik]

On Mon, Nov 3, 2025 at 5:45 AM Al Viro <viro@zeniv.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.

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

[Al Viro]

On Mon, Nov 03, 2025 at 05:44:07PM +0100, Mateusz Guzik wrote:

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

That would make sense had there been any places where we would want
use the alien_filename contents (hell, access it) in any way other
than "destroy and get a struct filename reference".  I don't see any
candidates, TBH...