| Message ID | m1ehrjfg2s.fsf@fess.ebiederm.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
> @@ -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
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
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.
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); } }
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(-)