: Make some sysctl RO in net namespaces.

PATCH In review Pavel Emelyanov 2 participants 7 messages Feb 19, 2008 – Feb 27, 2008

Subsystems (4)

Networking Network drivers Proc filesystem Proc sysctl

lore

Share & tools

lore

Pavel Emelyanov

author

2008-02-19 11:55 UTC #0 lore raw

cover : Make some sysctl RO in net namespaces.

Hi, David.

Some time ago, when I made the net.core.somaxconn ctl per-namespace, you told that the approach I used to make some ctl tables read-only in namespace was not very good and said to improve it. After looking at other code, I decided, that many ctl variables will have to be read-only in namespace, so we need some generic way to do this.

So, here's the patchset, that allows to create ctl tables, that are read-only in some namespace in general (and in some net namespace in particular). I tried to make it work the way not to consume extra memory at run time.

This patchset is related to net namespaces only, but on the other hand it affects the core sysctl engine. What is your opinion about this set:
should I send these patches (or some of them) to Andrew instead and wait till it appears in mainline (and sequentially in net tree) or will you accept this one in net-2.6.26?

Thanks,
Pavel

Pavel Emelyanov

author

2008-02-19 11:56 UTC #1 lore raw

PATCH 1/5 : Merge equal code in sysctl proc handlers.

The ->read and ->write callbacks act in a very similar way, so merge these paths to reduce the number of places to patch later.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 fs/proc/proc_sysctl.c |   50 ++++++++++--------------------------------------
 1 files changed, 11 insertions(+), 39 deletions(-)

fs/proc/proc_sysctl.c +11 ~~-39~~

index 614c34b..5e31585 100644
@@ -165,8 +165,8 @@ out:
 	return err;
 }
 
-static ssize_t proc_sys_read(struct file *filp, char __user *buf,
-				size_t count, loff_t *ppos)
+static ssize_t proc_sys_call_handler(struct file *filp, void __user *buf,
+		size_t count, loff_t *ppos, int write)
 {
 	struct dentry *dentry = filp->f_dentry;
 	struct ctl_table_header *head;

@@ -190,12 +190,12 @@ static ssize_t proc_sys_read(struct file *filp, char __user *buf,
 	 * and won't be until we finish.
 	 */
 	error = -EPERM;
-	if (sysctl_perm(table, MAY_READ))
+	if (sysctl_perm(table, write ? MAY_WRITE : MAY_READ))
 		goto out;
 
 	/* careful: calling conventions are nasty here */
 	res = count;
-	error = table->proc_handler(table, 0, filp, buf, &res, ppos);
+	error = table->proc_handler(table, write, filp, buf, &res, ppos);
 	if (!error)
 		error = res;
 out:

@@ -204,44 +204,16 @@ out:
 	return error;
 }
 
-static ssize_t proc_sys_write(struct file *filp, const char __user *buf,
+static ssize_t proc_sys_read(struct file *filp, char __user *buf,
 				size_t count, loff_t *ppos)
 {
-	struct dentry *dentry = filp->f_dentry;
-	struct ctl_table_header *head;
-	struct ctl_table *table;
-	ssize_t error;
-	size_t res;
-
-	table = do_proc_sys_lookup(dentry->d_parent, &dentry->d_name, &head);
-	/* Has the sysctl entry disappeared on us? */
-	error = -ENOENT;
-	if (!table)
-		goto out;
-
-	/* Has the sysctl entry been replaced by a directory? */
-	error = -EISDIR;
-	if (!table->proc_handler)
-		goto out;
-
-	/*
-	 * At this point we know that the sysctl was not unregistered
-	 * and won't be until we finish.
-	 */
-	error = -EPERM;
-	if (sysctl_perm(table, MAY_WRITE))
-		goto out;
-
-	/* careful: calling conventions are nasty here */
-	res = count;
-	error = table->proc_handler(table, 1, filp, (char __user *)buf,
-				    &res, ppos);
-	if (!error)
-		error = res;
-out:
-	sysctl_head_finish(head);
+	return proc_sys_call_handler(filp, (void __user *)buf, count, ppos, 0);
+}
 
-	return error;
+static ssize_t proc_sys_write(struct file *filp, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	return proc_sys_call_handler(filp, (void __user *)buf, count, ppos, 1);
 }

Signature · 1 line

1.5.3.4

Pavel Emelyanov

author

2008-02-19 11:58 UTC #2 lore raw

PATCH 2/5 : Clean sysctls from unneeded extern and forward declarations.

In reply to Pavel Emelyanov

The do_sysctl_strategy can be static since it's used in kernel/sysctl.c only.

Besides, move it and parse_table above their callers and drop the forward declarations.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 include/linux/sysctl.h |    5 --
 kernel/sysctl.c        |  144 +++++++++++++++++++++++-------------------------
 2 files changed, 68 insertions(+), 81 deletions(-)

include/linux/sysctl.h +0 -5

index 571f01d..8e50196 100644
@@ -981,11 +981,6 @@ extern int do_sysctl (int __user *name, int nlen,
 		      void __user *oldval, size_t __user *oldlenp,
 		      void __user *newval, size_t newlen);
 
-extern int do_sysctl_strategy (struct ctl_table *table,
-			       int __user *name, int nlen,
-			       void __user *oldval, size_t __user *oldlenp,
-			       void __user *newval, size_t newlen);
-
 extern ctl_handler sysctl_data;
 extern ctl_handler sysctl_string;
 extern ctl_handler sysctl_intvec;

kernel/sysctl.c +68 ~~-76~~

162 lines not on this page · raw message

Signature · 1 line

1.5.3.4

Pavel Emelyanov

author

2008-02-19 12:01 UTC #3 lore raw

PATCH 3/5 : Add the ->permissions callback on the ctl_table_root.

In reply to Pavel Emelyanov

When the table came from some other root, this root may affect the table's permissions, depending on who is working with the table.

The core hunk is at the bottom of this patch. All the rest is just pushing the ctl_table_root argument up to the sysctl_perm function.

This will be mostly (only?) used in the net sysctls.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 fs/proc/proc_sysctl.c  |    4 ++--
 include/linux/sysctl.h |    7 ++++++-
 kernel/sysctl.c        |   25 ++++++++++++++++++-------
 3 files changed, 26 insertions(+), 10 deletions(-)

fs/proc/proc_sysctl.c +2 -2

index 5e31585..5acc001 100644
@@ -190,7 +190,7 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *buf,
 	 * and won't be until we finish.
 	 */
 	error = -EPERM;
-	if (sysctl_perm(table, write ? MAY_WRITE : MAY_READ))
+	if (sysctl_perm(head->root, table, write ? MAY_WRITE : MAY_READ))
 		goto out;
 
 	/* careful: calling conventions are nasty here */

@@ -388,7 +388,7 @@ static int proc_sys_permission(struct inode *inode, int mask, struct nameidata *
 		goto out;
 
 	/* Use the permissions on the sysctl table entry */
-	error = sysctl_perm(table, mask);
+	error = sysctl_perm(head->root, table, mask);
 out:
 	sysctl_head_finish(head);
 	return error;

include/linux/sysctl.h +6 -1

index 8e50196..3239561 100644
@@ -945,11 +945,14 @@ enum
 /* For the /proc/sys support */
 struct ctl_table;
 struct nsproxy;
+struct ctl_table_root;
+
 extern struct ctl_table_header *sysctl_head_next(struct ctl_table_header *prev);
 extern struct ctl_table_header *__sysctl_head_next(struct nsproxy *namespaces,
 						struct ctl_table_header *prev);
 extern void sysctl_head_finish(struct ctl_table_header *prev);
-extern int sysctl_perm(struct ctl_table *table, int op);
+extern int sysctl_perm(struct ctl_table_root *root,
+		struct ctl_table *table, int op);
 
 typedef struct ctl_table ctl_table;

@@ -1049,6 +1052,8 @@ struct ctl_table_root {
 	struct list_head header_list;
 	struct list_head *(*lookup)(struct ctl_table_root *root,
 					   struct nsproxy *namespaces);
+	int (*permissions)(struct ctl_table_root *root,
+			struct nsproxy *namespaces, struct ctl_table *table);
 };
 
 /* struct ctl_table_header is used to maintain dynamic lists of

kernel/sysctl.c +18 -7

index c224cc5..8b8b582 100644
@@ -1454,7 +1454,8 @@ void register_sysctl_root(struct ctl_table_root *root)
 
 #ifdef CONFIG_SYSCTL_SYSCALL
 /* Perform the actual read/write of a sysctl table entry. */
-static int do_sysctl_strategy (struct ctl_table *table,
+static int do_sysctl_strategy (struct ctl_table_root *root,
+			struct ctl_table *table,
 			int __user *name, int nlen,
 			void __user *oldval, size_t __user *oldlenp,
 			void __user *newval, size_t newlen)

@@ -1465,7 +1466,7 @@ static int do_sysctl_strategy (struct ctl_table *table,
 		op |= 004;
 	if (newval) 
 		op |= 002;
-	if (sysctl_perm(table, op))
+	if (sysctl_perm(root, table, op))
 		return -EPERM;
 
 	if (table->strategy) {

@@ -1491,6 +1492,7 @@ static int do_sysctl_strategy (struct ctl_table *table,
 static int parse_table(int __user *name, int nlen,
 		       void __user *oldval, size_t __user *oldlenp,
 		       void __user *newval, size_t newlen,
+		       struct ctl_table_root *root,
 		       struct ctl_table *table)
 {
 	int n;

@@ -1505,14 +1507,14 @@ repeat:
 		if (n == table->ctl_name) {
 			int error;
 			if (table->child) {
-				if (sysctl_perm(table, 001))
+				if (sysctl_perm(root, table, 001))
 					return -EPERM;
 				name++;
 				nlen--;
 				table = table->child;
 				goto repeat;
 			}
-			error = do_sysctl_strategy(table, name, nlen,
+			error = do_sysctl_strategy(root, table, name, nlen,
 						   oldval, oldlenp,
 						   newval, newlen);
 			return error;

@@ -1538,7 +1540,8 @@ int do_sysctl(int __user *name, int nlen, void __user *oldval, size_t __user *ol
 	for (head = sysctl_head_next(NULL); head;
 			head = sysctl_head_next(head)) {
 		error = parse_table(name, nlen, oldval, oldlenp, 
-					newval, newlen, head->ctl_table);
+					newval, newlen,
+					head->root, head->ctl_table);
 		if (error != -ENOTDIR) {
 			sysctl_head_finish(head);
 			break;

@@ -1584,13 +1587,21 @@ static int test_perm(int mode, int op)
 	return -EACCES;
 }
 
-int sysctl_perm(struct ctl_table *table, int op)
+int sysctl_perm(struct ctl_table_root *root, struct ctl_table *table, int op)
 {
 	int error;
+	int mode;
+
 	error = security_sysctl(table, op);
 	if (error)
 		return error;
-	return test_perm(table->mode, op);
+
+	if (root->permissions)
+		mode = root->permissions(root, current->nsproxy, table);
+	else
+		mode = table->mode;
+
+	return test_perm(mode, op);
 }
 
 static void sysctl_set_parent(struct ctl_table *parent, struct ctl_table *table)

Signature · 1 line

1.5.3.4

Pavel Emelyanov

author

2008-02-19 12:02 UTC #4 lore raw

PATCH 4/5 : Create the net sysctl root for RO tables.

In reply to Pavel Emelyanov

This root keeps ctl tables in one global list, but doesn't allow for non-init namespaces to write into tables, stored in it.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 include/net/net_namespace.h |    2 ++
 net/sysctl_net.c            |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

include/net/net_namespace.h +2 -0

index 28738b7..2930ae3 100644
@@ -173,6 +173,8 @@ struct ctl_table;
 struct ctl_table_header;
 extern struct ctl_table_header *register_net_sysctl_table(struct net *net,
 	const struct ctl_path *path, struct ctl_table *table);
+extern struct ctl_table_header *register_init_net_ctl_table(
+		struct ctl_path *path, struct ctl_table *table);
 extern void unregister_net_sysctl_table(struct ctl_table_header *header);
 
 #endif /* __NET_NET_NAMESPACE_H */

net/sysctl_net.c +33 -0

index 665e856..42c99e6 100644
@@ -40,6 +40,30 @@ static struct ctl_table_root net_sysctl_root = {
 	.lookup = net_ctl_header_lookup,
 };
 
+static LIST_HEAD(net_ro_headers);
+
+static struct list_head *net_ctl_ro_header_lookup(struct ctl_table_root *root,
+		struct nsproxy *namespaces)
+{
+	return &net_ro_headers;
+}
+
+static int net_ctl_ro_permissions(struct ctl_table_root *root,
+		struct nsproxy *ns, struct ctl_table *table)
+{
+	int mode;
+
+	mode = table->mode;
+	if (ns->net_ns != &init_net)
+		mode &= ~0222;
+	return mode;
+}
+
+static struct ctl_table_root net_sysctl_ro_root = {
+	.lookup = net_ctl_ro_header_lookup,
+	.permissions = net_ctl_ro_permissions,
+};
+
 static int sysctl_net_init(struct net *net)
 {
 	INIT_LIST_HEAD(&net->sysctl_table_headers);

@@ -64,6 +88,7 @@ static __init int sysctl_init(void)
 	if (ret)
 		goto out;
 	register_sysctl_root(&net_sysctl_root);
+	register_sysctl_root(&net_sysctl_ro_root);
 out:
 	return ret;
 }

@@ -80,6 +105,14 @@ struct ctl_table_header *register_net_sysctl_table(struct net *net,
 }
 EXPORT_SYMBOL_GPL(register_net_sysctl_table);
 
+struct ctl_table_header *register_init_net_ctl_table(struct ctl_path *path,
+		struct ctl_table *table)
+{
+	return __register_sysctl_paths(&net_sysctl_ro_root,
+			&init_nsproxy, path, table);
+}
+EXPORT_SYMBOL_GPL(register_net_ro_ctl_table);
+
 void unregister_net_sysctl_table(struct ctl_table_header *header)
 {
 	return unregister_sysctl_table(header);

Signature · 1 line

1.5.3.4

Pavel Emelyanov

author

2008-02-19 12:05 UTC #5 lore raw

PATCH 5/5 : Move some net.core sysctls to RO root.

In reply to Pavel Emelyanov

There are many tables in net/core/sysctl_net_core.c that are to be read-only. Current implementation duplicates this array for each namespace just to clear the "write" bits in the permissions mask.

Keep the writable tables to per-net ctl root and move the others to the read-only one. This saves some memory in run time and removes the... ugly code, that prepared the tables.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 net/core/sysctl_net_core.c |   35 +++++++++++++++++------------------
 1 files changed, 17 insertions(+), 18 deletions(-)

net/core/sysctl_net_core.c +17 ~~-18~~

index 130338f..4e530ce 100644
@@ -125,14 +125,6 @@ static struct ctl_table net_core_table[] = {
 #endif /* CONFIG_XFRM */
 #endif /* CONFIG_NET */
 	{
-		.ctl_name	= NET_CORE_SOMAXCONN,
-		.procname	= "somaxconn",
-		.data		= &init_net.sysctl_somaxconn,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= &proc_dointvec
-	},
-	{
 		.ctl_name	= NET_CORE_BUDGET,
 		.procname	= "netdev_budget",
 		.data		= &netdev_budget,

@@ -151,6 +143,18 @@ static struct ctl_table net_core_table[] = {
 	{ .ctl_name = 0 }
 };
 
+static struct ctl_table netns_core_table[] = {
+	{
+		.ctl_name	= NET_CORE_SOMAXCONN,
+		.procname	= "somaxconn",
+		.data		= &init_net.sysctl_somaxconn,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec
+	},
+	{ .ctl_name = 0 }
+};
+
 static __net_initdata struct ctl_path net_core_path[] = {
 	{ .procname = "net", .ctl_name = CTL_NET, },
 	{ .procname = "core", .ctl_name = NET_CORE, },

@@ -159,23 +163,17 @@ static __net_initdata struct ctl_path net_core_path[] = {
 
 static __net_init int sysctl_core_net_init(struct net *net)
 {
-	struct ctl_table *tbl, *tmp;
+	struct ctl_table *tbl;
 
 	net->sysctl_somaxconn = SOMAXCONN;
 
-	tbl = net_core_table;
+	tbl = netns_core_table;
 	if (net != &init_net) {
-		tbl = kmemdup(tbl, sizeof(net_core_table), GFP_KERNEL);
+		tbl = kmemdup(tbl, sizeof(netns_core_table), GFP_KERNEL);
 		if (tbl == NULL)
 			goto err_dup;
 
-		for (tmp = tbl; tmp->procname; tmp++) {
-			if (tmp->data >= (void *)&init_net &&
-					tmp->data < (void *)(&init_net + 1))
-				tmp->data += (char *)net - (char *)&init_net;
-			else
-				tmp->mode &= ~0222;
-		}
+		tbl[0].data = &net->sysctl_somaxconn;
 	}
 
 	net->sysctl_core_hdr = register_net_sysctl_table(net,

@@ -209,6 +207,7 @@ static __net_initdata struct pernet_operations sysctl_core_ops = {
 
 static __init int sysctl_core_init(void)
 {
+	register_init_net_ctl_table(net_core_path, net_core_table);
 	return register_pernet_subsys(&sysctl_core_ops);
 }

Signature · 1 line

1.5.3.4

David Miller

maintainer

2008-02-27 07:10 UTC #6 lore raw

In reply to Pavel Emelyanov

From: Pavel Emelyanov <[email protected]>
Date: Tue, 19 Feb 2008 14:54:55 +0300

Quoted 4 lines

So, here's the patchset, that allows to create ctl tables, that are read-only in some namespace in general (and in some net namespace in particular). I tried to make it work the way not to consume extra memory at run time.

These changes look great.

Quoted 5 lines

This patchset is related to net namespaces only, but on the other hand it affects the core sysctl engine. What is your opinion about this set:
should I send these patches (or some of them) to Andrew instead and wait till it appears in mainline (and sequentially in net tree) or will you accept this one in net-2.6.26?

What we should do is get patches 1-3 reviewed on linux-kernel and then into linux-next somehow (perhaps there is some FS tree?).

Once that happens, I can get net-2.6.26 going and put patch 4 and 5 in there.

Thanks!