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

On Tue, Aug 11, 2026 at 4:54 PM Christian Brauner <[email protected]> wrote:
>
> 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
> > <[email protected]> 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.

I did consider using scope-based cleanup, but there's a gap in the available
guard infrastructure: rwsem read locks have conditional guard variants for
_try and _intr, but not for _kill (down_read_killable). I'd prefer to keep the
current manual lock/unlock + goto style for now rather than switch to
interruptible acquisition or add a new guard definition just for this.