ipc: Store mqueue sysctls in the ipc namespace

Right now, the mqueue sysctls take ipc namespaces into account in a rather hacky way. This works in most cases, but does not respect the user namespace.

Within the user namespace, the user cannot change the /proc/sys/fs/mqueue/* parametres. This poses a problem in the rootless containers.

To solve this I changed the implementation of the mqueue sysctls just like some other sysctls.

Before this change:

$ echo 5 | unshare -r -U -i tee /proc/sys/fs/mqueue/msg_max
tee: /proc/sys/fs/mqueue/msg_max: Permission denied
5

Quoted 10 lines

Right now, the mqueue sysctls take ipc namespaces into account in a rather hacky way. This works in most cases, but does not respect the user namespace.

Within the user namespace, the user cannot change the /proc/sys/fs/mqueue/* parametres. This poses a problem in the rootless containers.

To solve this I changed the implementation of the mqueue sysctls just like some other sysctls.

Quoted 5 lines

Before this change:

$ echo 5 | unshare -r -U -i tee /proc/sys/fs/mqueue/msg_max
tee: /proc/sys/fs/mqueue/msg_max: Permission denied
5

Could you crosscheck that all (relevant) allocations in ipc/mqueue.c use GFP_KERNEL_ACCOUNT?

We should not allow normal users to use up all memory.

Otherwise:
The idea is good, the limits do not really prevent using up all memory, _ACCOUNT is the better approach.
And with _ACCOUNT, it doesn't hurt that the namespace root is able to set limits.

Manfred Spraul <[email protected]> writes:
Hi Alexey,

Quoted 11 lines · Alexey Gladkov

Quoted 10 lines

Right now, the mqueue sysctls take ipc namespaces into account in a rather hacky way. This works in most cases, but does not respect the user namespace.

Within the user namespace, the user cannot change the /proc/sys/fs/mqueue/* parametres. This poses a problem in the rootless containers.

To solve this I changed the implementation of the mqueue sysctls just like some other sysctls.

Quoted 7 lines

Quoted 5 lines

Before this change:

$ echo 5 | unshare -r -U -i tee /proc/sys/fs/mqueue/msg_max
tee: /proc/sys/fs/mqueue/msg_max: Permission denied
5

Could you crosscheck that all (relevant) allocations in ipc/mqueue.c use GFP_KERNEL_ACCOUNT?

They are not.

Quoted 7 lines

We should not allow normal users to use up all memory.

Otherwise:
The idea is good, the limits do not really prevent using up all memory, _ACCOUNT is the better approach.
And with _ACCOUNT, it doesn't hurt that the namespace root is able to set limits.

Saying the cgroup kernel memory limit is the only thing that works, and that is always better is silly.

First the cgroup kernel memory limits noted with ACCOUNT are not acceptable on several kernel hot paths because they are so expensive.

Further the memory cgroup kernel memory limit is not always delegated to non-root users, which precludes using the memory cgroup kernel memory limit in many situations.

The RLIMIT_MQUEUE limit definitely works, and as I read the kernel source correct it defaults to MQ_BYTES_MAX aka 819200. A limit of 800KiB should prevent using up all of system memory, except on very low memory machines.

So please let's not confuse apples and oranges, and let's use the tools in the kernel where they work, and not set them up in contest with each other.

Rlimits with generous but real limits in general are good at catching when a program misbehaves. The cgroups are better at setting a total memory cap. In this case the rlimit cap is low enough it simply should not matter.

What has been fixed with the ucount rlimits is that (baring implementation bugs) it is now not possible to create a user namespace and escape your rlimits by using multiple users.

Hello Eric,

Quoted 29 lines · Eric W. Biederman

Manfred Spraul <[email protected]> writes:
Hi Alexey,

Quoted 11 lines · Alexey Gladkov

Quoted 10 lines

Right now, the mqueue sysctls take ipc namespaces into account in a rather hacky way. This works in most cases, but does not respect the user namespace.

Within the user namespace, the user cannot change the /proc/sys/fs/mqueue/* parametres. This poses a problem in the rootless containers.

To solve this I changed the implementation of the mqueue sysctls just like some other sysctls.

Quoted 7 lines

Quoted 5 lines

Before this change:

$ echo 5 | unshare -r -U -i tee /proc/sys/fs/mqueue/msg_max
tee: /proc/sys/fs/mqueue/msg_max: Permission denied
5

Could you crosscheck that all (relevant) allocations in ipc/mqueue.c use GFP_KERNEL_ACCOUNT?

They are not.

Quoted 7 lines

We should not allow normal users to use up all memory.

Otherwise:
The idea is good, the limits do not really prevent using up all memory, _ACCOUNT is the better approach.
And with _ACCOUNT, it doesn't hurt that the namespace root is able to set limits.

Quoted 6 lines

Saying the cgroup kernel memory limit is the only thing that works, and that is always better is silly.

First the cgroup kernel memory limits noted with ACCOUNT are not acceptable on several kernel hot paths because they are so expensive.

I was not aware that ACCOUNT allocations are very expensive.

OTHO adding ACCOUNT resolved various out of memory crashes for IIRC ipc/sem.c and/or ipc/msg.c. But we also do not have an RLIMIT for ipc/sem.c or ipc/msg.c

Let me rephrase my question:

When we allow non-root users to write to /proc/sys/fs/mqueue/msg_max, are there any _relevant_ allocations that bypass _all_ limits?

As you write, we have RLIMIT_MSGQUEUE.

And several allocations for ipc/mqueue already use ACCOUNT:

- the messages themselves, via load_msg()/alloc_msg().

- the inodes, via mqueue_inode_cachep().

Quoted 9 lines

Further the memory cgroup kernel memory limit is not always delegated to non-root users, which precludes using the memory cgroup kernel memory limit in many situations.

The RLIMIT_MQUEUE limit definitely works, and as I read the kernel source correct it defaults to MQ_BYTES_MAX aka 819200. A limit of 800KiB should prevent using up all of system memory, except on very low memory machines.

I'd agree that 800 kB is not relevant. But we need to be certain that there are no loopholes.

I do not see anything relevant, e.g. 0-byte messages should be covered by mq_maxmsg. But perhaps I overlook something.

Quoted 9 lines

So please let's not confuse apples and oranges, and let's use the tools in the kernel where they work, and not set them up in contest with each other.

Rlimits with generous but real limits in general are good at catching when a program misbehaves. The cgroups are better at setting a total memory cap. In this case the rlimit cap is low enough it simply should not matter.

Quoted 3 lines

What has been fixed with the ucount rlimits is that (baring implementation bugs) it is now not possible to create a user namespace and escape your rlimits by using multiple users.

I'll try to check the patch in detail in the next few days.

parametres. This poses a problem in the rootless containers.

Quoted 1 line · Eric W. Biederman

parametres. This poses a problem in the rootless containers.

Did this ever go into the Linux kernel?

Right now, the mqueue sysctls take ipc namespaces into account in a rather hacky way. This works in most cases, but does not respect the user namespace.

Within the user namespace, the user cannot change the /proc/sys/fs/mqueue/* parametres. This poses a problem in the rootless containers.

To solve this I changed the implementation of the mqueue sysctls just like some other sysctls.

Before this change:

$ echo 5 | unshare -r -U -i tee /proc/sys/fs/mqueue/msg_max
tee: /proc/sys/fs/mqueue/msg_max: Permission denied
5

After this change:

$ echo 5 | unshare -r -U -i tee /proc/sys/fs/mqueue/msg_max
5

v2:
* Fixed compilation problem if CONFIG_POSIX_MQUEUE_SYSCTL is not specified.

Reported-by kernel test robot <[email protected]>

Signed-off-by Alexey Gladkov <[email protected]>

---
 include/linux/ipc_namespace.h |  18 ++++-
 ipc/mq_sysctl.c               | 137 ++++++++++++++++++++--------------
 ipc/mqueue.c                  |  10 +--
 ipc/namespace.c               |   6 ++
 4 files changed, 106 insertions(+), 65 deletions(-)

include/linux/ipc_namespace.h +1 -0

index b75395ec8d52..bcedc86a6f1d 100644
@@ -10,6 +10,7 @@
 #include <linux/ns_common.h>
 #include <linux/refcount.h>
 #include <linux/rhashtable-types.h>
+#include <linux/sysctl.h>
 
 struct user_namespace;
 

include/linux/ipc_namespace.h +8 -4

 	/* user_ns which owns the ipc ns */
 	struct user_namespace *user_ns;
 	struct ucounts *ucounts;

@@ -169,14 +175,18 @@ static inline void put_ipc_ns(struct ipc_namespace *ns)
 
 #ifdef CONFIG_POSIX_MQUEUE_SYSCTL
 
-struct ctl_table_header;
-extern struct ctl_table_header *mq_register_sysctl_table(void);
+void retire_mq_sysctls(struct ipc_namespace *ns);
+bool setup_mq_sysctls(struct ipc_namespace *ns);
 
 #else /* CONFIG_POSIX_MQUEUE_SYSCTL */
 
-static inline struct ctl_table_header *mq_register_sysctl_table(void)
+static inline void retire_mq_sysctls(struct ipc_namespace *ns)
 {
-	return NULL;
+}
+
+static inline bool setup_mq_sysctls(struct ipc_namespace *ns)
+{
+	return true;
 }
 
 #endif /* CONFIG_POSIX_MQUEUE_SYSCTL */

ipc/mq_sysctl.c +21 -53

index 72a92a08c848..56afb0503296 100644
@@ -9,39 +9,9 @@
 #include <linux/ipc_namespace.h>
 #include <linux/sysctl.h>
 
-#ifdef CONFIG_PROC_SYSCTL
-static void *get_mq(struct ctl_table *table)
-{
-	char *which = table->data;
-	struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
-	which = (which - (char *)&init_ipc_ns) + (char *)ipc_ns;
-	return which;
-}
-
-static int proc_mq_dointvec(struct ctl_table *table, int write,
-			    void *buffer, size_t *lenp, loff_t *ppos)
-{
-	struct ctl_table mq_table;
-	memcpy(&mq_table, table, sizeof(mq_table));
-	mq_table.data = get_mq(table);
-
-	return proc_dointvec(&mq_table, write, buffer, lenp, ppos);
-}
-
-static int proc_mq_dointvec_minmax(struct ctl_table *table, int write,
-		void *buffer, size_t *lenp, loff_t *ppos)
-{
-	struct ctl_table mq_table;
-	memcpy(&mq_table, table, sizeof(mq_table));
-	mq_table.data = get_mq(table);
-
-	return proc_dointvec_minmax(&mq_table, write, buffer,
-					lenp, ppos);
-}
-#else
-#define proc_mq_dointvec NULL
-#define proc_mq_dointvec_minmax NULL
-#endif
+#include <linux/stat.h>
+#include <linux/capability.h>
+#include <linux/slab.h>
 
 static int msg_max_limit_min = MIN_MSGMAX;
 static int msg_max_limit_max = HARD_MSGMAX;

@@ -55,14 +25,14 @@ static struct ctl_table mq_sysctls[] = {
 		.data		= &init_ipc_ns.mq_queues_max,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_mq_dointvec,
+		.proc_handler	= proc_dointvec,
 	},
 	{
 		.procname	= "msg_max",
 		.data		= &init_ipc_ns.mq_msg_max,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_mq_dointvec_minmax,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &msg_max_limit_min,
 		.extra2		= &msg_max_limit_max,
 	},

@@ -71,7 +41,7 @@ static struct ctl_table mq_sysctls[] = {
 		.data		= &init_ipc_ns.mq_msgsize_max,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_mq_dointvec_minmax,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &msg_maxsize_limit_min,
 		.extra2		= &msg_maxsize_limit_max,
 	},

@@ -80,7 +50,7 @@ static struct ctl_table mq_sysctls[] = {
 		.data		= &init_ipc_ns.mq_msg_default,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_mq_dointvec_minmax,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &msg_max_limit_min,
 		.extra2		= &msg_max_limit_max,
 	},

@@ -89,32 +59,89 @@ static struct ctl_table mq_sysctls[] = {
 		.data		= &init_ipc_ns.mq_msgsize_default,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_mq_dointvec_minmax,
+		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &msg_maxsize_limit_min,
 		.extra2		= &msg_maxsize_limit_max,
 	},
 	{}
 };
 
-static struct ctl_table mq_sysctl_dir[] = {
-	{
-		.procname	= "mqueue",
-		.mode		= 0555,
-		.child		= mq_sysctls,
-	},
-	{}
-};
+static struct ctl_table_set *
+set_lookup(struct ctl_table_root *root)
+{
+	return &current->nsproxy->ipc_ns->set;
+}
 
-static struct ctl_table mq_sysctl_root[] = {
-	{
-		.procname	= "fs",
-		.mode		= 0555,
-		.child		= mq_sysctl_dir,
-	},
-	{}
+static int set_is_seen(struct ctl_table_set *set)
+{
+	return &current->nsproxy->ipc_ns->set == set;
+}
+
+static int set_permissions(struct ctl_table_header *head, struct ctl_table *table)
+{
+	struct ipc_namespace *ns = container_of(head->set, struct ipc_namespace, set);

ipc/mq_sysctl.c +50 -2

+static struct ctl_table_root set_root = {
+	.lookup = set_lookup,
+	.permissions = set_permissions,
 };
 
-struct ctl_table_header *mq_register_sysctl_table(void)
+bool setup_mq_sysctls(struct ipc_namespace *ns)
 {
-	return register_sysctl_table(mq_sysctl_root);
+	struct ctl_table *tbl;
+
+	setup_sysctl_set(&ns->set, &set_root, set_is_seen);
+
+	tbl = kmemdup(mq_sysctls, sizeof(mq_sysctls), GFP_KERNEL);
+	if (tbl) {
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(mq_sysctls); i++) {
+			if (tbl[i].data == &init_ipc_ns.mq_queues_max)
+				tbl[i].data = &ns->mq_queues_max;
+
+			else if (tbl[i].data == &init_ipc_ns.mq_msg_max)
+				tbl[i].data = &ns->mq_msg_max;
+
+			else if (tbl[i].data == &init_ipc_ns.mq_msgsize_max)
+				tbl[i].data = &ns->mq_msgsize_max;
+
+			else if (tbl[i].data == &init_ipc_ns.mq_msg_default)
+				tbl[i].data = &ns->mq_msg_default;
+
+			else if (tbl[i].data == &init_ipc_ns.mq_msgsize_default)
+				tbl[i].data = &ns->mq_msgsize_default;
+			else
+				tbl[i].data = NULL;
+		}
+
+		ns->sysctls = __register_sysctl_table(&ns->set, "fs/mqueue", tbl);
+	}
+	if (!ns->sysctls) {
+		kfree(tbl);
+		retire_sysctl_set(&ns->set);
+		return false;
+	}
+
+	return true;
+}
+
+void retire_mq_sysctls(struct ipc_namespace *ns)
+{
+	struct ctl_table *tbl;
+
+	tbl = ns->sysctls->ctl_table_arg;
+	unregister_sysctl_table(ns->sysctls);
+	retire_sysctl_set(&ns->set);
+	kfree(tbl);
 }

ipc/mqueue.c +4 -6

index 5becca9be867..1b4a3be71636 100644
@@ -163,8 +163,6 @@ static void remove_notification(struct mqueue_inode_info *info);
 
 static struct kmem_cache *mqueue_inode_cachep;
 
-static struct ctl_table_header *mq_sysctl_table;
-
 static inline struct mqueue_inode_info *MQUEUE_I(struct inode *inode)
 {
 	return container_of(inode, struct mqueue_inode_info, vfs_inode);

@@ -1713,8 +1711,10 @@ static int __init init_mqueue_fs(void)
 	if (mqueue_inode_cachep == NULL)
 		return -ENOMEM;
 
-	/* ignore failures - they are not fatal */
-	mq_sysctl_table = mq_register_sysctl_table();
+	if (!setup_mq_sysctls(&init_ipc_ns)) {
+		pr_warn("sysctl registration failed\n");
+		return -ENOMEM;
+	}
 
 	error = register_filesystem(&mqueue_fs_type);
 	if (error)

@@ -1731,8 +1731,6 @@ static int __init init_mqueue_fs(void)
 out_filesystem:
 	unregister_filesystem(&mqueue_fs_type);
 out_sysctl:
-	if (mq_sysctl_table)
-		unregister_sysctl_table(mq_sysctl_table);
 	kmem_cache_destroy(mqueue_inode_cachep);
 	return error;
 }

ipc/namespace.c +0 -0

index ae83f0f2651b..f760243ca685 100644