Patchwork [net-next,12/19] net neighbour: Convert to use register_net_sysctl

login
register
mail settings
Submitter Eric W. Biederman
Date April 19, 2012, 11:38 p.m.
Message ID <m1ehrjfg2s.fsf@fess.ebiederm.org>
Download mbox | patch
Permalink /patch/153892/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric W. Biederman - April 19, 2012, 11:38 p.m.
Using an ascii path to register_net_sysctl as opposed to the slightly
awkward ctl_path allows for much simpler code.

We no longer need to malloc dev_name to keep it alive the length of our
sysctl register instead we can use a small temporary buffer on the
stack.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 net/core/neighbour.c |   33 ++++++---------------------------
 1 files changed, 6 insertions(+), 27 deletions(-)
Pavel Emelyanov - April 20, 2012, 5:21 a.m.
> @@ -2925,19 +2924,7 @@ int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p,
>  {
>  	struct neigh_sysctl_table *t;
>  	const char *dev_name_source = NULL;
> -
> -#define NEIGH_CTL_PATH_ROOT	0
> -#define NEIGH_CTL_PATH_PROTO	1
> -#define NEIGH_CTL_PATH_NEIGH	2
> -#define NEIGH_CTL_PATH_DEV	3
> -
> -	struct ctl_path neigh_path[] = {
> -		{ .procname = "net",	 },
> -		{ .procname = "proto",	 },
> -		{ .procname = "neigh",	 },
> -		{ .procname = "default", },
> -		{ },
> -	};
> +	char neigh_path[ sizeof("net//neigh/") + IFNAMSIZ + IFNAMSIZ ];

Why two IFNAMSIZ-es? One is for the dev->name, but the other one is not.
Is it just for not having any other better constant at hands?

>  	t = kmemdup(&neigh_sysctl_template, sizeof(*t), GFP_KERNEL);
>  	if (!t)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman - April 20, 2012, 7:25 a.m.
Pavel Emelyanov <xemul@parallels.com> writes:

>> @@ -2925,19 +2924,7 @@ int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p,
>>  {
>>  	struct neigh_sysctl_table *t;
>>  	const char *dev_name_source = NULL;
>> -
>> -#define NEIGH_CTL_PATH_ROOT	0
>> -#define NEIGH_CTL_PATH_PROTO	1
>> -#define NEIGH_CTL_PATH_NEIGH	2
>> -#define NEIGH_CTL_PATH_DEV	3
>> -
>> -	struct ctl_path neigh_path[] = {
>> -		{ .procname = "net",	 },
>> -		{ .procname = "proto",	 },
>> -		{ .procname = "neigh",	 },
>> -		{ .procname = "default", },
>> -		{ },
>> -	};
>> +	char neigh_path[ sizeof("net//neigh/") + IFNAMSIZ + IFNAMSIZ ];
>
> Why two IFNAMSIZ-es? One is for the dev->name, but the other one is not.
> Is it just for not having any other better constant at hands?

Yep.  We don't seem to have any proto name size constants, and all
of decnet ipv4 and ipv6 are all shorter than the 16 bytes of IFNAMSIZ.

Even if I am wrong the snprintf below truncates it's output to the
buffer size and null terminates it so in the worst case we won't cause
a buffer overflow, we will just get a truncated path name to pass
to sysctl.

Shrug I stopped at good enough but I am happy for a better number.

Eric

>>  	t = kmemdup(&neigh_sysctl_template, sizeof(*t), GFP_KERNEL);
>>  	if (!t)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - April 22, 2012, 2:36 a.m.
On Fri, 2012-04-20 at 00:25 -0700, Eric W. Biederman wrote:
> Pavel Emelyanov <xemul@parallels.com> writes:
> 
> >> @@ -2925,19 +2924,7 @@ int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p,
> >>  {
> >>  	struct neigh_sysctl_table *t;
> >>  	const char *dev_name_source = NULL;
> >> -
> >> -#define NEIGH_CTL_PATH_ROOT	0
> >> -#define NEIGH_CTL_PATH_PROTO	1
> >> -#define NEIGH_CTL_PATH_NEIGH	2
> >> -#define NEIGH_CTL_PATH_DEV	3
> >> -
> >> -	struct ctl_path neigh_path[] = {
> >> -		{ .procname = "net",	 },
> >> -		{ .procname = "proto",	 },
> >> -		{ .procname = "neigh",	 },
> >> -		{ .procname = "default", },
> >> -		{ },
> >> -	};
> >> +	char neigh_path[ sizeof("net//neigh/") + IFNAMSIZ + IFNAMSIZ ];
> >
> > Why two IFNAMSIZ-es? One is for the dev->name, but the other one is not.
> > Is it just for not having any other better constant at hands?
> 
> Yep.  We don't seem to have any proto name size constants, and all
> of decnet ipv4 and ipv6 are all shorter than the 16 bytes of IFNAMSIZ.

I don't think it makes any sense to put in IFNAMSIZ as a size for a
string that isn't a device name.

> Even if I am wrong the snprintf below truncates it's output to the
> buffer size and null terminates it so in the worst case we won't cause
> a buffer overflow, we will just get a truncated path name to pass
> to sysctl.
> 
> Shrug I stopped at good enough but I am happy for a better number.

Truncation by snprintf() is definitely better than overflow, but we
should also check and WARN so that if someone breaks this it's hard to
miss.

Ben.

Patch

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 0c2df3d..fadaa81 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2799,7 +2799,6 @@  enum {
 static struct neigh_sysctl_table {
 	struct ctl_table_header *sysctl_header;
 	struct ctl_table neigh_vars[NEIGH_VAR_MAX + 1];
-	char *dev_name;
 } neigh_sysctl_template __read_mostly = {
 	.neigh_vars = {
 		[NEIGH_VAR_MCAST_PROBE] = {
@@ -2925,19 +2924,7 @@  int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p,
 {
 	struct neigh_sysctl_table *t;
 	const char *dev_name_source = NULL;
-
-#define NEIGH_CTL_PATH_ROOT	0
-#define NEIGH_CTL_PATH_PROTO	1
-#define NEIGH_CTL_PATH_NEIGH	2
-#define NEIGH_CTL_PATH_DEV	3
-
-	struct ctl_path neigh_path[] = {
-		{ .procname = "net",	 },
-		{ .procname = "proto",	 },
-		{ .procname = "neigh",	 },
-		{ .procname = "default", },
-		{ },
-	};
+	char neigh_path[ sizeof("net//neigh/") + IFNAMSIZ + IFNAMSIZ ];
 
 	t = kmemdup(&neigh_sysctl_template, sizeof(*t), GFP_KERNEL);
 	if (!t)
@@ -2965,7 +2952,7 @@  int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p,
 		memset(&t->neigh_vars[NEIGH_VAR_GC_INTERVAL], 0,
 		       sizeof(t->neigh_vars[NEIGH_VAR_GC_INTERVAL]));
 	} else {
-		dev_name_source = neigh_path[NEIGH_CTL_PATH_DEV].procname;
+		dev_name_source = "default";
 		t->neigh_vars[NEIGH_VAR_GC_INTERVAL].data = (int *)(p + 1);
 		t->neigh_vars[NEIGH_VAR_GC_THRESH1].data = (int *)(p + 1) + 1;
 		t->neigh_vars[NEIGH_VAR_GC_THRESH2].data = (int *)(p + 1) + 2;
@@ -2988,23 +2975,16 @@  int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p,
 		t->neigh_vars[NEIGH_VAR_BASE_REACHABLE_TIME_MS].extra1 = dev;
 	}
 
-	t->dev_name = kstrdup(dev_name_source, GFP_KERNEL);
-	if (!t->dev_name)
-		goto free;
-
-	neigh_path[NEIGH_CTL_PATH_DEV].procname = t->dev_name;
-	neigh_path[NEIGH_CTL_PATH_PROTO].procname = p_name;
-
+	snprintf(neigh_path, sizeof(neigh_path), "net/%s/neigh/%s",
+		p_name, dev_name_source);
 	t->sysctl_header =
-		register_net_sysctl_table(neigh_parms_net(p), neigh_path, t->neigh_vars);
+		register_net_sysctl(neigh_parms_net(p), neigh_path, t->neigh_vars);
 	if (!t->sysctl_header)
-		goto free_procname;
+		goto free;
 
 	p->sysctl_table = t;
 	return 0;
 
-free_procname:
-	kfree(t->dev_name);
 free:
 	kfree(t);
 err:
@@ -3018,7 +2998,6 @@  void neigh_sysctl_unregister(struct neigh_parms *p)
 		struct neigh_sysctl_table *t = p->sysctl_table;
 		p->sysctl_table = NULL;
 		unregister_net_sysctl_table(t->sysctl_header);
-		kfree(t->dev_name);
 		kfree(t);
 	}
 }