Re: [PATCH] pidfd: hold exec_update_lock around namespace ioctl

[Christian Brauner]

On Mon, Aug 03, 2026 at 03:44:21PM +0200, Jann Horn wrote:
> On Fri, Jul 31, 2026 at 4:50 PM Chen Linxuan via B4 Relay
> <devnull+me.black-desk.cn@xxxxxxxxxx> wrote:
> > The PIDFD_GET_*_NAMESPACE ioctls in pidfd_ioctl() perform a filesystem
> > credentials ptrace access check before handing out a namespace file
> > descriptor.  The accompanying comment states that the code "mirrors nsfs
> > behavior", but, unlike the corresponding procfs paths, it does so without
> > holding the target task's exec_update_lock.
> >
> > proc_ns_get_link() and proc_ns_readlink() both take exec_update_lock for
> > reading around the ptrace check and the namespace lookup, so that the
> > credentials used for the access decision match those of the task when its
> > namespace is read.  Without it, a caller can pass the check against the
> > target's old credentials and then read the namespace after the target has
> > execve()'d a setuid binary and committed new credentials -- accessing
> > namespace information it should have been denied.
> >
> > Hold exec_update_lock for reading around the ptrace check and the
> > namespace lookup so that pidfd truly mirrors nsfs behavior, as the comment
> > already claims.  open_namespace() itself runs outside the lock: once a
> > namespace reference is obtained it carries its own refcount and is opened
> > with the caller's own credentials, so a concurrent execve() on the target
> > can no longer affect the outcome.
> 
> I think this makes sense.
> 
> Given that the rest of this function is written with scope-based
> cleanup (https://docs.kernel.org/core-api/cleanup.html), I wonder if
> this patch would look cleaner if it also used scope-based cleanup...
> that documentation also says:
> 
> "Lastly, given that the benefit of cleanup helpers is removal of
> “goto”, and that the “goto” statement can jump between scopes, the
> expectation is that usage of “goto” and cleanup helpers is never mixed
> in the same function."
> 
> But we probably shouldn't be holding the exec_update_lock across the
> open_namespace() call... so I guess this will require either
> refactoring this function into two, or indenting most of the function
> body, or some explicit "drop this guard" operation.
> 
> @Christian, do you have an opinion on this?

No strong opinion tbh. If I don't like the next version I can also just
massage it when applying.