| Message ID | 1436130933-17639-1-git-send-email-bernhard.thaler@wvnet.at |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
Bernhard Thaler <bernhard.thaler@wvnet.at> wrote: > /sys/class/net/brXXX/bridge/nf_call_ip6tables and > /proc/sys/net/bridge/bridge-nf-call-ip6tables can be set to 1 with > CONFIG_IPV6=n. But br_nf_pre_routing_ipv6() is not available and > ip6tables would not be usable as well. > > Do not allow to set both flags to 1 with CONFIG_IPV6=n. > > Change return value of placeholder for br_validate_ipv6() as it is > used in br_nf_forward_ip() even with CONFIG_IPV6=n. > > Fixes: 230ac490f7fba ("netfilter: bridge: split ipv6 code into separated file") > Signed-off-by: Bernhard Thaler <bernhard.thaler@wvnet.at> > --- > static inline unsigned int > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c > index d89f4fa..db0d038 100644 > --- a/net/bridge/br_netfilter_hooks.c > +++ b/net/bridge/br_netfilter_hooks.c > @@ -47,14 +47,22 @@ > #ifdef CONFIG_SYSCTL > static struct ctl_table_header *brnf_sysctl_header; > static int brnf_call_iptables __read_mostly = 1; > +#if IS_ENABLED(CONFIG_IPV6) IS_ENABLED(IP6_NF_IPTABLES) ? > static struct ctl_table brnf_table[] = { > { > .procname = "bridge-nf-call-arptables", > @@ -985,7 +1004,7 @@ static struct ctl_table brnf_table[] = { > .data = &brnf_call_ip6tables, > .maxlen = sizeof(int), > .mode = 0644, > - .proc_handler = brnf_sysctl_call_tables, > + .proc_handler = brnf_sysctl_call_ip6tables, > }, Might also make sense to not create the sysctl and sysfs entry in the first place if no ip6tables is available. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05.07.2015 23:53, Florian Westphal wrote: > Bernhard Thaler <bernhard.thaler@wvnet.at> wrote: >> /sys/class/net/brXXX/bridge/nf_call_ip6tables and >> /proc/sys/net/bridge/bridge-nf-call-ip6tables can be set to 1 with >> CONFIG_IPV6=n. But br_nf_pre_routing_ipv6() is not available and >> ip6tables would not be usable as well. >> >> Do not allow to set both flags to 1 with CONFIG_IPV6=n. >> >> Change return value of placeholder for br_validate_ipv6() as it is >> used in br_nf_forward_ip() even with CONFIG_IPV6=n. >> >> Fixes: 230ac490f7fba ("netfilter: bridge: split ipv6 code into separated file") >> Signed-off-by: Bernhard Thaler <bernhard.thaler@wvnet.at> >> --- >> static inline unsigned int >> diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c >> index d89f4fa..db0d038 100644 >> --- a/net/bridge/br_netfilter_hooks.c >> +++ b/net/bridge/br_netfilter_hooks.c >> @@ -47,14 +47,22 @@ >> #ifdef CONFIG_SYSCTL >> static struct ctl_table_header *brnf_sysctl_header; >> static int brnf_call_iptables __read_mostly = 1; >> +#if IS_ENABLED(CONFIG_IPV6) > > IS_ENABLED(IP6_NF_IPTABLES) ? > br_netfilter_ipv6.o is dependent on CONFIG_IPV6 and contains br_nf_pre_routing_ipv6()...for this reason I sticked with CONFIG_IPV6 to stay consistent. Maybe we should check for both here? >> static struct ctl_table brnf_table[] = { >> { >> .procname = "bridge-nf-call-arptables", >> @@ -985,7 +1004,7 @@ static struct ctl_table brnf_table[] = { >> .data = &brnf_call_ip6tables, >> .maxlen = sizeof(int), >> .mode = 0644, >> - .proc_handler = brnf_sysctl_call_tables, >> + .proc_handler = brnf_sysctl_call_ip6tables, >> }, > > Might also make sense to not create the sysctl and sysfs entry in the > first place if no ip6tables is available. Totally agree, it would be the best solution. My idea was that I do not know how admins and their existing scripts react if sysctl and sysfs entry are gone entirely...and if everybody assumes the default is 0 if these entry do not exist. But scripts that do not check the return code of their write operations on the sysctl and sysfs may not check for the existance of these entries either... A message in dmesg log explaining that ip6tables sysctl and sysfs entries are not exposed due to CONFIG_IPV6=n (and/or IP6_NF_IPTABLES) may be more helpful to understand what is going on. I will update the code to remove the entries instead of blocking the write. > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bernhard Thaler <bernhard.thaler@wvnet.at> wrote: > >> index d89f4fa..db0d038 100644 > >> --- a/net/bridge/br_netfilter_hooks.c > >> +++ b/net/bridge/br_netfilter_hooks.c > >> @@ -47,14 +47,22 @@ > >> #ifdef CONFIG_SYSCTL > >> static struct ctl_table_header *brnf_sysctl_header; > >> static int brnf_call_iptables __read_mostly = 1; > >> +#if IS_ENABLED(CONFIG_IPV6) > > > > IS_ENABLED(IP6_NF_IPTABLES) ? > > > > br_netfilter_ipv6.o is dependent on CONFIG_IPV6 and contains > br_nf_pre_routing_ipv6()...for this reason I sticked with CONFIG_IPV6 to > stay consistent. Maybe we should check for both here? Hmmm, good point. I think br_netfilter_ipv6.o should depend on IP6_NF_IPTABLES (which depends on IPV6) too. The entire point of that thing is to push skbs into ip6tables... > > Might also make sense to not create the sysctl and sysfs entry in the > > first place if no ip6tables is available. > > Totally agree, it would be the best solution. > > My idea was that I do not know how admins and their existing scripts > react if sysctl and sysfs entry are gone entirely...and if everybody > assumes the default is 0 if these entry do not exist. > > But scripts that do not check the return code of their write operations > on the sysctl and sysfs may not check for the existance of these entries > either... Yes, thats the problem, a script checking the errors would break as well. Fortunately its not really important since this only affects custom kernel builds. > A message in dmesg log explaining that ip6tables sysctl and sysfs > entries are not exposed due to CONFIG_IPV6=n (and/or IP6_NF_IPTABLES) > may be more helpful to understand what is going on. Hmm, not sure if there is any point in doing that. We don't do that in other cases either, the assumotion is that if you build your own kernels you better know what you're doing (also, in this case ip6tables doesn't work either which is hopefully the right clue...) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 06, 2015 at 11:41:13PM +0200, Florian Westphal wrote: > Bernhard Thaler <bernhard.thaler@wvnet.at> wrote: [...] > > > Might also make sense to not create the sysctl and sysfs entry in the > > > first place if no ip6tables is available. > > > > Totally agree, it would be the best solution. > > > > My idea was that I do not know how admins and their existing scripts > > react if sysctl and sysfs entry are gone entirely...and if everybody > > assumes the default is 0 if these entry do not exist. > > > > But scripts that do not check the return code of their write operations > > on the sysctl and sysfs may not check for the existance of these entries > > either... > > Yes, thats the problem, a script checking the errors would break as > well. > > Fortunately its not really important since this only affects custom > kernel builds. Right. I think it would be good to have that patch to disable the /proc interface when CONFIG_IPV6 is not built. Would you please send us that patch Bernhard? > > A message in dmesg log explaining that ip6tables sysctl and sysfs > > entries are not exposed due to CONFIG_IPV6=n (and/or IP6_NF_IPTABLES) > > may be more helpful to understand what is going on. > > Hmm, not sure if there is any point in doing that. > We don't do that in other cases either, the assumotion is that if you > build your own kernels you better know what you're doing (also, in this > case ip6tables doesn't work either which is hopefully the right clue...) Agreed. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/netfilter/br_netfilter.h b/include/net/netfilter/br_netfilter.h index bab824b..f2601c1 100644 --- a/include/net/netfilter/br_netfilter.h +++ b/include/net/netfilter/br_netfilter.h @@ -52,7 +52,7 @@ unsigned int br_nf_pre_routing_ipv6(const struct nf_hook_ops *ops, #else static inline int br_validate_ipv6(struct sk_buff *skb) { - return -1; + return 0; } static inline unsigned int diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c index d89f4fa..db0d038 100644 --- a/net/bridge/br_netfilter_hooks.c +++ b/net/bridge/br_netfilter_hooks.c @@ -47,14 +47,22 @@ #ifdef CONFIG_SYSCTL static struct ctl_table_header *brnf_sysctl_header; static int brnf_call_iptables __read_mostly = 1; +#if IS_ENABLED(CONFIG_IPV6) static int brnf_call_ip6tables __read_mostly = 1; +#else +static int brnf_call_ip6tables __read_mostly = 0; +#endif static int brnf_call_arptables __read_mostly = 1; static int brnf_filter_vlan_tagged __read_mostly = 0; static int brnf_filter_pppoe_tagged __read_mostly = 0; static int brnf_pass_vlan_indev __read_mostly = 0; #else #define brnf_call_iptables 1 +#if IS_ENABLED(CONFIG_IPV6) #define brnf_call_ip6tables 1 +#else +#define brnf_call_ip6tables 0 +#endif #define brnf_call_arptables 1 #define brnf_filter_vlan_tagged 0 #define brnf_filter_pppoe_tagged 0 @@ -965,6 +973,17 @@ int brnf_sysctl_call_tables(struct ctl_table *ctl, int write, return ret; } +static +int brnf_sysctl_call_ip6tables(struct ctl_table *ctl, int write, + void __user *buffer, size_t *lenp, loff_t *ppos) +{ + if (!IS_ENABLED(CONFIG_IPV6)) { + if (write) + return -EINVAL; + } + return brnf_sysctl_call_tables(ctl, write, buffer, lenp, ppos); +} + static struct ctl_table brnf_table[] = { { .procname = "bridge-nf-call-arptables", @@ -985,7 +1004,7 @@ static struct ctl_table brnf_table[] = { .data = &brnf_call_ip6tables, .maxlen = sizeof(int), .mode = 0644, - .proc_handler = brnf_sysctl_call_tables, + .proc_handler = brnf_sysctl_call_ip6tables, }, { .procname = "bridge-nf-filter-vlan-tagged", diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c index 4c97fc5..8767477 100644 --- a/net/bridge/br_sysfs_br.c +++ b/net/bridge/br_sysfs_br.c @@ -660,6 +660,9 @@ static ssize_t nf_call_ip6tables_show( static int set_nf_call_ip6tables(struct net_bridge *br, unsigned long val) { + if (!IS_ENABLED(CONFIG_IPV6)) + return -EINVAL; + br->nf_call_ip6tables = val ? true : false; return 0; }
/sys/class/net/brXXX/bridge/nf_call_ip6tables and /proc/sys/net/bridge/bridge-nf-call-ip6tables can be set to 1 with CONFIG_IPV6=n. But br_nf_pre_routing_ipv6() is not available and ip6tables would not be usable as well. Do not allow to set both flags to 1 with CONFIG_IPV6=n. Change return value of placeholder for br_validate_ipv6() as it is used in br_nf_forward_ip() even with CONFIG_IPV6=n. Fixes: 230ac490f7fba ("netfilter: bridge: split ipv6 code into separated file") Signed-off-by: Bernhard Thaler <bernhard.thaler@wvnet.at> --- checkpatch.pl throws error "ERROR: do not initialise statics to 0 or NULL" but left for consistency with similar declarations include/net/netfilter/br_netfilter.h | 2 +- net/bridge/br_netfilter_hooks.c | 21 ++++++++++++++++++++- net/bridge/br_sysfs_br.c | 3 +++ 3 files changed, 24 insertions(+), 2 deletions(-)