ipc: Store mq and ipc sysctls in the ipc namespace

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

include/linux/ipc_namespace.h +12 -4

index b75395ec8d52..fa787d97d60a 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;

@@ -63,6 +64,9 @@ struct ipc_namespace {
 	unsigned int    mq_msg_default;
 	unsigned int    mq_msgsize_default;
 
+	struct ctl_table_set	mq_set;
+	struct ctl_table_header	*mq_sysctls;
+
 	/* user_ns which owns the ipc ns */
 	struct user_namespace *user_ns;
 	struct ucounts *ucounts;

@@ -169,14 +173,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 +66 ~~-55~~

165 lines not on this page · raw message

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 +6 -0

index ae83f0f2651b..f760243ca685 100644
@@ -59,6 +59,10 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns,
 	if (err)
 		goto fail_put;
 
+	err = -ENOMEM;
+	if (!setup_mq_sysctls(ns))
+		goto fail_put;
+
 	sem_init_ns(ns);
 	msg_init_ns(ns);
 	shm_init_ns(ns);

@@ -125,6 +129,8 @@ static void free_ipc_ns(struct ipc_namespace *ns)
 	msg_exit_ns(ns);
 	shm_exit_ns(ns);
 
+	retire_mq_sysctls(ns);
+
 	dec_ipc_namespaces(ns->ucounts);
 	put_user_ns(ns->user_ns);
 	ns_free_inum(&ns->ns);

Signature · 1 line

2.33.0

Alexey Gladkov

author

2022-02-14 18:20 UTC #2 lore raw

PATCH v4 2/2 ipc: Store ipc sysctls in the ipc namespace

The ipc sysctls are not available for modification inside the user namespace. Following the mqueue sysctls, we changed the implementation to be more userns friendly.

So far, the changes do not provide additional access to files. This will be done in a future patch.

---
 include/linux/ipc_namespace.h |  21 ++++
 ipc/ipc_sysctl.c              | 189 ++++++++++++++++++++++------------
 ipc/namespace.c               |   4 +
 3 files changed, 147 insertions(+), 67 deletions(-)

include/linux/ipc_namespace.h +21 -0

index fa787d97d60a..e3e8c8662b49 100644
@@ -67,6 +67,9 @@ struct ipc_namespace {
 	struct ctl_table_set	mq_set;
 	struct ctl_table_header	*mq_sysctls;
 
+	struct ctl_table_set	ipc_set;
+	struct ctl_table_header	*ipc_sysctls;
+
 	/* user_ns which owns the ipc ns */
 	struct user_namespace *user_ns;
 	struct ucounts *ucounts;

@@ -188,4 +191,22 @@ static inline bool setup_mq_sysctls(struct ipc_namespace *ns)
 }
 
 #endif /* CONFIG_POSIX_MQUEUE_SYSCTL */
+
+#ifdef CONFIG_SYSVIPC_SYSCTL
+
+bool setup_ipc_sysctls(struct ipc_namespace *ns);
+void retire_ipc_sysctls(struct ipc_namespace *ns);
+
+#else /* CONFIG_SYSVIPC_SYSCTL */
+
+static inline void retire_ipc_sysctls(struct ipc_namespace *ns)
+{
+}
+
+static inline bool setup_ipc_sysctls(struct ipc_namespace *ns)
+{
+	return true;
+}
+
+#endif /* CONFIG_SYSVIPC_SYSCTL */
 #endif

ipc/ipc_sysctl.c +122 ~~-67~~

302 lines not on this page · raw message

ipc/namespace.c +4 -0

index f760243ca685..754f3237194a 100644
@@ -63,6 +63,9 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns,
 	if (!setup_mq_sysctls(ns))
 		goto fail_put;
 
+	if (!setup_ipc_sysctls(ns))
+		goto fail_put;
+
 	sem_init_ns(ns);
 	msg_init_ns(ns);
 	shm_init_ns(ns);

@@ -130,6 +133,7 @@ static void free_ipc_ns(struct ipc_namespace *ns)
 	shm_exit_ns(ns);
 
 	retire_mq_sysctls(ns);
+	retire_ipc_sysctls(ns);
 
 	dec_ipc_namespaces(ns->ucounts);
 	put_user_ns(ns->user_ns);

Signature · 1 line

2.33.0

Eric W. Biederman

2022-03-23 20:24 UTC #3 lore raw

[GIT PULL] ipc: Bind to the ipc namespace at open time.

Linus,

Please pull the per-namespace-ipc-sysctls-for-v5.18 tag from the git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git per-namespace-ipc-sysctls-for-v5.18
HEAD: 1f5c135ee509e89e0cc274333a65f73c62cb16e5 ipc: Store ipc sysctls in the ipc namespace

The per ipc namespace sysctls have been imperfect since they were implemented. Instead of binding to the ipc namespace of the opener of the file the code bound to the ipc namespace of the writer of the file.

This short series of changes addresses that old deficiency in the code.

Alexey Gladkov (2):
ipc: Store mqueue sysctls in the ipc namespace
ipc: Store ipc sysctls in the ipc namespace

 include/linux/ipc_namespace.h |  37 ++++++++-
 ipc/ipc_sysctl.c              | 189 +++++++++++++++++++++++++++---------------
 ipc/mq_sysctl.c               | 121 +++++++++++++++------------
 ipc/mqueue.c                  |  10 +--
 ipc/namespace.c               |  10 +++
 5 files changed, 235 insertions(+), 132 deletions(-)

Signed-off-by "Eric W. Biederman" <[email protected]>

Eric

Linus Torvalds

2022-03-24 18:12 UTC #4 lore raw

Re: [GIT PULL] ipc: Bind to the ipc namespace at open time.

Quoted 2 lines · Eric W. Biederman

Please pull the per-namespace-ipc-sysctls-for-v5.18 tag from the git tree:

Ugh.

I pulled this. Then I stared at it for a long time.

And then I decided that this is too ugly to live.

I'm sorry. I think Alexey has probably spent a fair amount of time on it, but I really think the sysctl code needs to be cleaned up way more than this.

The old code was horribly hacky too, but that setup_ipc_sysctls() (and setup_mq_sysctls()) thing that copies the whole sysctls table, and then walks it entry by entry to modify it, is just too ugly for words.

And then it hides things in .extra1, and because it does that it can't use the normal "extra1 and extra2 contains the limits" so then at write() time it copies it into a local one AGAIN only to set the limits back so that it can call the normal routines again.

Not ok.

Yes, yes, the old code did some similar things - to set the 'data' pointer. That was disgusting too. Don't get me wrong - the existing code was nasty too. But this took nasty code, and doubled down on it.

I really think this is a fundamental problem, and needs a more fundamental fix than adding more and more of these kinds of nasty hacks.

And yes, that fundamental fix is almost certainly to pass in 'struct cred *' to all those sysctl helper functions.

Then, when you do the actual 'sysctl()' system calls, you pass in 'current_cred()".

And the /proc users would pass in file->f_cred.

And yes, that patch might be quite messy, because we have quite a lot of those random .proc_handler users.

But *most* of them by far (at least in random code) use the standard proc_dointvec_minmax() and friends, and won't even notice.

And then the ones that are about namespace issues will have to continue to do the nasty "make a copy and update the data pointer", but *MAYBE* we could also introduce the notion of an "offset" to those proc_dointvec_minmax() things to help them out (and at least avoid the "make a copy" thing).

Anyway, I really think we must not make that sysctl code even uglier than it is today, and I think we need to move towards a model that actually makes sense. And that "pass in the right cred" is the only sensible model I can see.

I haven't tried to create such a patch, and maybe Alexey already tried to do something like that and it turned out to be too ugly for words and that's why these nasty patches happened.

But at least for now, I can't with a good conscience pull this.

Sorry,
Linus

Eric W. Biederman

2022-03-24 21:49 UTC #5 lore raw

Re: [GIT PULL] ipc: Bind to the ipc namespace at open time.

Quoted 10 lines · Linus Torvalds

Quoted 2 lines · Eric W. Biederman
Please pull the per-namespace-ipc-sysctls-for-v5.18 tag from the git tree:
Ugh.
I pulled this. Then I stared at it for a long time.
And then I decided that this is too ugly to live.

Quoted 7 lines

I'm sorry. I think Alexey has probably spent a fair amount of time on it, but I really think the sysctl code needs to be cleaned up way more than this.
The old code was horribly hacky too, but that setup_ipc_sysctls() (and setup_mq_sysctls()) thing that copies the whole sysctls table, and then walks it entry by entry to modify it, is just too ugly for words.

I am not convinced when it comes to the .data pointers it is too ugly to live. The only realistic alternative is doing something with the offset to adjust the pointers to the per namespace values.

Fundamentally the copy is needed because in the general case (which is present in the networking stack) we only show a subset of the sysctls in anything other than the initial namespace. That is necessary as some sysctls have not been shown to be safe for unprivileged users to write.

So there does need to be a separate array of ctl_table entries per namespace, and the data pointers in those entries need to be adjusted to the per namespace values.

Quoted 6 lines

And then it hides things in .extra1, and because it does that it can't use the normal "extra1 and extra2 contains the limits" so then at write() time it copies it into a local one AGAIN only to set the limits back so that it can call the normal routines again.
Not ok.

I agree. That is a bit ugly, and I missed it when I was looking at the code.

The mq_sysctl.c changes look reasonable to me. There are no strange games being played with .extra1 or .extra2. All I see happening in mq_sysctl.c is boiler plate to make things work.

Looking at ipc_sysctl.c I agree playing games with .extra1 and .extra2 is ugly and error prone. Worse I noticed when reading it through that proc_ipc_sem_dointvec passes sem_check_semmni current->nsproxy->ipc_ns instead of passing the computed ns value. A bug but not a regression.

I went through the code and we don't need to play games with .extra1 to get the namespace value all we need to call container_of with the .data member. That takes a little extra boiler plate especially for the checkpoint_restore case.

The checkpoint_restore sysctls arguably need to be done differently so that the permissions can be checked at open time instead of at write time. That would eliminate the need to play games with foobar.

Quoted 5 lines

Anyway, I really think we must not make that sysctl code even uglier than it is today, and I think we need to move towards a model that actually makes sense. And that "pass in the right cred" is the only sensible model I can see.

I don't see how passing in struct cred does anything helpful. The namespace can not be found in struct cred.

Now this set of changes is a setup to allow implementing .set_ownership and .permission methods so that we can allow the namespace root to write to sysctls it is safe for the namespace root to be able to write to. Even there I don't see how passing in a struct cred would help.

Given that there are no regressions I don't see it even being helpful to not merge this code.

I did look and there are some cleanup pretty significant cleanup opportunities, by using container_of on .data, which avoids the need to stuff a namespace parameter in .extra1.

There are also significant cleanup opportunities in implementing a .permission method that will allow the checkpoint_restart sysctls to perform all of their permission checks at open time, and not need any other special code.

But all of these cleanups I see are moving forward from the current point so I don't see why we would not want to merge the code as is and then get the tested versions of my cleanups below.

ipc/ipc_sysctl.c | 81 +++++++++++++++++++++++++-------------------------------

 1 file changed, 36 insertions(+), 45 deletions(-)

ipc/ipc_sysctl.c +36 ~~-45~~

index 15210ac47e9e..e2209d48db04 100644
@@ -19,16 +19,11 @@
 static int proc_ipc_dointvec_minmax_orphans(struct ctl_table *table, int write,
 		void *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct ipc_namespace *ns = table->extra1;
-	struct ctl_table ipc_table;
+	struct ipc_namespace *ns =
+		container_of(table->data, struct ipc_namespace, shm_rmid_forced);
 	int err;
 
-	memcpy(&ipc_table, table, sizeof(ipc_table));
-
-	ipc_table.extra1 = SYSCTL_ZERO;
-	ipc_table.extra2 = SYSCTL_ONE;
-
-	err = proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
+	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 
 	if (err < 0)
 		return err;

@@ -55,20 +50,15 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 	void *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct ipc_namespace *ns = table->extra1;
-	struct ctl_table ipc_table;
+	struct ipc_namespace *ns =
+		container_of(table->data, struct ipc_namespace, sem_ctls);
 	int ret, semmni;
 
-	memcpy(&ipc_table, table, sizeof(ipc_table));
-
-	ipc_table.extra1 = NULL;
-	ipc_table.extra2 = NULL;
-
 	semmni = ns->sem_ctls[3];
 	ret = proc_dointvec(table, write, buffer, lenp, ppos);
 
 	if (!ret)
-		ret = sem_check_semmni(current->nsproxy->ipc_ns);
+		ret = sem_check_semmni(ns);
 
 	/*
 	 * Reset the semmni value if an error happens.

@@ -78,24 +68,6 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 	return ret;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-static int proc_ipc_dointvec_minmax_checkpoint_restore(struct ctl_table *table,
-		int write, void *buffer, size_t *lenp, loff_t *ppos)
-{
-	struct ipc_namespace *ns = table->extra1;
-	struct ctl_table ipc_table;
-
-	if (write && !checkpoint_restore_ns_capable(ns->user_ns))
-		return -EPERM;
-
-	memcpy(&ipc_table, table, sizeof(ipc_table));
-
-	ipc_table.extra1 = SYSCTL_ZERO;
-	ipc_table.extra2 = SYSCTL_INT_MAX;
-
-	return proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
-}
-#endif
 
 int ipc_mni = IPCMNI;
 int ipc_mni_shift = IPCMNI_SHIFT;

@@ -131,6 +103,8 @@ static struct ctl_table ipc_sysctls[] = {
 		.maxlen		= sizeof(init_ipc_ns.shm_rmid_forced),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax_orphans,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
 	},
 	{
 		.procname	= "msgmax",

@@ -180,22 +154,28 @@ static struct ctl_table ipc_sysctls[] = {
 		.procname	= "sem_next_id",
 		.data		= &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
-		.mode		= 0666,
-		.proc_handler	= proc_ipc_dointvec_minmax_checkpoint_restore,
+		.mode		= 0444,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
 		.procname	= "msg_next_id",
 		.data		= &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
-		.mode		= 0666,
-		.proc_handler	= proc_ipc_dointvec_minmax_checkpoint_restore,
+		.mode		= 0444,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
 		.procname	= "shm_next_id",
 		.data		= &init_ipc_ns.ids[IPC_SHM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id),
-		.mode		= 0666,
-		.proc_handler	= proc_ipc_dointvec_minmax_checkpoint_restore,
+		.mode		= 0444,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 #endif
 	{}

@@ -211,8 +191,24 @@ static int set_is_seen(struct ctl_table_set *set)
 	return &current->nsproxy->ipc_ns->ipc_set == set;
 }
 
+static int ipc_permissions(struct ctl_table_header *head, struct ctl_table *table)
+{
+	struct ipc_namespace *ns = current->nsproxy->ipc_ns;
+	int mode = table->mode;
+
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	if (((table->data == &ns->ids[IPC_SEM_IDS].next_id) ||
+	     (table->data == &ns->ids[IPC_MSG_IDS].next_id) ||
+	     (table->data == &ns->ids[IPC_SHM_IDS].next_id)) &&
+	    checkpoint_restore_ns_capable(ns->user_ns))
+		mode = 0666;
+#endif
+	return mode;
+}
+
 static struct ctl_table_root set_root = {
 	.lookup = set_lookup,
+	.permissions = ipc_permissions,
 };
 
 bool setup_ipc_sysctls(struct ipc_namespace *ns)

@@ -237,7 +233,6 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 
 			} else if (tbl[i].data == &init_ipc_ns.shm_rmid_forced) {
 				tbl[i].data = &ns->shm_rmid_forced;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.msg_ctlmax) {
 				tbl[i].data = &ns->msg_ctlmax;

@@ -250,19 +245,15 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 
 			} else if (tbl[i].data == &init_ipc_ns.sem_ctls) {
 				tbl[i].data = &ns->sem_ctls;
-				tbl[i].extra1 = ns;
 #ifdef CONFIG_CHECKPOINT_RESTORE
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SEM_IDS].next_id;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_MSG_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_MSG_IDS].next_id;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SHM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SHM_IDS].next_id;
-				tbl[i].extra1 = ns;
 #endif
 			} else {
 				tbl[i].data = NULL;

Eric

Linus Torvalds

2022-03-24 22:17 UTC #6 lore raw

Re: [GIT PULL] ipc: Bind to the ipc namespace at open time.

Quoted 3 lines · Eric W. Biederman

I don't see how passing in struct cred does anything helpful. The namespace can not be found in struct cred.

Duh. For some reason I thought we had it there, but yeah, that only contains the user-namespace.

The rest is in tsk->nsproxy.

So it would have to be squirrelled away in some other way.

Quoted 3 lines

But all of these cleanups I see are moving forward from the current point so I don't see why we would not want to merge the code as is and then get the tested versions of my cleanups below.

What part of "that code is too ugly to live" did not end up registering.

Maybe it can be cleaned up. But it had better be cleaned up before I pull it.

Linus

Alexey Gladkov

author

2022-03-25 12:11 UTC #7 lore raw

Re: [GIT PULL] ipc: Bind to the ipc namespace at open time.

[1] https://lore.kernel.org/lkml/0f0408bb7fbf3187966a9bf19a98642a5d9669fe.1641225760.git.legion@kernel.org/

Quoted 6 lines · Linus Torvalds

Quoted 2 lines · Eric W. Biederman
Please pull the per-namespace-ipc-sysctls-for-v5.18 tag from the git tree:
Ugh.

Quoted 7 lines

I pulled this. Then I stared at it for a long time.
And then I decided that this is too ugly to live.
I'm sorry. I think Alexey has probably spent a fair amount of time on it, but I really think the sysctl code needs to be cleaned up way more than this.

Apparently it's my fault that the purpose of these changes is not clear. I did this refactoring not for the sake of refactoring as such.

In my original patch [1], I was trying to fix the situation where the user cannot change the /proc/sys/fs/mqueue/* options inside rootless container.

But then I split the changes into refactoring which fixes the hack and permission changes which I wanted to discuss and propose later.

Quoted 44 lines

The old code was horribly hacky too, but that setup_ipc_sysctls() (and setup_mq_sysctls()) thing that copies the whole sysctls table, and then walks it entry by entry to modify it, is just too ugly for words.
And then it hides things in .extra1, and because it does that it can't use the normal "extra1 and extra2 contains the limits" so then at write() time it copies it into a local one AGAIN only to set the limits back so that it can call the normal routines again.
Not ok.
Yes, yes, the old code did some similar things - to set the 'data' pointer. That was disgusting too. Don't get me wrong - the existing code was nasty too. But this took nasty code, and doubled down on it.
I really think this is a fundamental problem, and needs a more fundamental fix than adding more and more of these kinds of nasty hacks.
And yes, that fundamental fix is almost certainly to pass in 'struct cred *' to all those sysctl helper functions.
Then, when you do the actual 'sysctl()' system calls, you pass in 'current_cred()".
And the /proc users would pass in file->f_cred.
And yes, that patch might be quite messy, because we have quite a lot of those random .proc_handler users.
But *most* of them by far (at least in random code) use the standard proc_dointvec_minmax() and friends, and won't even notice.
And then the ones that are about namespace issues will have to continue to do the nasty "make a copy and update the data pointer", but *MAYBE* we could also introduce the notion of an "offset" to those proc_dointvec_minmax() things to help them out (and at least avoid the "make a copy" thing).
Anyway, I really think we must not make that sysctl code even uglier than it is today, and I think we need to move towards a model that actually makes sense. And that "pass in the right cred" is the only sensible model I can see.

Quoted 8 lines

I haven't tried to create such a patch, and maybe Alexey already tried to do something like that and it turned out to be too ugly for words and that's why these nasty patches happened.
But at least for now, I can't with a good conscience pull this.
Sorry,
Linus

OK. I'll try to come up with some other solution.

Signature · 1 line

Rgrds, legion

Alexey Gladkov

author

2022-04-22 12:54 UTC #8 lore raw

cover ipc: Remove extra1 field abuse to pass ipc namespace

Link https://lore.kernel.org/lkml/[email protected]/ open here

These patches are made on top of branch:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git per-namespace-ipc-sysctls-for-v5.18

After discussion, I took into account Eric Biederman's fixes. With these changes, the hacks of passing namespace through .extra1 are not required.

With the current design of sysctl dynamic memory allocation is necessary.

Yes, Linus, these changes are not the refactoring you were talking about, but I plan to try to do such a refactoring in the my next patchset.

Alexey Gladkov (4):
ipc: Remove extra1 field abuse to pass ipc namespace
ipc: Use proper ipc namespace
ipc: Check permissions for checkpoint_restart sysctls at open time
ipc: Remove extra braces

 ipc/ipc_sysctl.c | 108 +++++++++++++++++++++--------------------------
 1 file changed, 49 insertions(+), 59 deletions(-)

Signature · 1 line

2.33.2

Alexey Gladkov

author

2022-04-22 12:54 UTC #9 lore raw

PATCH 1/4 ipc: Remove extra1 field abuse to pass ipc namespace

Eric Biederman pointed out that using .extra1 to pass ipc namespace looks like an ugly hack and there is a better solution.

Signed-off-by Eric W. Biederman <[email protected]>

---
 ipc/ipc_sysctl.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

ipc/ipc_sysctl.c +8 ~~-18~~

index 15210ac47e9e..eb7ba8e0a355 100644
@@ -19,16 +19,11 @@
 static int proc_ipc_dointvec_minmax_orphans(struct ctl_table *table, int write,
 		void *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct ipc_namespace *ns = table->extra1;
-	struct ctl_table ipc_table;
+	struct ipc_namespace *ns =
+		container_of(table->data, struct ipc_namespace, shm_rmid_forced);
 	int err;
 
-	memcpy(&ipc_table, table, sizeof(ipc_table));
-
-	ipc_table.extra1 = SYSCTL_ZERO;
-	ipc_table.extra2 = SYSCTL_ONE;
-
-	err = proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
+	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 
 	if (err < 0)
 		return err;

@@ -55,20 +50,15 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 	void *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct ipc_namespace *ns = table->extra1;
-	struct ctl_table ipc_table;
+	struct ipc_namespace *ns =
+		container_of(table->data, struct ipc_namespace, sem_ctls);
 	int ret, semmni;
 
-	memcpy(&ipc_table, table, sizeof(ipc_table));
-
-	ipc_table.extra1 = NULL;
-	ipc_table.extra2 = NULL;
-
 	semmni = ns->sem_ctls[3];
 	ret = proc_dointvec(table, write, buffer, lenp, ppos);
 
 	if (!ret)
-		ret = sem_check_semmni(current->nsproxy->ipc_ns);
+		ret = sem_check_semmni(ns);
 
 	/*
 	 * Reset the semmni value if an error happens.

@@ -131,6 +121,8 @@ static struct ctl_table ipc_sysctls[] = {
 		.maxlen		= sizeof(init_ipc_ns.shm_rmid_forced),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax_orphans,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
 	},
 	{
 		.procname	= "msgmax",

@@ -237,7 +229,6 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 
 			} else if (tbl[i].data == &init_ipc_ns.shm_rmid_forced) {
 				tbl[i].data = &ns->shm_rmid_forced;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.msg_ctlmax) {
 				tbl[i].data = &ns->msg_ctlmax;

@@ -250,7 +241,6 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 
 			} else if (tbl[i].data == &init_ipc_ns.sem_ctls) {
 				tbl[i].data = &ns->sem_ctls;
-				tbl[i].extra1 = ns;
 #ifdef CONFIG_CHECKPOINT_RESTORE
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SEM_IDS].next_id;

Signature · 1 line

2.33.2

Eric W. Biederman

2022-05-02 16:08 UTC #10 lore raw

Re: [PATCH v1 1/4] ipc: Remove extra1 field abuse to pass ipc namespace

Quoted 23 lines · Alexey Gladkov

Eric Biederman pointed out that using .extra1 to pass ipc namespace looks like an ugly hack and there is a better solution.
Link https://lore.kernel.org/lkml/[email protected]/ open here
Signed-off-by Eric W. Biederman <[email protected]>
Signed-off-by Alexey Gladkov <[email protected]>
---
 ipc/ipc_sysctl.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)
ipc/ipc_sysctl.c +2 -3
index 15210ac47e9e..eb7ba8e0a355 100644
@@ -55,20 +50,15 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 	void *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct ipc_namespace *ns = table->extra1;
-	struct ctl_table ipc_table;
+	struct ipc_namespace *ns =
+		container_of(table->data, struct ipc_namespace, sem_ctls);
 	int ret, semmni;
 
-	memcpy(&ipc_table, table, sizeof(ipc_table));

ipc/ipc_sysctl.c static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, · line 61

Quoted 10 lines

ipc/ipc_sysctl.c +1 -5

-
-	ipc_table.extra1 = NULL;
-	ipc_table.extra2 = NULL;
-
 	semmni = ns->sem_ctls[3];
 	ret = proc_dointvec(table, write, buffer, lenp, ppos);
 
 	if (!ret)
-		ret = sem_check_semmni(current->nsproxy->ipc_ns);
+		ret = sem_check_semmni(ns);

		^^^^^^^^^^^^^^^^^^^^^^^^^^^

Can you break this one line change into a separate patch?

It is a bug fix so that the entire function uses the same ns value. I expect the change would read easier if the change was separate.

ipc/ipc_sysctl.c static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, · line 64

Quoted 3 lines

ipc/ipc_sysctl.c +0 -0

 	/*
 	 * Reset the semmni value if an error happens.

Eric

Alexey Gladkov

author

2022-04-22 12:55 UTC #11 lore raw

PATCH 2/4 ipc: Use proper ipc namespace

As Eric Biederman pointed out, changing the namespace broke checkpoint restore. I have reverted my previous changes.

Signed-off-by Eric W. Biederman <[email protected]>

---
 ipc/ipc_sysctl.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

ipc/ipc_sysctl.c +1 -4

index eb7ba8e0a355..ff99d0305a5b 100644
@@ -72,7 +72,7 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 static int proc_ipc_dointvec_minmax_checkpoint_restore(struct ctl_table *table,
 		int write, void *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct ipc_namespace *ns = table->extra1;
+	struct ipc_namespace *ns = current->nsproxy->ipc_ns;
 	struct ctl_table ipc_table;
 
 	if (write && !checkpoint_restore_ns_capable(ns->user_ns))

@@ -244,15 +244,12 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 #ifdef CONFIG_CHECKPOINT_RESTORE
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SEM_IDS].next_id;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_MSG_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_MSG_IDS].next_id;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SHM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SHM_IDS].next_id;
-				tbl[i].extra1 = ns;
 #endif
 			} else {
 				tbl[i].data = NULL;

Signature · 1 line

2.33.2

Eric W. Biederman

2022-05-02 16:09 UTC #12 lore raw

Re: [PATCH v1 2/4] ipc: Use proper ipc namespace

Quoted 2 lines · Alexey Gladkov

As Eric Biederman pointed out, changing the namespace broke checkpoint restore. I have reverted my previous changes.

Can you remind me the bug that is being fixed here?

I am probably just going to fast to see it, but it would be good to have it described in the commit comment.

Thanks,
Eric

Quoted 36 lines

Signed-off-by Eric W. Biederman <[email protected]>

Link https://lore.kernel.org/lkml/[email protected]/ open here

---
 ipc/ipc_sysctl.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

ipc/ipc_sysctl.c +1 -4

index eb7ba8e0a355..ff99d0305a5b 100644
@@ -72,7 +72,7 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 static int proc_ipc_dointvec_minmax_checkpoint_restore(struct ctl_table *table,
 		int write, void *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct ipc_namespace *ns = table->extra1;
+	struct ipc_namespace *ns = current->nsproxy->ipc_ns;
 	struct ctl_table ipc_table;
 
 	if (write && !checkpoint_restore_ns_capable(ns->user_ns))

@@ -244,15 +244,12 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 #ifdef CONFIG_CHECKPOINT_RESTORE
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SEM_IDS].next_id;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_MSG_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_MSG_IDS].next_id;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SHM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SHM_IDS].next_id;
-				tbl[i].extra1 = ns;
 #endif
 			} else {
 				tbl[i].data = NULL;

Alexey Gladkov

author

2022-05-03 15:06 UTC #13 lore raw

Re: [PATCH v1 2/4] ipc: Use proper ipc namespace

Quoted 9 lines · Eric W. Biederman

Quoted 2 lines · Alexey Gladkov
As Eric Biederman pointed out, changing the namespace broke checkpoint restore. I have reverted my previous changes.
Can you remind me the bug that is being fixed here?
I am probably just going to fast to see it, but it would be good to have it described in the commit comment.

It was preparation for the "ipc: Check permissions for checkpoint_restart sysctls at open time". I wanted to split the commit and make it more readable, but it looks like I just confused everyone. This change should be part of the next commit.

Here is a new version of the patchset where I moved the bugfix to a separate commit as you requested in the next email.

Does that look better?

Alexey Gladkov (4):
ipc: Use the same namespace to modify and validate
ipc: Remove extra1 field abuse to pass ipc namespace
ipc: Check permissions for checkpoint_restart sysctls at open time
ipc: Remove extra braces

 ipc/ipc_sysctl.c | 108 +++++++++++++++++++++--------------------------
 1 file changed, 49 insertions(+), 59 deletions(-)

Signature · 1 line

2.33.3

Alexey Gladkov

author

2022-05-03 14:09 UTC #14 lore raw

PATCH v2 1/4 ipc: Use the same namespace to modify and validate

In the 1f5c135ee509 ("ipc: Store ipc sysctls in the ipc namespace") I missed that in addition to the modification of sem_ctls[3], the change is validated. This validation must occur in the same namespace.

Fixes 1f5c135ee509 ("ipc: Store ipc sysctls in the ipc namespace")

---
 ipc/ipc_sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ipc/ipc_sysctl.c +1 -1

index 15210ac47e9e..d1d5204cf589 100644
@@ -68,7 +68,7 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 	ret = proc_dointvec(table, write, buffer, lenp, ppos);
 
 	if (!ret)
-		ret = sem_check_semmni(current->nsproxy->ipc_ns);
+		ret = sem_check_semmni(ns);
 
 	/*
 	 * Reset the semmni value if an error happens.

Signature · 1 line

2.33.3

Alexey Gladkov

author

2022-05-03 16:12 UTC #15 lore raw

PATCH v2 2/4 ipc: Remove extra1 field abuse to pass ipc namespace

Link https://lore.kernel.org/lkml/[email protected]/ open here

Eric Biederman pointed out that using .extra1 to pass ipc namespace looks like an ugly hack and there is a better solution. We can get the ipc_namespace using the .data field.

Fixes 1f5c135ee509 ("ipc: Store ipc sysctls in the ipc namespace")

Signed-off-by Eric W. Biederman <[email protected]>

---
 ipc/ipc_sysctl.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

ipc/ipc_sysctl.c +7 ~~-17~~

index d1d5204cf589..eb7ba8e0a355 100644
@@ -19,16 +19,11 @@
 static int proc_ipc_dointvec_minmax_orphans(struct ctl_table *table, int write,
 		void *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct ipc_namespace *ns = table->extra1;
-	struct ctl_table ipc_table;
+	struct ipc_namespace *ns =
+		container_of(table->data, struct ipc_namespace, shm_rmid_forced);
 	int err;
 
-	memcpy(&ipc_table, table, sizeof(ipc_table));
-
-	ipc_table.extra1 = SYSCTL_ZERO;
-	ipc_table.extra2 = SYSCTL_ONE;
-
-	err = proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
+	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 
 	if (err < 0)
 		return err;

@@ -55,15 +50,10 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 	void *buffer, size_t *lenp, loff_t *ppos)
 {
-	struct ipc_namespace *ns = table->extra1;
-	struct ctl_table ipc_table;
+	struct ipc_namespace *ns =
+		container_of(table->data, struct ipc_namespace, sem_ctls);
 	int ret, semmni;
 
-	memcpy(&ipc_table, table, sizeof(ipc_table));
-
-	ipc_table.extra1 = NULL;
-	ipc_table.extra2 = NULL;
-
 	semmni = ns->sem_ctls[3];
 	ret = proc_dointvec(table, write, buffer, lenp, ppos);

@@ -131,6 +121,8 @@ static struct ctl_table ipc_sysctls[] = {
 		.maxlen		= sizeof(init_ipc_ns.shm_rmid_forced),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax_orphans,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
 	},
 	{
 		.procname	= "msgmax",

@@ -237,7 +229,6 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 
 			} else if (tbl[i].data == &init_ipc_ns.shm_rmid_forced) {
 				tbl[i].data = &ns->shm_rmid_forced;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.msg_ctlmax) {
 				tbl[i].data = &ns->msg_ctlmax;

@@ -250,7 +241,6 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 
 			} else if (tbl[i].data == &init_ipc_ns.sem_ctls) {
 				tbl[i].data = &ns->sem_ctls;
-				tbl[i].extra1 = ns;
 #ifdef CONFIG_CHECKPOINT_RESTORE
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SEM_IDS].next_id;

Signature · 1 line

2.33.3

Alexey Gladkov

author

2022-05-03 15:21 UTC #16 lore raw

PATCH v2 3/4 ipc: Check permissions for checkpoint_restart sysctls at open time

Link https://lore.kernel.org/lkml/[email protected]/ open here

As Eric Biederman pointed out, it is possible not to use a custom proc_handler and check permissions for every write, but to use a .permission handler. That will allow the checkpoint_restart sysctls to perform all of their permission checks at open time, and not need any other special code.

Fixes 1f5c135ee509 ("ipc: Store ipc sysctls in the ipc namespace")

Signed-off-by Eric W. Biederman <[email protected]>

---
 ipc/ipc_sysctl.c | 57 ++++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

ipc/ipc_sysctl.c +29 ~~-28~~

index eb7ba8e0a355..5a58598d48c8 100644
@@ -68,25 +68,6 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 	return ret;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-static int proc_ipc_dointvec_minmax_checkpoint_restore(struct ctl_table *table,
-		int write, void *buffer, size_t *lenp, loff_t *ppos)
-{
-	struct ipc_namespace *ns = table->extra1;
-	struct ctl_table ipc_table;
-
-	if (write && !checkpoint_restore_ns_capable(ns->user_ns))
-		return -EPERM;
-
-	memcpy(&ipc_table, table, sizeof(ipc_table));
-
-	ipc_table.extra1 = SYSCTL_ZERO;
-	ipc_table.extra2 = SYSCTL_INT_MAX;
-
-	return proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
-}
-#endif
-
 int ipc_mni = IPCMNI;
 int ipc_mni_shift = IPCMNI_SHIFT;
 int ipc_min_cycle = RADIX_TREE_MAP_SIZE;

@@ -172,22 +153,28 @@ static struct ctl_table ipc_sysctls[] = {
 		.procname	= "sem_next_id",
 		.data		= &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
-		.mode		= 0666,
-		.proc_handler	= proc_ipc_dointvec_minmax_checkpoint_restore,
+		.mode		= 0444,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
 		.procname	= "msg_next_id",
 		.data		= &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
-		.mode		= 0666,
-		.proc_handler	= proc_ipc_dointvec_minmax_checkpoint_restore,
+		.mode		= 0444,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
 		.procname	= "shm_next_id",
 		.data		= &init_ipc_ns.ids[IPC_SHM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id),
-		.mode		= 0666,
-		.proc_handler	= proc_ipc_dointvec_minmax_checkpoint_restore,
+		.mode		= 0444,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 #endif
 	{}

@@ -203,8 +190,25 @@ static int set_is_seen(struct ctl_table_set *set)
 	return &current->nsproxy->ipc_ns->ipc_set == set;
 }
 
+static int ipc_permissions(struct ctl_table_header *head, struct ctl_table *table)
+{
+	int mode = table->mode;
+
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	struct ipc_namespace *ns = current->nsproxy->ipc_ns;
+
+	if (((table->data == &ns->ids[IPC_SEM_IDS].next_id) ||
+	     (table->data == &ns->ids[IPC_MSG_IDS].next_id) ||
+	     (table->data == &ns->ids[IPC_SHM_IDS].next_id)) &&
+	    checkpoint_restore_ns_capable(ns->user_ns))
+		mode = 0666;
+#endif
+	return mode;
+}
+
 static struct ctl_table_root set_root = {
 	.lookup = set_lookup,
+	.permissions = ipc_permissions,
 };
 
 bool setup_ipc_sysctls(struct ipc_namespace *ns)

@@ -244,15 +248,12 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 #ifdef CONFIG_CHECKPOINT_RESTORE
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SEM_IDS].next_id;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_MSG_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_MSG_IDS].next_id;
-				tbl[i].extra1 = ns;
 
 			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SHM_IDS].next_id) {
 				tbl[i].data = &ns->ids[IPC_SHM_IDS].next_id;
-				tbl[i].extra1 = ns;
 #endif
 			} else {
 				tbl[i].data = NULL;

Signature · 1 line

2.33.3

Alexey Gladkov

author

2022-05-03 15:18 UTC #17 lore raw

PATCH v2 4/4 ipc: Remove extra braces

Fix coding style. In the previous commit, I added braces because, in addition to changing .data, .extra1 also changed. Now this is not needed.

Fixes 1f5c135ee509 ("ipc: Store ipc sysctls in the ipc namespace")

---
 ipc/ipc_sysctl.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

ipc/ipc_sysctl.c +12 ~~-13~~

index 5a58598d48c8..ef313ecfb53a 100644
@@ -222,42 +222,41 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 		int i;
 
 		for (i = 0; i < ARRAY_SIZE(ipc_sysctls); i++) {
-			if (tbl[i].data == &init_ipc_ns.shm_ctlmax) {
+			if (tbl[i].data == &init_ipc_ns.shm_ctlmax)
 				tbl[i].data = &ns->shm_ctlmax;
 
-			} else if (tbl[i].data == &init_ipc_ns.shm_ctlall) {
+			else if (tbl[i].data == &init_ipc_ns.shm_ctlall)
 				tbl[i].data = &ns->shm_ctlall;
 
-			} else if (tbl[i].data == &init_ipc_ns.shm_ctlmni) {
+			else if (tbl[i].data == &init_ipc_ns.shm_ctlmni)
 				tbl[i].data = &ns->shm_ctlmni;
 
-			} else if (tbl[i].data == &init_ipc_ns.shm_rmid_forced) {
+			else if (tbl[i].data == &init_ipc_ns.shm_rmid_forced)
 				tbl[i].data = &ns->shm_rmid_forced;
 
-			} else if (tbl[i].data == &init_ipc_ns.msg_ctlmax) {
+			else if (tbl[i].data == &init_ipc_ns.msg_ctlmax)
 				tbl[i].data = &ns->msg_ctlmax;
 
-			} else if (tbl[i].data == &init_ipc_ns.msg_ctlmni) {
+			else if (tbl[i].data == &init_ipc_ns.msg_ctlmni)
 				tbl[i].data = &ns->msg_ctlmni;
 
-			} else if (tbl[i].data == &init_ipc_ns.msg_ctlmnb) {
+			else if (tbl[i].data == &init_ipc_ns.msg_ctlmnb)
 				tbl[i].data = &ns->msg_ctlmnb;
 
-			} else if (tbl[i].data == &init_ipc_ns.sem_ctls) {
+			else if (tbl[i].data == &init_ipc_ns.sem_ctls)
 				tbl[i].data = &ns->sem_ctls;
 #ifdef CONFIG_CHECKPOINT_RESTORE
-			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id) {
+			else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id)
 				tbl[i].data = &ns->ids[IPC_SEM_IDS].next_id;
 
-			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_MSG_IDS].next_id) {
+			else if (tbl[i].data == &init_ipc_ns.ids[IPC_MSG_IDS].next_id)
 				tbl[i].data = &ns->ids[IPC_MSG_IDS].next_id;
 
-			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SHM_IDS].next_id) {
+			else if (tbl[i].data == &init_ipc_ns.ids[IPC_SHM_IDS].next_id)
 				tbl[i].data = &ns->ids[IPC_SHM_IDS].next_id;
 #endif
-			} else {
+			else
 				tbl[i].data = NULL;
-			}
 		}
 
 		ns->ipc_sysctls = __register_sysctl_table(&ns->ipc_set, "kernel", tbl);

Signature · 1 line

2.33.3

Alexey Gladkov

author

2022-04-22 12:54 UTC #18 lore raw

PATCH 3/4 ipc: Check permissions for checkpoint_restart sysctls at open time

Link https://lore.kernel.org/lkml/[email protected]/ open here

Signed-off-by Eric W. Biederman <[email protected]>

---
 ipc/ipc_sysctl.c | 54 ++++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

ipc/ipc_sysctl.c +29 ~~-25~~

index ff99d0305a5b..5a58598d48c8 100644
@@ -68,25 +68,6 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 	return ret;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-static int proc_ipc_dointvec_minmax_checkpoint_restore(struct ctl_table *table,
-		int write, void *buffer, size_t *lenp, loff_t *ppos)
-{
-	struct ipc_namespace *ns = current->nsproxy->ipc_ns;
-	struct ctl_table ipc_table;
-
-	if (write && !checkpoint_restore_ns_capable(ns->user_ns))
-		return -EPERM;
-
-	memcpy(&ipc_table, table, sizeof(ipc_table));
-
-	ipc_table.extra1 = SYSCTL_ZERO;
-	ipc_table.extra2 = SYSCTL_INT_MAX;
-
-	return proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
-}
-#endif
-
 int ipc_mni = IPCMNI;
 int ipc_mni_shift = IPCMNI_SHIFT;
 int ipc_min_cycle = RADIX_TREE_MAP_SIZE;

@@ -172,22 +153,28 @@ static struct ctl_table ipc_sysctls[] = {
 		.procname	= "sem_next_id",
 		.data		= &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
-		.mode		= 0666,
-		.proc_handler	= proc_ipc_dointvec_minmax_checkpoint_restore,
+		.mode		= 0444,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
 		.procname	= "msg_next_id",
 		.data		= &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
-		.mode		= 0666,
-		.proc_handler	= proc_ipc_dointvec_minmax_checkpoint_restore,
+		.mode		= 0444,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
 		.procname	= "shm_next_id",
 		.data		= &init_ipc_ns.ids[IPC_SHM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id),
-		.mode		= 0666,
-		.proc_handler	= proc_ipc_dointvec_minmax_checkpoint_restore,
+		.mode		= 0444,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_INT_MAX,
 	},
 #endif
 	{}

@@ -203,8 +190,25 @@ static int set_is_seen(struct ctl_table_set *set)
 	return &current->nsproxy->ipc_ns->ipc_set == set;
 }
 
+static int ipc_permissions(struct ctl_table_header *head, struct ctl_table *table)
+{
+	int mode = table->mode;
+
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	struct ipc_namespace *ns = current->nsproxy->ipc_ns;
+
+	if (((table->data == &ns->ids[IPC_SEM_IDS].next_id) ||
+	     (table->data == &ns->ids[IPC_MSG_IDS].next_id) ||
+	     (table->data == &ns->ids[IPC_SHM_IDS].next_id)) &&
+	    checkpoint_restore_ns_capable(ns->user_ns))
+		mode = 0666;
+#endif
+	return mode;
+}
+
 static struct ctl_table_root set_root = {
 	.lookup = set_lookup,
+	.permissions = ipc_permissions,
 };
 
 bool setup_ipc_sysctls(struct ipc_namespace *ns)

Signature · 1 line

2.33.2

Alexey Gladkov

author

2022-04-22 12:54 UTC #19 lore raw

PATCH 4/4 ipc: Remove extra braces

Fix coding style. In the previous commit, I added braces because, in addition to changing .data, .extra1 also changed. Now this is not needed.

---
 ipc/ipc_sysctl.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

ipc/ipc_sysctl.c +12 ~~-13~~

index 5a58598d48c8..ef313ecfb53a 100644
@@ -222,42 +222,41 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 		int i;
 
 		for (i = 0; i < ARRAY_SIZE(ipc_sysctls); i++) {
-			if (tbl[i].data == &init_ipc_ns.shm_ctlmax) {
+			if (tbl[i].data == &init_ipc_ns.shm_ctlmax)
 				tbl[i].data = &ns->shm_ctlmax;
 
-			} else if (tbl[i].data == &init_ipc_ns.shm_ctlall) {
+			else if (tbl[i].data == &init_ipc_ns.shm_ctlall)
 				tbl[i].data = &ns->shm_ctlall;
 
-			} else if (tbl[i].data == &init_ipc_ns.shm_ctlmni) {
+			else if (tbl[i].data == &init_ipc_ns.shm_ctlmni)
 				tbl[i].data = &ns->shm_ctlmni;
 
-			} else if (tbl[i].data == &init_ipc_ns.shm_rmid_forced) {
+			else if (tbl[i].data == &init_ipc_ns.shm_rmid_forced)
 				tbl[i].data = &ns->shm_rmid_forced;
 
-			} else if (tbl[i].data == &init_ipc_ns.msg_ctlmax) {
+			else if (tbl[i].data == &init_ipc_ns.msg_ctlmax)
 				tbl[i].data = &ns->msg_ctlmax;
 
-			} else if (tbl[i].data == &init_ipc_ns.msg_ctlmni) {
+			else if (tbl[i].data == &init_ipc_ns.msg_ctlmni)
 				tbl[i].data = &ns->msg_ctlmni;
 
-			} else if (tbl[i].data == &init_ipc_ns.msg_ctlmnb) {
+			else if (tbl[i].data == &init_ipc_ns.msg_ctlmnb)
 				tbl[i].data = &ns->msg_ctlmnb;
 
-			} else if (tbl[i].data == &init_ipc_ns.sem_ctls) {
+			else if (tbl[i].data == &init_ipc_ns.sem_ctls)
 				tbl[i].data = &ns->sem_ctls;
 #ifdef CONFIG_CHECKPOINT_RESTORE
-			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id) {
+			else if (tbl[i].data == &init_ipc_ns.ids[IPC_SEM_IDS].next_id)
 				tbl[i].data = &ns->ids[IPC_SEM_IDS].next_id;
 
-			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_MSG_IDS].next_id) {
+			else if (tbl[i].data == &init_ipc_ns.ids[IPC_MSG_IDS].next_id)
 				tbl[i].data = &ns->ids[IPC_MSG_IDS].next_id;
 
-			} else if (tbl[i].data == &init_ipc_ns.ids[IPC_SHM_IDS].next_id) {
+			else if (tbl[i].data == &init_ipc_ns.ids[IPC_SHM_IDS].next_id)
 				tbl[i].data = &ns->ids[IPC_SHM_IDS].next_id;
 #endif
-			} else {
+			else
 				tbl[i].data = NULL;
-			}
 		}
 
 		ns->ipc_sysctls = __register_sysctl_table(&ns->ipc_set, "kernel", tbl);

Signature · 1 line

2.33.2

Linus Torvalds

2022-04-22 20:45 UTC #20 lore raw

Re: [PATCH v1 0/4] ipc: Remove extra1 field abuse to pass ipc namespace

Quoted 4 lines · Alexey Gladkov

Yes, Linus, these changes are not the refactoring you were talking about, but I plan to try to do such a refactoring in the my next patchset.

Heh. Ok, I'm not saying these patches are pretty, and looking up the namespace thing is a bit subtle, but it's certainly prettier than the existing odd "create a new ctl_table entry because of field abuse".

So this certainly looks like an improvement from a quick look.

Linus

Philip Rhoades

2022-05-04 03:48 UTC #21 lore raw

Re: [PATCH v1 0/4] ipc: Remove extra1 field abuse to pass ipc namespace

People,

Quoted 6 lines · Linus Torvalds

Quoted 4 lines · Alexey Gladkov
Yes, Linus, these changes are not the refactoring you were talking about, but I plan to try to do such a refactoring in the my next patchset.

Quoted 7 lines

Heh. Ok, I'm not saying these patches are pretty, and looking up the namespace thing is a bit subtle, but it's certainly prettier than the existing odd "create a new ctl_table entry because of field abuse".
So this certainly looks like an improvement from a quick look.
Linus

Thanks for that little glimpse into what happens with kernel development - interesting!

Phil.

Signature · 6 lines

Philip Rhoades

PO Box 896
Cowra  NSW  2794
Australia
E-mail:  [email protected]

Alexey Gladkov

author

2022-06-01 13:20 UTC #22 lore raw

cover API extension for handling sysctl

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/printk/sysctl.c#n17

Quoted 9 lines · Linus Torvalds

Quoted 4 lines · Alexey Gladkov
Yes, Linus, these changes are not the refactoring you were talking about, but I plan to try to do such a refactoring in the my next patchset.
Heh. Ok, I'm not saying these patches are pretty, and looking up the namespace thing is a bit subtle, but it's certainly prettier than the existing odd "create a new ctl_table entry because of field abuse".

As I promised, here is one of the possible options for how to get rid of dynamic memory allocation.

We can slightly extend the API and thus be able to save data at the time the file is opened. This will not only eliminate the need to allocate memory, but also provide access to file struct and f_cred.

I made an RFC because I'm not sure that I did the permissions check for ipc_sysctl. I also did not change all the places where this API can be applied to make the patch smaller. As in the case of /proc/sys/kernel/printk where CAP_SYS_ADMIN is checked[1] for the current process at the time of write.

I made a patchset on top of:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-next

Because there are my previous changes.

Alexey Gladkov (4):
sysctl: API extension for handling sysctl
sysctl: ipc: Do not use dynamic memory
sysctl: userns: Do not use dynamic memory
sysctl: mqueue: Do not use dynamic memory

 fs/proc/proc_sysctl.c          |  71 ++++++++--
 include/linux/ipc_namespace.h  |  35 -----
 include/linux/sysctl.h         |  20 ++-
 include/linux/user_namespace.h |   6 -
 ipc/ipc_sysctl.c               | 236 +++++++++++++++++----------------
 ipc/mq_sysctl.c                | 138 ++++++++++---------
 ipc/mqueue.c                   |   5 -
 ipc/namespace.c                |  10 --
 kernel/ucount.c                | 116 +++++++---------
 kernel/user_namespace.c        |  10 +-
 10 files changed, 323 insertions(+), 324 deletions(-)

Signature · 1 line

2.33.3

Alexey Gladkov

author

2022-06-01 13:20 UTC #23 lore raw

RFC PATCH 1/4 sysctl: API extension for handling sysctl

This adds additional optional functions for handling open, read, and write operations that can be customized for each sysctl file. It also creates ctl_context that persists from opening to closing the file in the /proc/sys.

The context allows us to store dynamic information at the time the file is opened. This eliminates the need to duplicate ctl_table in order to dynamically change .data, .extra1 or .extra2.

This API extends the existing one and does not require any changes to already existing sysctl handlers.

In reply to Matthew Wilcox

---
 fs/proc/proc_sysctl.c  | 71 +++++++++++++++++++++++++++++++++++-------
 include/linux/sysctl.h | 20 ++++++++++--
 2 files changed, 77 insertions(+), 14 deletions(-)

fs/proc/proc_sysctl.c +59 ~~-12~~

index 7d9cfc730bd4..d3d43e738f01 100644
@@ -560,6 +560,7 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
 	struct inode *inode = file_inode(iocb->ki_filp);
 	struct ctl_table_header *head = grab_header(inode);
 	struct ctl_table *table = PROC_I(inode)->sysctl_entry;
+	struct ctl_fops *fops = table->ctl_fops;
 	size_t count = iov_iter_count(iter);
 	char *kbuf;
 	ssize_t error;

@@ -577,7 +578,7 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
 
 	/* if that can happen at all, it should be -EINVAL, not -EISDIR */
 	error = -EINVAL;
-	if (!table->proc_handler)
+	if (!table->proc_handler && !fops)
 		goto out;
 
 	/* don't even try if the size is too large */

@@ -600,8 +601,20 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
 	if (error)
 		goto out_free_buf;
 
-	/* careful: calling conventions are nasty here */
-	error = table->proc_handler(table, write, kbuf, &count, &iocb->ki_pos);
+	if (fops) {
+		struct ctl_context *ctx = iocb->ki_filp->private_data;
+
+		if (write && fops->write)
+			error = fops->write(ctx, iocb->ki_filp, kbuf, &count, &iocb->ki_pos);
+		else if (!write && fops->read)
+			error = fops->read(ctx, iocb->ki_filp, kbuf, &count, &iocb->ki_pos);
+		else
+			error = -EINVAL;
+	} else {
+		/* careful: calling conventions are nasty here */
+		error = table->proc_handler(table, write, kbuf, &count, &iocb->ki_pos);
+	}
+
 	if (error)
 		goto out_free_buf;

@@ -634,17 +647,50 @@ static int proc_sys_open(struct inode *inode, struct file *filp)
 {
 	struct ctl_table_header *head = grab_header(inode);
 	struct ctl_table *table = PROC_I(inode)->sysctl_entry;
+	struct ctl_context *ctx;
+	int ret = 0;
 
 	/* sysctl was unregistered */
 	if (IS_ERR(head))
 		return PTR_ERR(head);
 
-	if (table->poll)
-		filp->private_data = proc_sys_poll_event(table->poll);
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->table = table;
+	filp->private_data = ctx;
+
+	if (table->ctl_fops && table->ctl_fops->open)
+		ret = table->ctl_fops->open(ctx, inode, filp);
+
+	if (!ret && table->poll)
+		ctx->poll_event = proc_sys_poll_event(table->poll);
 
 	sysctl_head_finish(head);
 
-	return 0;
+	return ret;
+}
+
+static int proc_sys_release(struct inode *inode, struct file *filp)
+{
+	struct ctl_table_header *head = grab_header(inode);
+	struct ctl_table *table = PROC_I(inode)->sysctl_entry;
+	struct ctl_context *ctx = filp->private_data;
+	int ret = 0;
+
+	if (IS_ERR(head))
+		return PTR_ERR(head);
+
+	if (table->ctl_fops && table->ctl_fops->release)
+		ret = table->ctl_fops->release(ctx, inode, filp);
+
+	sysctl_head_finish(head);
+
+	kfree(ctx);
+	filp->private_data =  NULL;
+
+	return ret;
 }
 
 static __poll_t proc_sys_poll(struct file *filp, poll_table *wait)

@@ -653,23 +699,23 @@ static __poll_t proc_sys_poll(struct file *filp, poll_table *wait)
 	struct ctl_table_header *head = grab_header(inode);
 	struct ctl_table *table = PROC_I(inode)->sysctl_entry;
 	__poll_t ret = DEFAULT_POLLMASK;
-	unsigned long event;
+	struct ctl_context *ctx;
 
 	/* sysctl was unregistered */
 	if (IS_ERR(head))
 		return EPOLLERR | EPOLLHUP;
 
-	if (!table->proc_handler)
+	if (!table->proc_handler && !table->ctl_fops)
 		goto out;
 
 	if (!table->poll)
 		goto out;
 
-	event = (unsigned long)filp->private_data;
+	ctx = filp->private_data;
 	poll_wait(filp, &table->poll->wait, wait);
 
-	if (event != atomic_read(&table->poll->event)) {
-		filp->private_data = proc_sys_poll_event(table->poll);
+	if (ctx->poll_event != atomic_read(&table->poll->event)) {
+		ctx->poll_event = proc_sys_poll_event(table->poll);
 		ret = EPOLLIN | EPOLLRDNORM | EPOLLERR | EPOLLPRI;
 	}

@@ -866,6 +912,7 @@ static int proc_sys_getattr(struct user_namespace *mnt_userns,
 
 static const struct file_operations proc_sys_file_operations = {
 	.open		= proc_sys_open,
+	.release	= proc_sys_release,
 	.poll		= proc_sys_poll,
 	.read_iter	= proc_sys_read,
 	.write_iter	= proc_sys_write,

@@ -1153,7 +1200,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
 			else
 				err |= sysctl_check_table_array(path, table);
 		}
-		if (!table->proc_handler)
+		if (!table->proc_handler && !table->ctl_fops)
 			err |= sysctl_err(path, table, "No proc_handler");
 
 		if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode)

include/linux/sysctl.h +18 -2

index 6353d6db69b2..ca5657c9fcb2 100644
@@ -116,9 +116,9 @@ struct ctl_table_poll {
 	wait_queue_head_t wait;
 };
 
-static inline void *proc_sys_poll_event(struct ctl_table_poll *poll)
+static inline unsigned long proc_sys_poll_event(struct ctl_table_poll *poll)
 {
-	return (void *)(unsigned long)atomic_read(&poll->event);
+	return (unsigned long)atomic_read(&poll->event);
 }
 
 #define __CTL_TABLE_POLL_INITIALIZER(name) {				\

@@ -128,6 +128,21 @@ static inline void *proc_sys_poll_event(struct ctl_table_poll *poll)
 #define DEFINE_CTL_TABLE_POLL(name)					\
 	struct ctl_table_poll name = __CTL_TABLE_POLL_INITIALIZER(name)
 
+struct ctl_context {
+	struct ctl_table *table;
+	unsigned long poll_event;
+	void *ctl_data;
+};
+
+struct inode;
+
+struct ctl_fops {
+	int (*open) (struct ctl_context *, struct inode *, struct file *);
+	int (*release) (struct ctl_context *, struct inode *, struct file *);
+	ssize_t (*read) (struct ctl_context *, struct file *, char *, size_t *, loff_t *);
+	ssize_t (*write) (struct ctl_context *, struct file *, char *, size_t *, loff_t *);
+};
+
 /* A sysctl table is an array of struct ctl_table: */
 struct ctl_table {
 	const char *procname;		/* Text ID for /proc/sys, or zero */

@@ -139,6 +154,7 @@ struct ctl_table {
 	struct ctl_table_poll *poll;
 	void *extra1;
 	void *extra2;
+	struct ctl_fops *ctl_fops;
 } __randomize_layout;
 
 struct ctl_node {

Signature · 1 line

2.33.3

Matthew Wilcox

2022-06-01 19:19 UTC #24 lore raw

Re: [RFC PATCH 1/4] sysctl: API extension for handling sysctl

include/linux/sysctl.h static inline void *proc_sys_poll_event(struct ctl_table_poll *poll) · line 144

Quoted 6 lines · Alexey Gladkov

include/linux/sysctl.h +6 -0

+struct ctl_fops {
+	int (*open) (struct ctl_context *, struct inode *, struct file *);
+	int (*release) (struct ctl_context *, struct inode *, struct file *);
+	ssize_t (*read) (struct ctl_context *, struct file *, char *, size_t *, loff_t *);
+	ssize_t (*write) (struct ctl_context *, struct file *, char *, size_t *, loff_t *);
+};

Why not pass the iocb in ->read and ->write? We're still regretting not doing that with file_operations.

Linus Torvalds

2022-06-01 19:23 UTC #25 lore raw

Re: [RFC PATCH 1/4] sysctl: API extension for handling sysctl

Quoted 3 lines · Matthew Wilcox

Why not pass the iocb in ->read and ->write? We're still regretting not doing that with file_operations.

No, all the actual "io" is done by the caller.

There is no way in hell I want the sysctl callbacks to actually possibly do user space accesses etc.

They get a kernel buffer that has already been set up. There is no iocb or iovec left for them.

(That also means that they can take whatever locks they need, including spinlocks, because there's not going to be any random user accesses or complex pipe buffer lookups or whatever).

Linus

Matthew Wilcox

2022-06-01 19:25 UTC #26 lore raw

Re: [RFC PATCH 1/4] sysctl: API extension for handling sysctl

Quoted 5 lines · Linus Torvalds

Quoted 3 lines · Matthew Wilcox
Why not pass the iocb in ->read and ->write? We're still regretting not doing that with file_operations.

Quoted 7 lines

No, all the actual "io" is done by the caller.
There is no way in hell I want the sysctl callbacks to actually possibly do user space accesses etc.
They get a kernel buffer that has already been set up. There is no iocb or iovec left for them.

I wasn't suggesting the iovec. Just the iocb, instead of passing in the ki_filp and the ki_pos.

Quoted 5 lines

(That also means that they can take whatever locks they need, including spinlocks, because there's not going to be any random user accesses or complex pipe buffer lookups or whatever).
Linus

Linus Torvalds

2022-06-01 19:32 UTC #27 lore raw

Re: [RFC PATCH 1/4] sysctl: API extension for handling sysctl

Quoted 3 lines · Matthew Wilcox

I wasn't suggesting the iovec. Just the iocb, instead of passing in the ki_filp and the ki_pos.

I guess that would work, but would it actually help anything?

I think it's a lot more obvious when it just looks mostly like a "read/write()" system call (ok, pread/pwrite since you get the pos, but still).

There is nothing that could possibly be relevant in the kiocb. All those fields are about odd async or atomicity issues (or "report completion" issues) that simply cannot possibly be valid for a sysctl value.

And if they are, that sysctl value is doing something really really really wrong. So we absolutely don't want to encourage that.

This is not a "do IO" interface. This is a "read or write value" interface.

Linus

Alexey Gladkov

author

2022-06-01 19:32 UTC #28 lore raw

Re: [RFC PATCH 1/4] sysctl: API extension for handling sysctl

Quoted 10 lines · Matthew Wilcox

include/linux/sysctl.h static inline void *proc_sys_poll_event(struct ctl_table_poll *poll) · line 144
Quoted 6 lines · Alexey Gladkov
include/linux/sysctl.h +6 -0
+struct ctl_fops {
+	int (*open) (struct ctl_context *, struct inode *, struct file *);
+	int (*release) (struct ctl_context *, struct inode *, struct file *);
+	ssize_t (*read) (struct ctl_context *, struct file *, char *, size_t *, loff_t *);
+	ssize_t (*write) (struct ctl_context *, struct file *, char *, size_t *, loff_t *);
+};
Why not pass the iocb in ->read and ->write? We're still regretting not doing that with file_operations.

Because buf and len can be modified in BPF_CGROUP_RUN_PROG_SYSCTL. We need to pass the result of this hook to read/write callback.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/proc/proc_sysctl.c#n605
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/bpf/cgroup.c#n1441

Signature · 1 line

Rgrds, legion

Alexey Gladkov

author

2022-06-01 13:21 UTC #29 lore raw

RFC PATCH 2/4 sysctl: ipc: Do not use dynamic memory

Dynamic memory allocation is needed to modify .data and specify the per namespace parameter. The new sysctl API is allowed to get rid of the need for such modification.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/bpf/cgroup.c#n1392:

---
 include/linux/ipc_namespace.h |  18 ---
 ipc/ipc_sysctl.c              | 236 +++++++++++++++++-----------------
 ipc/namespace.c               |   4 -
 3 files changed, 121 insertions(+), 137 deletions(-)

include/linux/ipc_namespace.h +0 ~~-18~~

index e3e8c8662b49..51c2c247c447 100644
@@ -191,22 +191,4 @@ static inline bool setup_mq_sysctls(struct ipc_namespace *ns)
 }
 
 #endif /* CONFIG_POSIX_MQUEUE_SYSCTL */
-
-#ifdef CONFIG_SYSVIPC_SYSCTL
-
-bool setup_ipc_sysctls(struct ipc_namespace *ns);
-void retire_ipc_sysctls(struct ipc_namespace *ns);
-
-#else /* CONFIG_SYSVIPC_SYSCTL */
-
-static inline void retire_ipc_sysctls(struct ipc_namespace *ns)
-{
-}
-
-static inline bool setup_ipc_sysctls(struct ipc_namespace *ns)
-{
-	return true;
-}
-
-#endif /* CONFIG_SYSVIPC_SYSCTL */
 #endif

ipc/ipc_sysctl.c +121 ~~-115~~

339 lines not on this page · raw message

ipc/namespace.c +0 -4

index 754f3237194a..f760243ca685 100644
@@ -63,9 +63,6 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns,
 	if (!setup_mq_sysctls(ns))
 		goto fail_put;
 
-	if (!setup_ipc_sysctls(ns))
-		goto fail_put;
-
 	sem_init_ns(ns);
 	msg_init_ns(ns);
 	shm_init_ns(ns);

@@ -133,7 +130,6 @@ static void free_ipc_ns(struct ipc_namespace *ns)
 	shm_exit_ns(ns);
 
 	retire_mq_sysctls(ns);
-	retire_ipc_sysctls(ns);
 
 	dec_ipc_namespaces(ns->ucounts);
 	put_user_ns(ns->user_ns);

Signature · 1 line

2.33.3

Linus Torvalds

2022-06-01 16:45 UTC #30 lore raw

Re: [RFC PATCH 2/4] sysctl: ipc: Do not use dynamic memory

Quoted 4 lines · Alexey Gladkov

Dynamic memory allocation is needed to modify .data and specify the per namespace parameter. The new sysctl API is allowed to get rid of the need for such modification.

Ok, this is looking better. That said, a few comments:

Quoted 15 lines

ipc/ipc_sysctl.c +6 -0

index ef313ecfb53a..833b670c38f3 100644
@@ -68,26 +68,94 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
        return ret;
 }
 
+static inline void *data_from_ns(struct ctl_context *ctx, struct ctl_table *table);
+
+static int ipc_sys_open(struct ctl_context *ctx, struct inode *inode, struct file *file)
+{
+       struct ipc_namespace *ns = current->nsproxy->ipc_ns;
+

ipc/ipc_sysctl.c static int proc_ipc_sem_dointvec(struct ctl_table *table, int write, · line 86

Quoted 10 lines

ipc/ipc_sysctl.c +10 -0

+       // For now, we only allow changes in init_user_ns.
+       if (ns->user_ns != &init_user_ns)
+               return -EPERM;
+
+#ifdef CONFIG_CHECKPOINT_RESTORE
+       int index = (ctx->table - ipc_sysctls);
+
+       switch (index) {
+               case IPC_SYSCTL_SEM_NEXT_ID:
+               case IPC_SYSCTL_MSG_NEXT_ID:

[...]

I don't think you actually even compile-tested this, because you're using these IPC_SYSCTL_SEM_NEXT_ID etc enums before you even declared them later in the same file.

ipc/ipc_sysctl.c static int proc_ipc_sem_dointvec(struct ctl_table *table, int write, · line 103

Quoted 7 lines

ipc/ipc_sysctl.c +7 -0

+static ssize_t ipc_sys_read(struct ctl_context *ctx, struct file *file,
+                    char *buffer, size_t *lenp, loff_t *ppos)
+{
+       struct ctl_table table = *ctx->table;
+       table.data = data_from_ns(ctx, ctx->table);
+       return table.proc_handler(&table, 0, buffer, lenp, ppos);
+}

Can we please fix the names and the types of this new 'ctx' structure?

Yes, yes, I know the old legacy "sysctl table" is horribly named, and uses "ctl_table".

But let's just write it out. It's not some random control table for anything. It's a very odd and specific thing: "sysctl". Let's use the full name.

Also, Please just make that "ctl_data" member in that "ctl_context" struct not just have a real name, but a real type. Make it literally be

    struct ipc_namespace *ipc_ns;

and if we end up with other things wanting other pointers, just add a new one (or make a union if we care about the size of that allocation, which I don't see any reason we'd do when it's literally just like a couple of pointers in size).

There is no reason to have some pseudo-generic "void *ctl_data" that makes it ambiguous and allows for type confusion and isn't self-documenting. I'd rather have a properly typed pointer that is just initialized to NULL and is not always used or needed, but always has a clear case for *what* it would be used for.

Yes, yes, we have f_private etc for things that are really very very generic and have arbitrary users. But 'sysctl' is not that kind of truly generic use.

I wish we didn't have that silly "create a temporary ctl_table entry" either, and I wish it was properly named. But it's not worth the pointless churn to fix old bad interfaces. But the new ones should have better names, and try to avoid those bad old decisions.

But yeah, I think this all is a step in the right direction. And maybe some of those cases and old 'ctl_table' things can be migrated to just using individual read() functions entirely. The whole 'ctl_table' model was broken, and came from the bad old days with an actual 'sysctl()' system call.

Because I think it would be lovely if people would move away from the 'sysctl table' approach entirely for cases where that makes sense, and these guys that already need special handling are very much in that situation.

Linus

Alexey Gladkov

author

2022-06-01 18:25 UTC #31 lore raw

Re: [RFC PATCH 2/4] sysctl: ipc: Do not use dynamic memory

Quoted 17 lines · Linus Torvalds

Quoted 4 lines · Alexey Gladkov
Dynamic memory allocation is needed to modify .data and specify the per namespace parameter. The new sysctl API is allowed to get rid of the need for such modification.
Ok, this is looking better. That said, a few comments:
Quoted 9 lines
ipc/ipc_sysctl.c +0 -0
index ef313ecfb53a..833b670c38f3 100644
@@ -68,26 +68,94 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
        return ret;
 }
 

Quoted 16 lines

Quoted 6 lines

ipc/ipc_sysctl.c +6 -0

+static inline void *data_from_ns(struct ctl_context *ctx, struct ctl_table *table);
+
+static int ipc_sys_open(struct ctl_context *ctx, struct inode *inode, struct file *file)
+{
+       struct ipc_namespace *ns = current->nsproxy->ipc_ns;
+

ipc/ipc_sysctl.c static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,

Quoted 10 lines

ipc/ipc_sysctl.c +10 -0

+       // For now, we only allow changes in init_user_ns.
+       if (ns->user_ns != &init_user_ns)
+               return -EPERM;
+
+#ifdef CONFIG_CHECKPOINT_RESTORE
+       int index = (ctx->table - ipc_sysctls);
+
+       switch (index) {
+               case IPC_SYSCTL_SEM_NEXT_ID:
+               case IPC_SYSCTL_MSG_NEXT_ID

Quoted 5 lines

[...]
I don't think you actually even compile-tested this, because you're using these IPC_SYSCTL_SEM_NEXT_ID etc enums before you even declared them later in the same file.

I did it but without CONFIG_CHECKPOINT_RESTORE.

This is where I'm not sure who can write to ipc sysctls inside ipc_namespace.

Quoted 10 lines

ipc/ipc_sysctl.c static int proc_ipc_sem_dointvec(struct ctl_table *table, int write, · line 103
Quoted 7 lines
ipc/ipc_sysctl.c +7 -0
+static ssize_t ipc_sys_read(struct ctl_context *ctx, struct file *file,
+                    char *buffer, size_t *lenp, loff_t *ppos)
+{
+       struct ctl_table table = *ctx->table;
+       table.data = data_from_ns(ctx, ctx->table);
+       return table.proc_handler(&table, 0, buffer, lenp, ppos);
+}
Can we please fix the names and the types of this new 'ctx' structure?

Quoted 2 lines

Yes, yes, I know the old legacy "sysctl table" is horribly named, and uses "ctl_table".

Sure.

Quoted 21 lines

But let's just write it out. It's not some random control table for anything. It's a very odd and specific thing: "sysctl". Let's use the full name.
Also, Please just make that "ctl_data" member in that "ctl_context" struct not just have a real name, but a real type. Make it literally be
    struct ipc_namespace *ipc_ns;
and if we end up with other things wanting other pointers, just add a new one (or make a union if we care about the size of that allocation, which I don't see any reason we'd do when it's literally just like a couple of pointers in size).
There is no reason to have some pseudo-generic "void *ctl_data" that makes it ambiguous and allows for type confusion and isn't self-documenting. I'd rather have a properly typed pointer that is just initialized to NULL and is not always used or needed, but always has a clear case for *what* it would be used for.

Quoted 3 lines

Yes, yes, we have f_private etc for things that are really very very generic and have arbitrary users. But 'sysctl' is not that kind of truly generic use.

Yep. I made ctl_data in the same way as f_private. My idea is that if someone needs to store more than one pointer, they can put a struct there. But it turned out that at least now, apart from ipc_namespace, nothing is needed.

Quoted 4 lines

I wish we didn't have that silly "create a temporary ctl_table entry" either, and I wish it was properly named. But it's not worth the pointless churn to fix old bad interfaces. But the new ones should have better names, and try to avoid those bad old decisions.

Currently temporary ctl_table is the main strategy for handling sysctl entries.

Perhaps it will be possible to get rid of this if we add another get_data() that would return what is currently placed in .data in ctl_table. I mean make getting .data dynamic.

Quoted 5 lines

But yeah, I think this all is a step in the right direction. And maybe some of those cases and old 'ctl_table' things can be migrated to just using individual read() functions entirely. The whole 'ctl_table' model was broken, and came from the bad old days with an actual 'sysctl()' system call.

I'm not sure how to get rid of ctl_table since net sysctls are heavily dependent on it.

I was wondering if it's possible to get rid of ctl_table but if it's not possible to rewrite everything to some kind of new magic API, then keeping two of them would be a nightmare.

Another problem is that ctl_table is being used by __cgroup_bpf_run_filter_sysctl.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/bpf/cgroup.c#n1392

Quoted 4 lines

Because I think it would be lovely if people would move away from the 'sysctl table' approach entirely for cases where that makes sense, and these guys that already need special handling are very much in that situation.

Since you think that these patches are a step in the right direction, then I will prepare the first version with your comments in mind.

Signature · 1 line

Rgrds, legion

Linus Torvalds

2022-06-01 18:34 UTC #32 lore raw

Re: [RFC PATCH 2/4] sysctl: ipc: Do not use dynamic memory

Quoted 3 lines · Alexey Gladkov

I'm not sure how to get rid of ctl_table since net sysctls are heavily dependent on it.

I don't actually think it's worth getting rid of entirely, because there's just a lot of simple cases where it "JustWorks(tm)" and having just that table entry describe all the semantics is not wrong at all.

The name may suck, but hey, it's not a big deal. Changing it now would be more pain than it's worth.

No, I was more thinking that things that already need more infrastructure than that simple static ctl_table entry might be better off trying to migrate to your new "proper read op" model, and having more of that dynamic behavior in the read op.

The whole "create dynamic ctl_table entries on the fly" model works, but it's kind of ugly.

Anyway, I think all of this is "I think there is more room for cleanup in this area", and maybe we'll never have enough motivation to actually do that.

Your patches seem to fix the extant issue with the ipc namespace, and the truly disgusting parts (although maybe there are other truly disgusting things hiding - I didn't go look for them).

Linus

Alexey Gladkov

author

2022-06-01 19:05 UTC #33 lore raw

Re: [RFC PATCH 2/4] sysctl: ipc: Do not use dynamic memory

Quoted 9 lines · Linus Torvalds

Quoted 3 lines · Alexey Gladkov
I'm not sure how to get rid of ctl_table since net sysctls are heavily dependent on it.
I don't actually think it's worth getting rid of entirely, because there's just a lot of simple cases where it "JustWorks(tm)" and having just that table entry describe all the semantics is not wrong at all.

Quoted 7 lines

The name may suck, but hey, it's not a big deal. Changing it now would be more pain than it's worth.
No, I was more thinking that things that already need more infrastructure than that simple static ctl_table entry might be better off trying to migrate to your new "proper read op" model, and having more of that dynamic behavior in the read op.

This was part of my plan. I wanted to step by step try migrating other sysctls to use open/read/write where it makes sense.

To be honest, it was Eric Biederman who came up with the idea to separate open, read and write. I am very grateful to him.

Quoted 10 lines

The whole "create dynamic ctl_table entries on the fly" model works, but it's kind of ugly.
Anyway, I think all of this is "I think there is more room for cleanup in this area", and maybe we'll never have enough motivation to actually do that.
Your patches seem to fix the extant issue with the ipc namespace, and the truly disgusting parts (although maybe there are other truly disgusting things hiding - I didn't go look for them).

I also hope to try and fix the f_cred issue.

Signature · 1 line

Rgrds, legion

Luis Chamberlain

2022-06-09 18:51 UTC #34 lore raw

Re: [RFC PATCH 2/4] sysctl: ipc: Do not use dynamic memory

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Alexey Gladkov
I'm not sure how to get rid of ctl_table since net sysctls are heavily dependent on it.
Anyway, I think all of this is "I think there is more room for cleanup in this area", and maybe we'll never have enough motivation to actually do that.

That's a good summary of the cleanup with sysctls so I appreciate the nudges in the right directions.

Luis

Alexey Gladkov

author

2022-06-01 13:21 UTC #35 lore raw

RFC PATCH 3/4 sysctl: userns: Do not use dynamic memory

Dynamic memory allocation is needed to modify .data and specify the per namespace parameter. The new sysctl API is allowed to get rid of the need for such modification.

---
 include/linux/user_namespace.h |   6 --
 kernel/ucount.c                | 116 +++++++++++++--------------------
 kernel/user_namespace.c        |  10 +--
 3 files changed, 46 insertions(+), 86 deletions(-)

include/linux/user_namespace.h +0 -6

index 45f09bec02c4..7b134516e5cb 100644
@@ -95,10 +95,6 @@ struct user_namespace {
 	struct key		*persistent_keyring_register;
 #endif
 	struct work_struct	work;
-#ifdef CONFIG_SYSCTL
-	struct ctl_table_set	set;
-	struct ctl_table_header *sysctls;
-#endif
 	struct ucounts		*ucounts;
 	long ucount_max[UCOUNT_COUNTS];
 	long rlimit_max[UCOUNT_RLIMIT_COUNTS];

@@ -116,8 +112,6 @@ struct ucounts {
 extern struct user_namespace init_user_ns;
 extern struct ucounts init_ucounts;
 
-bool setup_userns_sysctls(struct user_namespace *ns);
-void retire_userns_sysctls(struct user_namespace *ns);
 struct ucounts *inc_ucount(struct user_namespace *ns, kuid_t uid, enum ucount_type type);
 void dec_ucount(struct ucounts *ucounts, enum ucount_type type);
 struct ucounts *alloc_ucounts(struct user_namespace *ns, kuid_t uid);

kernel/ucount.c +45 ~~-71~~

159 lines not on this page · raw message

kernel/user_namespace.c +1 -9

index 981bb2d10d83..c0e707bc9a31 100644
@@ -149,17 +149,10 @@ int create_user_ns(struct cred *new)
 	INIT_LIST_HEAD(&ns->keyring_name_list);
 	init_rwsem(&ns->keyring_sem);
 #endif
-	ret = -ENOMEM;
-	if (!setup_userns_sysctls(ns))
-		goto fail_keyring;
 
 	set_cred_user_ns(new, ns);
 	return 0;
-fail_keyring:
-#ifdef CONFIG_PERSISTENT_KEYRINGS
-	key_put(ns->persistent_keyring_register);
-#endif
-	ns_free_inum(&ns->ns);
+
 fail_free:
 	kmem_cache_free(user_ns_cachep, ns);
 fail_dec:

@@ -208,7 +201,6 @@ static void free_user_ns(struct work_struct *work)
 			kfree(ns->projid_map.forward);
 			kfree(ns->projid_map.reverse);
 		}
-		retire_userns_sysctls(ns);
 		key_free_user_ns(ns);
 		ns_free_inum(&ns->ns);
 		kmem_cache_free(user_ns_cachep, ns);

Signature · 1 line

2.33.3

Alexey Gladkov

author

2022-06-01 13:21 UTC #36 lore raw

RFC PATCH 4/4 sysctl: mqueue: Do not use dynamic memory

Dynamic memory allocation is needed to modify .data and specify the per namespace parameter. The new sysctl API is allowed to get rid of the need for such modification.

---
 include/linux/ipc_namespace.h |  17 -----
 ipc/mq_sysctl.c               | 138 +++++++++++++++++++---------------
 ipc/mqueue.c                  |   5 --
 ipc/namespace.c               |   6 --
 4 files changed, 79 insertions(+), 87 deletions(-)

include/linux/ipc_namespace.h +0 ~~-17~~

index 51c2c247c447..d20753093a2c 100644
@@ -174,21 +174,4 @@ static inline void put_ipc_ns(struct ipc_namespace *ns)
 }
 #endif
 
-#ifdef CONFIG_POSIX_MQUEUE_SYSCTL
-
-void retire_mq_sysctls(struct ipc_namespace *ns);
-bool setup_mq_sysctls(struct ipc_namespace *ns);
-
-#else /* CONFIG_POSIX_MQUEUE_SYSCTL */
-
-static inline void retire_mq_sysctls(struct ipc_namespace *ns)
-{
-}
-
-static inline bool setup_mq_sysctls(struct ipc_namespace *ns)
-{
-	return true;
-}
-
-#endif /* CONFIG_POSIX_MQUEUE_SYSCTL */
 #endif

ipc/mq_sysctl.c +79 ~~-59~~

193 lines not on this page · raw message

ipc/mqueue.c +0 -5

index c0f24cc9f619..ffb79a24d70b 100644
@@ -1711,11 +1711,6 @@ static int __init init_mqueue_fs(void)
 	if (mqueue_inode_cachep == NULL)
 		return -ENOMEM;
 
-	if (!setup_mq_sysctls(&init_ipc_ns)) {
-		pr_warn("sysctl registration failed\n");
-		return -ENOMEM;
-	}
-
 	error = register_filesystem(&mqueue_fs_type);
 	if (error)
 		goto out_sysctl;

ipc/namespace.c +0 -6

index f760243ca685..ae83f0f2651b 100644
@@ -59,10 +59,6 @@ static struct ipc_namespace *create_ipc_ns(struct user_namespace *user_ns,
 	if (err)
 		goto fail_put;
 
-	err = -ENOMEM;
-	if (!setup_mq_sysctls(ns))
-		goto fail_put;
-
 	sem_init_ns(ns);
 	msg_init_ns(ns);
 	shm_init_ns(ns);

@@ -129,8 +125,6 @@ static void free_ipc_ns(struct ipc_namespace *ns)
 	msg_exit_ns(ns);
 	shm_exit_ns(ns);
 
-	retire_mq_sysctls(ns);
-
 	dec_ipc_namespaces(ns->ucounts);
 	put_user_ns(ns->user_ns);
 	ns_free_inum(&ns->ns);

Signature · 1 line

2.33.3

Luis Chamberlain

2022-06-09 16:45 UTC #37 lore raw

Re: [RFC PATCH 0/4] API extension for handling sysctl