pidfs: simplify PIDFD_GET_<type>_NAMESPACE ioctls
Christian Brauner
maintainerPATCH 1/1 pidfs: simplify PIDFD_GET_<type>_NAMESPACE ioctls
We have reworked namespaces sufficiently that all this special-casing shouldn't be needed anymore
--- fs/pidfs.c | 75 ++++++++++++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 39 deletions(-)
fs/pidfs.c +36 -39
@@ -454,7 +454,6 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct task_struct *task __free(put_task) = NULL; struct nsproxy *nsp __free(put_nsproxy) = NULL;struct ns_common *ns_common =NULL ; -struct pid_namespace *pid_ns;if (!pidfs_ioctl_valid (cmd))return -ENOIOCTLCMD ;
@@ -496,66 +495,64 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)switch (cmd) {/* Namespaces that hang of nsproxy. */ case PIDFD_GET_CGROUP_NAMESPACE : -if (IS_ENABLED (CONFIG_CGROUPS )) { -get_cgroup_ns (nsp->cgroup_ns); - ns_common =to_ns_common (nsp->cgroup_ns); - } +if (!ns_ref_get (nsp->cgroup_ns)) +break ; + ns_common =to_ns_common (nsp->cgroup_ns);break ;case PIDFD_GET_IPC_NAMESPACE : -if (IS_ENABLED (CONFIG_IPC_NS )) { -get_ipc_ns (nsp->ipc_ns); - ns_common =to_ns_common (nsp->ipc_ns); - } +if (!ns_ref_get (nsp->ipc_ns)) +break ; + ns_common =to_ns_common (nsp->ipc_ns);break ;case PIDFD_GET_MNT_NAMESPACE : -get_mnt_ns (nsp->mnt_ns); +if (!ns_ref_get (nsp->mnt_ns)) +break ; ns_common =to_ns_common (nsp->mnt_ns);break ;case PIDFD_GET_NET_NAMESPACE : -if (IS_ENABLED (CONFIG_NET_NS )) { - ns_common =to_ns_common (nsp->net_ns); -get_net_ns (ns_common); - } +if (!ns_ref_get (nsp->net_ns)) +break ; + ns_common =to_ns_common (nsp->net_ns);break ;case PIDFD_GET_PID_FOR_CHILDREN_NAMESPACE : -if (IS_ENABLED (CONFIG_PID_NS )) { -get_pid_ns (nsp->pid_ns_for_children); - ns_common =to_ns_common (nsp->pid_ns_for_children); - } +if (!ns_ref_get (nsp->pid_ns_for_children)) +break ; + ns_common =to_ns_common (nsp->pid_ns_for_children);break ;case PIDFD_GET_TIME_NAMESPACE : -if (IS_ENABLED (CONFIG_TIME_NS )) { -get_time_ns (nsp->time_ns); - ns_common =to_ns_common (nsp->time_ns); - } +if (!ns_ref_get (nsp->time_ns)) +break ; + ns_common =to_ns_common (nsp->time_ns);break ;case PIDFD_GET_TIME_FOR_CHILDREN_NAMESPACE : -if (IS_ENABLED (CONFIG_TIME_NS )) { -get_time_ns (nsp->time_ns_for_children); - ns_common =to_ns_common (nsp->time_ns_for_children); - } +if (!ns_ref_get (nsp->time_ns_for_children)) +break ; + ns_common =to_ns_common (nsp->time_ns_for_children);break ;case PIDFD_GET_UTS_NAMESPACE : -if (IS_ENABLED (CONFIG_UTS_NS )) { -get_uts_ns (nsp->uts_ns); - ns_common =to_ns_common (nsp->uts_ns); - } +if (!ns_ref_get (nsp->uts_ns)) +break ; + ns_common =to_ns_common (nsp->uts_ns);break ;/* Namespaces that don't hang of nsproxy. */ case PIDFD_GET_USER_NAMESPACE : -if (IS_ENABLED (CONFIG_USER_NS )) { -rcu_read_lock (); - ns_common =to_ns_common (get_user_ns (task_cred_xxx (task, user_ns))); -rcu_read_unlock (); + 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); }break ;case PIDFD_GET_PID_NAMESPACE : -if (IS_ENABLED (CONFIG_PID_NS )) { -rcu_read_lock (); + scoped_guard(rcu) { +struct pid_namespace *pid_ns; + pid_ns =task_active_pid_ns (task); -if (pid_ns) - ns_common =to_ns_common (get_pid_ns (pid_ns)); -rcu_read_unlock (); +if (!ns_ref_get (pid_ns)) +break ; + ns_common =to_ns_common (pid_ns); }break ;default :
Signature · 1 line
2.47.3
Quoted 4 lines · Christian Brauner
We have reworked namespaces sufficiently that all this special-casing shouldn't be needed anymore
Signed-off-by Christian Brauner <[email protected]>
This causes a NULL dereference in nsfs_evict() when CONFIG_NET_NS=n. Nothing special needed besides that: it happens just when booting the system, using systemd.
Reverting this commit on mainline fixes the bug.
- Eric
Christian Brauner
maintainerPATCH 1/1 pidfs: protect PIDFD_GET_* ioctls() via ifdef
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.
--- fs/pidfs.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
fs/pidfs.c +18 -0
@@ -517,14 +517,18 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)switch (cmd) {/* Namespaces that hang of nsproxy. */ case PIDFD_GET_CGROUP_NAMESPACE : +#ifdef CONFIG_CGROUPS if (!ns_ref_get (nsp->cgroup_ns))break ; ns_common =to_ns_common (nsp->cgroup_ns); +#endif break ;case PIDFD_GET_IPC_NAMESPACE : +#ifdef CONFIG_IPC_NS if (!ns_ref_get (nsp->ipc_ns))break ; ns_common =to_ns_common (nsp->ipc_ns); +#endif break ;case PIDFD_GET_MNT_NAMESPACE :if (!ns_ref_get (nsp->mnt_ns))
@@ -532,32 +536,43 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg) ns_common =to_ns_common (nsp->mnt_ns);break ;case PIDFD_GET_NET_NAMESPACE : +#ifdef CONFIG_NET_NS if (!ns_ref_get (nsp->net_ns))break ; ns_common =to_ns_common (nsp->net_ns); +#endif break ;case PIDFD_GET_PID_FOR_CHILDREN_NAMESPACE : +#ifdef CONFIG_PID_NS if (!ns_ref_get (nsp->pid_ns_for_children))break ; ns_common =to_ns_common (nsp->pid_ns_for_children); +#endif break ;case PIDFD_GET_TIME_NAMESPACE : +#ifdef CONFIG_TIME_NS if (!ns_ref_get (nsp->time_ns))break ; ns_common =to_ns_common (nsp->time_ns); +#endif break ;case PIDFD_GET_TIME_FOR_CHILDREN_NAMESPACE : +#ifdef CONFIG_TIME_NS if (!ns_ref_get (nsp->time_ns_for_children))break ; ns_common =to_ns_common (nsp->time_ns_for_children); +#endif break ;case PIDFD_GET_UTS_NAMESPACE : +#ifdef CONFIG_UTS_NS if (!ns_ref_get (nsp->uts_ns))break ; ns_common =to_ns_common (nsp->uts_ns); +#endif break ;/* Namespaces that don't hang of nsproxy. */ case PIDFD_GET_USER_NAMESPACE : +#ifdef CONFIG_USER_NS scoped_guard (rcu) {struct user_namespace *user_ns;
@@ -566,8 +581,10 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)break ; ns_common =to_ns_common (user_ns); } +#endif break ;case PIDFD_GET_PID_NAMESPACE : +#ifdef CONFIG_PID_NS scoped_guard (rcu) {struct pid_namespace *pid_ns;
@@ -576,6 +593,7 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)break ; ns_common = to_ns_common(pid_ns); } +#endif break ;default :return -ENOIOCTLCMD ;
Signature · 1 line
2.47.3
Re: [PATCH] pidfs: protect PIDFD_GET_* ioctls() via ifdef
Quoted 11 lines · Christian Brauner
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 offns->opswill 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->opsfor 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.
Quoted 4 lines
Reported-by Eric Biggers <[email protected]>Fixes a71e4f103aed ("pidfs: simplify PIDFD_GET_<type>_NAMESPACE ioctls")Signed-off-by Christian Brauner <[email protected]>
Quoted 10 lines
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); }
Quoted 2 lines
#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
Re: [PATCH] pidfs: protect PIDFD_GET_* ioctls() via ifdef
In reply to Christian BraunerQuoted 11 lines · Christian Brauner
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 offns->opswill 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->opsfor 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.
Quoted 4 lines
Reported-by Eric Biggers <[email protected]>Fixes a71e4f103aed ("pidfs: simplify PIDFD_GET_<type>_NAMESPACE ioctls")Signed-off-by Christian Brauner <[email protected]>
Thanks Christian, I was also getting a NULL deref from running the x86:ldt_gdt_32 kselftest on QEMU under a minimal kconfig (via task_active_pid_ns()), this fixes it.
Quoted 12 lines · Christian Brauner
We have reworked namespaces sufficiently that all this special-casing shouldn't be needed anymore
Signed-off-by Christian Brauner <[email protected]>--- fs/pidfs.c | 75 ++++++++++++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 39 deletions(-)fs/pidfs.c +0
-0
...
fs/pidfs.c static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 544
Quoted 11 lines
fs/pidfs.c +6
-4case PIDFD_GET_USER_NAMESPACE : -if (IS_ENABLED (CONFIG_USER_NS )) { -rcu_read_lock (); - ns_common =to_ns_common (get_user_ns (task_cred_xxx (task, user_ns))); -rcu_read_unlock (); + scoped_guard(rcu) { +struct user_namespace *user_ns; + + user_ns =task_cred_xxx (task, user_ns); +if (!ns_ref_get (user_ns)) +break ;
I think this code is a bit misleading and could lead to unintentional mistakes in future changes.
scoped_guard() is implemented using a for loop, so this break statement is only breaking out of the the scoped_guard() scope and not breaking out of the case as one might expect.
I suggest to change the logic to avoid the break or at least add a comment pointing out the unusual behavior.
Quoted 5 lines
fs/pidfs.c +1
-1+ ns_common =to_ns_common (user_ns); }break ;case PIDFD_GET_PID_NAMESPACE : -if (IS_ENABLED (CONFIG_PID_NS )) {
fs/pidfs.c static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 554
Quoted 10 lines
fs/pidfs.c +5
-4-rcu_read_lock (); +scoped_guard (rcu) { +struct pid_namespace *pid_ns; + pid_ns =task_active_pid_ns (task); -if (pid_ns) - ns_common =to_ns_common (get_pid_ns (pid_ns)); -rcu_read_unlock (); +if (!ns_ref_get (pid_ns)) +break ;
Same situation here.
Quoted 4 lines
fs/pidfs.c +1
-0+ ns_common =to_ns_common (pid_ns); }break ;default :
Christian Brauner
maintainerQuoted 56 lines · David Lechner
Quoted 12 lines · Christian Brauner
We have reworked namespaces sufficiently that all this special-casing shouldn't be needed anymore
Signed-off-by Christian Brauner <[email protected]>--- fs/pidfs.c | 75 ++++++++++++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 39 deletions(-)fs/pidfs.c +0
-0...
fs/pidfs.c
Quoted 11 lines
fs/pidfs.c +6
-4case PIDFD_GET_USER_NAMESPACE : -if (IS_ENABLED (CONFIG_USER_NS )) { -rcu_read_lock (); - ns_common =to_ns_common (get_user_ns (task_cred_xxx (task, user_ns))); -rcu_read_unlock (); + scoped_guard(rcu) { +struct user_namespace *user_ns; + + user_ns =task_cred_xxx (task, user_ns); +if (!ns_ref_get (user_ns)) +break ;I think this code is a bit misleading and could lead to unintentional mistakes in future changes.
scoped_guard()is implemented using a for loop, so this break statement is only breaking out of the thescoped_guard()scope and not breaking out of the case as one might expect.I suggest to change the logic to avoid the break or at least add a comment pointing out the unusual behavior.
Quoted 5 lines
fs/pidfs.c +1
-1+ ns_common =to_ns_common (user_ns); }break ;case PIDFD_GET_PID_NAMESPACE : -if (IS_ENABLED (CONFIG_PID_NS )) {fs/pidfs.c
Quoted 10 lines
fs/pidfs.c +5
-4-rcu_read_lock (); +scoped_guard (rcu) { +struct pid_namespace *pid_ns; + pid_ns =task_active_pid_ns (task); -if (pid_ns) - ns_common =to_ns_common (get_pid_ns (pid_ns)); -rcu_read_unlock (); +if (!ns_ref_get (pid_ns)) +break ;Same situation here.
Quoted 4 lines
fs/pidfs.c
Quoted 4 lines
fs/pidfs.c +1
-0+ ns_common =to_ns_common (pid_ns); }break ;default :
Good point, we can change this to:
fs/pidfs.c +4 -6
@@ -577,9 +577,8 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)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); +if (ns_ref_get (user_ns)) + ns_common = to_ns_common(user_ns); }#endif break ;
@@ -589,9 +588,8 @@ static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)struct pid_namespace *pid_ns; pid_ns =task_active_pid_ns (task); -if (!ns_ref_get (pid_ns)) -break ; - ns_common = to_ns_common(pid_ns); +if (ns_ref_get (pid_ns)) + ns_common = to_ns_common(pid_ns); }#endif break ;