audit: Avoid excessive dput/dget in audit_context setup and reset paths

kernel/auditsc.c static struct audit_names *audit_alloc_name(struct audit_context *context, · line 2201

Quoted 6 lines · Waiman Long

kernel/auditsc.c +6 -0

+		/*
+		 * Defer the path_put() call of the old pwd to workqueue as
+		 * we may be in an atomic context that cannot call path_put()
+		 * directly because of might_sleep().
+		 */
+		pp = kmalloc(sizeof(*pp), GFP_NOFS);

If that really gets called in atomic context, this kmalloc will in itself be enough to fuck you over, won't it?

kernel/audit.h struct audit_context { · line 140

Quoted 13 lines · Waiman Long

kernel/audit.h +5 -0

index 7c401729e21b..03f3539b10e7 100644
@@ -133,6 +133,13 @@ struct audit_context {
  	int		    name_count; /* total records in names_list */
  	struct list_head    names_list;	/* struct audit_names->list anchor */
  	char		    *filterkey;	/* key for rule that triggered record */
+	/*
+	 * pwd_reset is set if audit_free_names() has been called from
+	 * audit_reset_context() to reset pwd, but pwd is still holding dentry
+	 * and mount references to be used in later audit action without
+	 * the need to reacqure the references again.

That's a delicate way to say "we have mounts stuck busy inexplicably for userland"...

Generally a chdir(2) away from something immediately followed by umount(2) is _not_ expected to leave you with -EBUSY when nobody else has been doing anything with the mount in question.

That is actually a concern that I have at the back of my mind. I can modify the patch to cache only the dentry and do get/put the mount every time which is much cheaper as it is a percpu counter.  In that way, a chdir(2) followed by a umount(2) shouldn't cause a -EBUSY. Right?

Quoted 4 lines · Waiman Long

That is actually a concern that I have at the back of my mind. I can modify the patch to cache only the dentry and do get/put the mount every time which is much cheaper as it is a percpu counter.  In that way, a chdir(2) followed by a umount(2) shouldn't cause a -EBUSY. Right?

Quite - it will just retain a reference to dentry, with filesystem shutdown being very unhappy about somebody retaining references to objects on the filesystem about to be taken out...

Quoted 9 lines · Al Viro

Quoted 4 lines · Waiman Long

That is actually a concern that I have at the back of my mind. I can modify the patch to cache only the dentry and do get/put the mount every time which is much cheaper as it is a percpu counter.  In that way, a chdir(2) followed by a umount(2) shouldn't cause a -EBUSY. Right?

Quite - it will just retain a reference to dentry, with filesystem shutdown being very unhappy about somebody retaining references to objects on the filesystem about to be taken out...

Sarcasm aside, I wonder if we could do the following trick:
* a new primitive for "grab or borrow pwd", similar to what fdget()
does for struct file. If current->fs is shared, do what we do now and return true; otherwise just copy the contents of current->fs->pwd return false.
* paired primitive that would take a boolean + struct path * and
do path_put() if boolean is true.
* syscalls that might alter ->fs, ->fs->pwd or add extra references to
->fs would start with grabbing an extra ref on entry and drop it in the end; that would make that primitive safe to use there.
* audit using that thing and storing the result along with the copy
of pwd; on the way out it would use the "put unless borrowed" primitive.

Might or might not be useful - hard to tell without knowing the job mix of those audit-afflicted production systems.

I'll try to put something along those lines together...

Interesting. So are you thinking about a reference on the pwd inside the fs_struct?

I am thinking about instead of getting a reference to pwd.dentry and pwd.mnt, we can just get a reference to the pwd itself. We have to grab the fs lock in get_fs_pwd() anyway.

This spinlock doesn't show up as being
contended in the customer bug report as each process can has its own fs_struct instead of many processing sharing the same working directory.

In the case of audit, fs->pwd is copied out at the beginning of the open* system call and then put back at the end of that system call. So it should last a pretty short time, i.e. the reference will be released shortly. However, that will make the set_fs_pwd() call a bit tricky to implement, but I think it is still doable.

Quoted 2 lines · Waiman Long

Interesting. So are you thinking about a reference on the pwd inside the fs_struct?

No. fs_struct has pwd pinned - both dentry and mount.

Each thread has a reference to fs_struct instance in its task_struct (task->fs).

It is possible for several threads to have their task->fs pointing to the same instance.

A thread may modify its fs_struct pointer, making it point to a different fs_struct instance; that happens on unshare(2). In that case new instance is an identical copy of the original one; reference counts of mounts and dentries (both for pwd and root) are incremented to account for additional references in the copy. That assignment to current->fs happens under task_lock(current).

A child gets either a reference to identical copy of parent's fs_struct or an extra reference to parent's fs_struct. In either case child->fs is set before the child gets to run; as the matter of fact, that happens before anyone besides the parent might observe the task_struct of child.

That's it - no other stores to task_struct.fs are ever allowed; no other thread can change your reference under you.

Other than initializing a new copy, *all* stores to fs_struct.pwd are done to current->fs.pwd; access to any other fs_struct instances is read-only. The only way another thread might change your pwd is if that thread shares fs_struct with you. They can observe its contents, as long as they take care to hold task_lock(your_thread), but no more than that.

What's more, if current->fs is the sole reference to fs_struct instance, it will remain such until you spawn a child and set it to share your instance (CLONE_FS). No other thread can gain extra references to it, etc.

What it means is that if you are holding the sole reference to current->fs, current->fs->pwd contents will remain unchanged (and pinning the same mount and dentry) through the entire syscall with very few exceptions:
1) you spawn a child with CLONE_FS - that has to be either
clone(2) or io_uring_setup(2) (the latter - via worker threads).
2) you explicitly modify your pwd - in chdir(2), fchdir(2),
pivot_root(2), setns(2) (with CLONE_NEWNS; it switches you to the root of namespace you've joined)

As long as these exceptions are accounted for, audit could check current->fs->users *and* skip incrementing refcounts if it's equal to 1. It would need to remember whether these reference had been taken - rechecking condition won't work, since other threads might by gone by the time we leave the syscall turning "shared" to "not shared".

That way we can avoid grabbing any references in a fairly common case.

Thanks for the detailed explanation. I am thinking about something like the code diff below. Of course, there are other corner cases like unshare(2) that still needs to be handled. Do you think something like this is viable?

Quoted 3 lines · Waiman Long

Thanks for the detailed explanation. I am thinking about something like the code diff below. Of course, there are other corner cases like unshare(2) that still needs to be handled. Do you think something like this is viable?

Deadlocks aside, the immediate problem here is that consensus number is too low. Take three threads sharing the same fs_struct instance. The first one calls your get_fs_pwd_share(); then the remaining two threads call set_fs_pwd() (e.g. by calling chdir(2) in userland code). The reference stored into fs->pwd_waiter by the first of those two gets overwritten by that stored by the second. When the caller of get_fs_pwd_share() gets to put_fs_pwd_share(), only one of the sleepers gets woken up...

And it's very easy to end up with something as simple as chdir("foo") deadlocking - we start with resolving the relative pathname we'd been given, audit wants to record the current directory, on the theory that relative pathname is none too useful in logs without knowing what had it been relative to. Then, in the same thread, you call set_fs_pwd() - after all, that's the main effect of chdir(2). Deadlock...

IOW, it's not just unshare(2) that needs to be taken care of - chdir(2) would need to be treated differently.

Quoted 21 lines · Al Viro

Quoted 3 lines · Waiman Long

Thanks for the detailed explanation. I am thinking about something like the code diff below. Of course, there are other corner cases like unshare(2) that still needs to be handled. Do you think something like this is viable?

Deadlocks aside, the immediate problem here is that consensus number is too low.  Take three threads sharing the same fs_struct instance. The first one calls your get_fs_pwd_share(); then the remaining two threads call set_fs_pwd() (e.g. by calling chdir(2) in userland code).  The reference stored into fs->pwd_waiter by the first of those two gets overwritten by that stored by the second.  When the caller of get_fs_pwd_share() gets to put_fs_pwd_share(), only one of the sleepers gets woken up...

And it's very easy to end up with something as simple as chdir("foo") deadlocking - we start with resolving the relative pathname we'd been given, audit wants to record the current directory, on the theory that relative pathname is none too useful in logs without knowing what had it been relative to. Then, in the same thread, you call set_fs_pwd() - after all, that's the main effect of chdir(2). Deadlock...

Quoted 2 lines

IOW, it's not just unshare(2) that needs to be taken care of - chdir(2) would need to be treated differently.

Now I realize that there is indeed a deadlock problem. Scrap that. Now I have a simpler idea that shouldn't have this type of deadlock problem. So what do you think about the sample code below?

(quoted diff) +0 -0

@@ -70,6 +74,8 @@ void chroot_fs_refs(const struct path *old_root, const

struct>
                                count++; path_get(new_root); }
+                       count += fs->pwd_xrefs; +                       fs->pwd_xrefs = 0; write_sequnlock(&fs->seq);

Quoted 8 lines · Waiman Long

(quoted diff) +0 -0

@@ -70,6 +74,8 @@ void chroot_fs_refs(const struct path *old_root, const

struct>
                                count++; path_get(new_root); }
+                       count += fs->pwd_xrefs; +                       fs->pwd_xrefs = 0;
                        write_sequnlock(&fs->seq);

Nope - you only need that for threads that have ->pwd equal to old_root. Incidentally, I'd forgotten about that sucker - it kills the idea of fdget-like tricks dead, more's the pity. Third-party modification of task->fs->pwd (under task->lock and task->fs->seq), possible even with task->fs->users == 1.

Quoted 16 lines · Al Viro

Quoted 9 lines · Waiman Long

(quoted diff) +0 -0

@@ -70,6 +74,8 @@ void chroot_fs_refs(const struct path *old_root, 

const
struct>
                                 count++; path_get(new_root); }
+                       count += fs->pwd_xrefs; +                       fs->pwd_xrefs = 0; write_sequnlock(&fs->seq);

Nope - you only need that for threads that have ->pwd equal to old_root. Incidentally, I'd forgotten about that sucker - it kills the idea of fdget-like tricks dead, more's the pity.  Third-party modification of task->fs->pwd (under task->lock and task->fs->seq), possible even with task->fs->users == 1.

Yes, I am aware of that when I took a further look at the patch that I sent out yesterday. I am testing the updated patch now and is trying to figure out why I get a warning from mntput_no_expire_slowpath() with a count of -1 when doing an umount. It is off by 1 somewhere. I will post the patch once I resolve this bug.

Quoted 18 lines · Waiman Long

Quoted 16 lines · Al Viro

Quoted 9 lines · Waiman Long

(quoted diff) +0 -0

@@ -70,6 +74,8 @@ void chroot_fs_refs(const struct path *old_root, 

const
struct>
                                 count++; path_get(new_root); }
+                       count += fs->pwd_xrefs; +                       fs->pwd_xrefs = 0; write_sequnlock(&fs->seq);

Nope - you only need that for threads that have ->pwd equal to old_root. Incidentally, I'd forgotten about that sucker - it kills the idea of fdget-like tricks dead, more's the pity.  Third-party modification of task->fs->pwd (under task->lock and task->fs->seq), possible even with task->fs->users == 1.

Quoted 1 line

Yes, I am aware of that when I took a further look at the patch that I sent out yesterday. I am testing the updated patch now and is trying to figure out why I get a warning from mntput_no_expire_slowpath() with a count of -1 when doing an umount. It is off by 1 somewhere. I will post the patch once I resolve this bug.

I now know why there are warnings. The problem is in the copy_mnt_ns() function in fs/namespace.c:

__latent_entropy
struct mnt_namespace *copy_mnt_ns(u64 flags, struct mnt_namespace *ns, struct user_namespace *user_ns, struct fs_struct *new_fs) {
  :
                if (new_fs) {
                        if (&p->mnt == new_fs->root.mnt) { new_fs->root.mnt = mntget(&q->mnt); rootmnt = &p->mnt;
                        }
                        if (&p->mnt == new_fs->pwd.mnt) { new_fs->pwd.mnt = mntget(&q->mnt); pwdmnt = &p->mnt;
                        }
                }

It is replacing the fs->pwd.mnt with a new one while pwd_refs is 1. I can make this work with the new fs_struct field. I do have one question though. Do we need to acquire write_seqlock(&new_fs->seq) if we are changing root or pwd here or if the new_fs are in such a state that it will never change when this copying operation is in progress?

__latent_entropy
struct mnt_namespace *copy_mnt_ns(u64 flags, struct mnt_namespace *ns, struct user_namespace *user_ns, struct fs_struct *new_fs) {
  :
                if (new_fs) {
                        if (&p->mnt == new_fs->root.mnt) { new_fs->root.mnt = mntget(&q->mnt); rootmnt = &p->mnt;
                        }
                        if (&p->mnt == new_fs->pwd.mnt) { new_fs->pwd.mnt = mntget(&q->mnt); pwdmnt = &p->mnt;
                        }
                }

It is replacing the fs->pwd.mnt with a new one while pwd_refs is 1. I can make this work with the new fs_struct field. I do have one question though. Do we need to acquire write_seqlock(&new_fs->seq) if we are changing root or pwd here or if the new_fs are in such a state that it will never change when this copying operation is in progress?

Quoted 16 lines · Waiman Long

__latent_entropy
struct mnt_namespace *copy_mnt_ns(u64 flags, struct mnt_namespace *ns, struct user_namespace *user_ns, struct fs_struct *new_fs) {
  :
                if (new_fs) {
                        if (&p->mnt == new_fs->root.mnt) { new_fs->root.mnt = mntget(&q->mnt); rootmnt = &p->mnt;
                        }
                        if (&p->mnt == new_fs->pwd.mnt) { new_fs->pwd.mnt = mntget(&q->mnt); pwdmnt = &p->mnt;
                        }
                }

Quoted 5 lines

It is replacing the fs->pwd.mnt with a new one while pwd_refs is 1. I can make this work with the new fs_struct field. I do have one question though. Do we need to acquire write_seqlock(&new_fs->seq) if we are changing root or pwd here or if the new_fs are in such a state that it will never change when this copying operation is in progress?

In all cases when we get to that point, new_fs is always a freshly created private copy of current->fs, not reachable from anywhere other than stack frames of the callers, but the proof is not pretty. copy_mnt_ns() is called only by create_new_namespaces() and it gets to copying anything if and only if CLONE_NEWNS is in the flags. So far, so good. The call in create_new_namespaces() is

	new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs);
and both flags and new_fs come straight from create_new_namespaces()
callers.  There are 3 of those:
1) int copy_namespaces(u64 flags, struct task_struct *tsk):
	new_ns = create_new_namespaces(flags, tsk, user_ns, tsk->fs);

That gets called by copy_process(), with tsk being the child we are creating there. tsk->fs is set a bit earlier, by copy_fs() call - either to extra ref to current->fs (when CLONE_FS is present in flags) or to a private copy thereof. Since in the very beginning of copy_process() we have

	if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS))
		return ERR_PTR(-EINVAL);

and we are interested in case when CLONE_NEWNS is set, tsk->fs is going to be a private copy.

2) int unshare_nsproxy_namespaces(unsigned long unshare_flags,
struct nsproxy **new_nsp, struct cred *new_cred, struct fs_struct *new_fs):
*new_nsp = create_new_namespaces(unshare_flags, current, user_ns,
new_fs ? new_fs : current->fs);
That gets called from ksys_unshare(). Earlier in ksys_unshare() we have

        /*
	 * If unsharing namespace, must also unshare filesystem information.
	 */
	if (unshare_flags & CLONE_NEWNS)
		unshare_flags |= CLONE_FS;

so in our case CLONE_FS is going to be set. new_fs is initially set to NULL and, since CLONE_FS is there, the call of unshare_fs() has replaced it with a reference to private copy of current->fs. Again, we get a private copy.

3) int exec_task_namespaces(void):

        new = create_new_namespaces(0, tsk, current_user_ns(), tsk->fs);

No CLONE_NEWNS in flags, so we don't get there at all.

Incidentally, one bogosity I've spotted in unshare_fs() today is

        if (!(unshare_flags & CLONE_FS) || !fs)
					^^^^^^ this
		return 0;

The history is amusing - it had been faithfully preserved since cf2e340f4249 ("[PATCH] unshare system call -v5: system call handler function") back in 2006, when unshare(2) had been added; initially it had been a stub:

(quoted diff) +10 -0

+static int unshare_fs(unsigned long unshare_flags, struct fs_struct **new_fsp)
+{
+       struct fs_struct *fs = current->fs;
+
+       if ((unshare_flags & CLONE_FS) &&
+           (fs && atomic_read(&fs->count) > 1))
+               return -EINVAL;
+
+       return 0;
+}

The thing is, task->fs does not become NULL until exit_fs() clears it, at which point we'd better *not* run into any syscalls, unshare(2) included ;-) The same had been true back in 2006; as the matter of fact, I don't know if it _ever_ had not been true. I suspect that the logics had been copied from exit_fs(), which also has a pointless check that seems to have been added there in 1.3.26, when task->fs went kmalloc'ed. The thing is, in 1.3.26 copy_fs() a failed allocation aborted do_fork() (today's copy_process()) with exit_fs() never called for the child-not-to-be...

Mea culpa - mnt_copy_ns()

callers. There are 3 of those:

Quoted 16 lines · Waiman Long

__latent_entropy
struct mnt_namespace *copy_mnt_ns(u64 flags, struct mnt_namespace *ns,
                struct user_namespace *user_ns, struct fs_struct *new_fs)
{
  :
                if (new_fs) {
                        if (&p->mnt == new_fs->root.mnt) { new_fs->root.mnt = mntget(&q->mnt); rootmnt = &p->mnt;
                        }
                        if (&p->mnt == new_fs->pwd.mnt) { new_fs->pwd.mnt = mntget(&q->mnt); pwdmnt = &p->mnt;
                        }
                }

Quoted 5 lines

It is replacing the fs->pwd.mnt with a new one while pwd_refs is 1. I can make this work with the new fs_struct field. I do have one question though. Do we need to acquire write_seqlock(&new_fs->seq) if we are changing root or pwd here or if the new_fs are in such a state that it will never change when this copying operation is in progress?

In all cases when we get to that point, new_fs is always a freshly created private copy of current->fs, not reachable from anywhere other than stack frames of the callers, but the proof is not pretty. copy_mnt_ns() is called only by create_new_namespaces() and it gets to copying anything if and only if CLONE_NEWNS is in the flags. So far, so good. The call in create_new_namespaces() is

	new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs);

Quoted 23 lines · Al Viro

Quoted 16 lines · Waiman Long

__latent_entropy
struct mnt_namespace *copy_mnt_ns(u64 flags, struct mnt_namespace *ns, struct user_namespace *user_ns, struct fs_struct *new_fs) {
   :
                 if (new_fs) {
                         if (&p->mnt == new_fs->root.mnt) { new_fs->root.mnt = mntget(&q->mnt); rootmnt = &p->mnt;
                         }
                         if (&p->mnt == new_fs->pwd.mnt) { new_fs->pwd.mnt = mntget(&q->mnt); pwdmnt = &p->mnt;
                         }
                 }

Quoted 5 lines

It is replacing the fs->pwd.mnt with a new one while pwd_refs is 1. I can make this work with the new fs_struct field. I do have one question though. Do we need to acquire write_seqlock(&new_fs->seq) if we are changing root or pwd here or if the new_fs are in such a state that it will never change when this copying operation is in progress?

Quoted 7 lines

In all cases when we get to that point, new_fs is always a freshly created private copy of current->fs, not reachable from anywhere other than stack frames of the callers, but the proof is not pretty. copy_mnt_ns() is called only by create_new_namespaces() and it gets to copying anything if and only if CLONE_NEWNS is in the flags.  So far, so good.  The call in create_new_namespaces() is
    new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs);

Thanks for the detailed explanation. After further investigation as to while the pwd_refs is set, I found out the code path leading to this situation is the unshare syscall.

__x64_sys_unshare()
 => ksys_unshare()
  => unshare_fs(unshare_flags, &new_fs)
  => unshare_nsproxy_namespaces(unshare_flags, &new_nsproxy, new_cred, new_fs); => create_new_namespaces(unshare_flags, current, user_ns, new_fs ? new_fs : current->fs);

Here, CLONE_FS isn't set in unshare_flags. So new_fs is NULL and current->fs is passed down to create_new_namespaces(). That is why pwd_refs can be set in this case. So it looks like the comment in copy_mnt_ns() saying that the fs_struct is private is no longer true, at least in this case. So changing fs_struct without taking the lock can lead to unexpected result.

Should we add locking to make it safe?

I guess if private means fs->users == 1, the condition could still be true.

I guess if private means fs->users == 1, the condition could still be true.

On Fri, Feb 06, 2026 at 03:04:53PM -0500, Waiman Long wrote:

[summary of subthread: there's an unpleasant corner case in unshare(2), when we have a CLONE_NEWNS in flags and current->fs hadn't been shared at all; in that case copy_mnt_ns() gets passed current->fs instead of a private copy, which causes interesting warts in proof of correctness]

Quoted 1 line

I guess if private means fs->users == 1, the condition could still be true.

Unfortunately, it's worse than just a convoluted proof of correctness. Consider the case when we have CLONE_NEWCGROUP in addition to CLONE_NEWNS (and current->fs->users == 1).

We pass current->fs to copy_mnt_ns(), all right. Suppose it succeeds and flips current->fs->{pwd,root} to corresponding locations in the new namespace. Now we proceed to copy_cgroup_ns(), which fails (e.g. with -ENOMEM). We call put_mnt_ns() on the namespace created by copy_mnt_ns(), it's destroyed and its mount tree is dissolved, but... current->fs->root and current->fs->pwd are both left pointing to now detached mounts.

They are pinning those, so it's not a UAF, but it leaves the calling process with unshare(2) failing with -ENOMEM _and_ leaving it with pwd and root on detached isolated mounts. The last part is clearly a bug.

There is other fun related to that mess (races with pivot_root(), including the one between pivot_root() and fork(), of all things), but this one is easy to isolate and fix - treat CLONE_NEWNS as "allocate a new fs_struct even if it hadn't been shared in the first place". Sure, we could go for something like "if both CLONE_NEWNS *and* one of the things that might end up failing after copy_mnt_ns() call in create_new_namespaces() are set, force allocation of new fs_struct", but let's keep it simple - the cost of copy_fs_struct() is trivial.

Another benefit is that copy_mnt_ns() with CLONE_NEWNS *always* gets a freshly allocated fs_struct, yet to be attached to anything. That seriously simplifies the analysis...

FWIW, that bug had been there since the introduction of unshare(2) ;-/

Signed-off-by Al Viro <[email protected]>

---

kernel/fork.c +0 -0

index b1f3915d5f8e..68ccbaea7398 100644

On Fri, Feb 06, 2026 at 03:04:53PM -0500, Waiman Long wrote:

[summary of subthread: there's an unpleasant corner case in unshare(2), when we have a CLONE_NEWNS in flags and current->fs hadn't been shared at all; in that case copy_mnt_ns() gets passed current->fs instead of a private copy, which causes interesting warts in proof of correctness]

Quoted 1 line

I guess if private means fs->users == 1, the condition could still be true.

Unfortunately, it's worse than just a convoluted proof of correctness. Consider the case when we have CLONE_NEWCGROUP in addition to CLONE_NEWNS (and current->fs->users == 1).

We pass current->fs to copy_mnt_ns(), all right. Suppose it succeeds and flips current->fs->{pwd,root} to corresponding locations in the new namespace. Now we proceed to copy_cgroup_ns(), which fails (e.g. with -ENOMEM). We call put_mnt_ns() on the namespace created by copy_mnt_ns(), it's destroyed and its mount tree is dissolved, but... current->fs->root and current->fs->pwd are both left pointing to now detached mounts.

They are pinning those, so it's not a UAF, but it leaves the calling process with unshare(2) failing with -ENOMEM _and_ leaving it with pwd and root on detached isolated mounts. The last part is clearly a bug.

There is other fun related to that mess (races with pivot_root(), including the one between pivot_root() and fork(), of all things), but this one is easy to isolate and fix - treat CLONE_NEWNS as "allocate a new fs_struct even if it hadn't been shared in the first place". Sure, we could go for something like "if both CLONE_NEWNS *and* one of the things that might end up failing after copy_mnt_ns() call in create_new_namespaces() are set, force allocation of new fs_struct", but let's keep it simple - the cost of copy_fs_struct() is trivial.

Another benefit is that copy_mnt_ns() with CLONE_NEWNS *always* gets a freshly allocated fs_struct, yet to be attached to anything. That seriously simplifies the analysis...

FWIW, that bug had been there since the introduction of unshare(2) ;-/

Signed-off-by Al Viro <[email protected]>

---

On Fri, Feb 06, 2026 at 03:04:53PM -0500, Waiman Long wrote:

[summary of subthread: there's an unpleasant corner case in unshare(2), when we have a CLONE_NEWNS in flags and current->fs hadn't been shared at all; in that case copy_mnt_ns() gets passed current->fs instead of a private copy, which causes interesting warts in proof of correctness]

[...]

Quoted 7 lines

In all cases when we get to that point, new_fs is always a freshly created private copy of current->fs, not reachable from anywhere other than stack frames of the callers, but the proof is not pretty. copy_mnt_ns() is called only by create_new_namespaces() and it gets to copying anything if and only if CLONE_NEWNS is in the flags. So far, so good. The call in create_new_namespaces() is

	new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs);

Thanks for the detailed explanation. After further investigation as to while the pwd_refs is set, I found out the code path leading to this situation is the unshare syscall.

__x64_sys_unshare()
 => ksys_unshare()
  => unshare_fs(unshare_flags, &new_fs)
  => unshare_nsproxy_namespaces(unshare_flags, &new_nsproxy, new_cred, new_fs); => create_new_namespaces(unshare_flags, current, user_ns, new_fs ? new_fs : current->fs);

Here, CLONE_FS isn't set in unshare_flags. So new_fs is NULL and current->fs is passed down to create_new_namespaces(). That is why pwd_refs can be set in this case. So it looks like the comment in copy_mnt_ns() saying that the fs_struct is private is no longer true, at least in this case. So changing fs_struct without taking the lock can lead to unexpected result.

Look: the case where we might get passed current->fs down there is real. It can happen in one and only one situation - CLONE_NEWNS in unshare(2) arguments *and* current->fs->users being 1.

It wouldn't suffice, since there's chroot_fs_refs() that doesn't give a rat's arse for task->fs being ours - it goes and replaces every ->fs->pwd or ->fs->root that happens to point to old_root.

It's still not a real race, though - both chroot_fs_refs() and that area in copy_mnt_ns() are serialized on namespace_sem.

And yes, it's obscenely byzantine. It gets even worse when you consider the fact that pivot_root(2) does not break only because the refcount drops in chroot_fs_refs() are guaranteed not to reach 0 - the caller is holding its own references to old_root.{mnt,dentry} and *thar* does not get dropped until we drop namespace_sem.

IOW, that shit is actually safe, but man, has its correctness grown fucking convoluted...

Grabbing fs->seq in copy_mnt_ns() wouldn't make the things better, though - it seriously relies upon the same exclusion with chroot_fs_refs() for correctness; unless you are willing to hold it over the entire walk through the mount tree, the proof of correctness doesn't get any simpler.

Quoted 17 lines · Al Viro

Look: the case where we might get passed current->fs down there is real. It can happen in one and only one situation - CLONE_NEWNS in unshare(2) arguments *and* current->fs->users being 1.

It wouldn't suffice, since there's chroot_fs_refs() that doesn't give a rat's arse for task->fs being ours - it goes and replaces every ->fs->pwd or ->fs->root that happens to point to old_root.

It's still not a real race, though - both chroot_fs_refs() and that area in copy_mnt_ns() are serialized on namespace_sem.

And yes, it's obscenely byzantine. It gets even worse when you consider the fact that pivot_root(2) does not break only because the refcount drops in chroot_fs_refs() are guaranteed not to reach 0 - the caller is holding its own references to old_root.{mnt,dentry} and *thar* does not get dropped until we drop namespace_sem.

Quoted 7 lines

IOW, that shit is actually safe, but man, has its correctness grown fucking convoluted...

Grabbing fs->seq in copy_mnt_ns() wouldn't make the things better, though - it seriously relies upon the same exclusion with chroot_fs_refs() for correctness; unless you are willing to hold it over the entire walk through the mount tree, the proof of correctness doesn't get any simpler.

Speaking of the race that _is_ there: pidfd setns() vs. pivot_root(). pivot_root() (well, chroot_fs_refs()) goes over all threads and flips their ->fs->{root,pwd} for the ones that used to be at old_root. The trouble is, in case where we have setns() with more than just CLONE_NEWNS in flags, we end up creating a temporary fs_struct, passing that to mntns_install() and then copying its pwd and root back to the caller's if everything goes well.

That temporary is _not_ going to be found by chroot_fs_refs(), though, so it misses the update by pivot_root().

Quoted 34 lines · Al Viro

Quoted 17 lines · Al Viro

Look: the case where we might get passed current->fs down there is real. It can happen in one and only one situation - CLONE_NEWNS in unshare(2) arguments *and* current->fs->users being 1.

It wouldn't suffice, since there's chroot_fs_refs() that doesn't give a rat's arse for task->fs being ours - it goes and replaces every ->fs->pwd or ->fs->root that happens to point to old_root.

It's still not a real race, though - both chroot_fs_refs() and that area in copy_mnt_ns() are serialized on namespace_sem.

And yes, it's obscenely byzantine. It gets even worse when you consider the fact that pivot_root(2) does not break only because the refcount drops in chroot_fs_refs() are guaranteed not to reach 0 - the caller is holding its own references to old_root.{mnt,dentry} and *thar* does not get dropped until we drop namespace_sem.

Quoted 7 lines

IOW, that shit is actually safe, but man, has its correctness grown fucking convoluted...

Grabbing fs->seq in copy_mnt_ns() wouldn't make the things better, though - it seriously relies upon the same exclusion with chroot_fs_refs() for correctness; unless you are willing to hold it over the entire walk through the mount tree, the proof of correctness doesn't get any simpler.

Speaking of the race that _is_ there: pidfd setns() vs. pivot_root(). pivot_root() (well, chroot_fs_refs()) goes over all threads and flips their ->fs->{root,pwd} for the ones that used to be at old_root. The trouble is, in case where we have setns() with more than just CLONE_NEWNS in flags, we end up creating a temporary fs_struct, passing that to mntns_install() and then copying its pwd and root back to the caller's if everything goes well.

Quoted 2 lines

That temporary is _not_ going to be found by chroot_fs_refs(), though, so it misses the update by pivot_root().

BTW, in the same case of setns(2) (e.g. CLONE_NEWNS | CLONE_NEWUTS in flags) we end up defeating the check for fs->users == 1 in mntns_install() - for the temporary fs_struct it will always be true. Unless I'm missing something elsewhere... Christian? Looks like that went in with 303cc571d107 ("nsproxy:
attach to namespaces via pidfds")...

Quoted 8 lines · Waiman Long

(quoted diff) +0 -0

@@ -70,6 +74,8 @@ void chroot_fs_refs(const struct path *old_root, const

struct>
                                count++; path_get(new_root); }
+                       count += fs->pwd_xrefs; +                       fs->pwd_xrefs = 0; write_sequnlock(&fs->seq);

Nope - you only need that for threads that have ->pwd equal to old_root. Incidentally, I'd forgotten about that sucker - it kills the idea of fdget-like tricks dead, more's the pity. Third-party modification of task->fs->pwd (under task->lock and task->fs->seq), possible even with task->fs->users == 1.

FWIW, I'm going through the fs_struct uses at the moment; will post whatever I get when I go down (or earlier, in the unlikely case it doesn't spill to tomorrow morning), will keep posting incremental followups until the documentation is done. I'm sick and tired of half-finished docs - let's see if I can push through that one this way ;-/

Now I realize that there is indeed a deadlock problem. Scrap that. Now I have a simpler idea that shouldn't have this type of deadlock problem. So what do you think about the sample code below?

Quoted 3 lines · Waiman Long

Now I realize that there is indeed a deadlock problem. Scrap that. Now I have a simpler idea that shouldn't have this type of deadlock problem. So what do you think about the sample code below?

That it's rather bizarre, TBH. Basically, you are allowing to park a number of (identical) references in there instead of dropping them, with your 'xrefs' being the count of skipped drops. get_share either clones a reference or uses up one of those skipped drops; put_share parks the reference if possible. And set discards everything not used up...

It could be made to work, but... ouch. It looks like a special-cased variant of something fairly generic, with really confusing calling conventions. Let me poke around and see if we have any other candidates for something similar; if nothing else, current->fs->root is interesting and not just for audit pathologies...

Note, BTW, that there's chroot_fs_refs() to deal with, along with free_fs_struct() (at least). This stuff is encapsulated in fs/fs_struct.c and include/linux/fs_struct.h... Oh, hell.

Mateusz: *is* there a measurable effect? Because if there isn't, I'm very tempted to simply revert that thing. "Churn of adding fs_struct.h as needed" is not the problem - try "exposing the object internals to far larger subset of the kernel". We had interesting bugs with weird shit deciding to poke in there, locking and refcounting be damned.

Quoted 5 lines · Al Viro

Mateusz: *is* there a measurable effect? Because if there isn't, I'm very tempted to simply revert that thing. "Churn of adding fs_struct.h as needed" is not the problem - try "exposing the object internals to far larger subset of the kernel". We had interesting bugs with weird shit deciding to poke in there, locking and refcounting be damned.

This one I did not bother benchmarking as it is an obvious change -- read a var directly instead of going through a func call.

If this is causing trouble then it definitely needs to be reverted, but any woes only speak to the mess in the header files.

Quoted 7 lines · Mateusz Guzik

Quoted 5 lines · Al Viro

Mateusz: *is* there a measurable effect? Because if there isn't, I'm very tempted to simply revert that thing. "Churn of adding fs_struct.h as needed" is not the problem - try "exposing the object internals to far larger subset of the kernel". We had interesting bugs with weird shit deciding to poke in there, locking and refcounting be damned.

Quoted 5 lines

This one I did not bother benchmarking as it is an obvious change -- read a var directly instead of going through a func call.

If this is causing trouble then it definitely needs to be reverted, but any woes only speak to the mess in the header files.

Any reason this isn't just stealing the cred.h trick?

include/linux/fs_struct.h +2 -1

index 0070764b790a..e8b8d073f9e0 100644
@@ -2,11 +2,12 @@
 #ifndef _LINUX_FS_STRUCT_H
 #define _LINUX_FS_STRUCT_H
 
-#include <linux/sched.h>
 #include <linux/path.h>
 #include <linux/spinlock.h>
 #include <linux/seqlock.h>
 
+struct task_struct;
+
 struct fs_struct {
        int users;
        seqlock_t seq;