Re: [PATCH] pidfs: protect PIDFD_GET_* ioctls() via ifdef

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Dec 24, 2025 at 01:00:24PM +0100, Christian Brauner wrote:
> We originally protected PIDFD_GET_<ns-type>_NAMESPACE ioctls() through
> ifdefs and recent rework made it possible to drop them. There was an
> oversight though. When the relevant namespace is turned off ns->ops will
> be NULL so even though opening a file descriptor is perfectly legitimate
> it would fail during inode eviction when the file was closed.
> 
> The simple fix would be to check ns->ops for NULL and continue allow to
> retrieve namespace fds from pidfds but we don't allow retrieving them
> when the relevant namespace type is turned off. So keep the
> simplification but add the ifdefs back in.
> 
> Reported-by: Eric Biggers <ebiggers@kernel.org>
> Link: https://lore.kernel.org/20251222214907.GA189632@quark
> Fixes: a71e4f103aed ("pidfs: simplify PIDFD_GET_<type>_NAMESPACE ioctls")
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Tested-by: Eric Biggers <ebiggers@kernel.org>

>	case PIDFD_GET_USER_NAMESPACE:
>#ifdef CONFIG_USER_NS
>		scoped_guard(rcu) {
>			struct user_namespace *user_ns;
>
>			user_ns = task_cred_xxx(task, user_ns);
>			if (!ns_ref_get(user_ns))
>				break;
>			ns_common = to_ns_common(user_ns);
>		}
>#endif
>  		break;

Not directly related to this patch, but you know that the 'break;' above
breaks from the scoped_guard() block and not from the switch statement,
right?  (Considering that scoped_guard() is implemented using 'for'.)
There is a break after the scoped_guard(), so it ends up being the same.
But it's confusing.  It would be much easier to understand if it was
rewritten to not use an inner 'break':

            if (ns_ref_get(user_ns))
                    ns_common = to_ns_common(user_ns);

- Eric