[RFC PATCH nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[RFC PATCH nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[Bernhard Thaler]

/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(-)

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;
 }
-- 
1.7.10.4

Re: [RFC PATCH nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[Florian Westphal]

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.

Re: [RFC PATCH nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[Bernhard Thaler]

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.

> 

Re: [RFC PATCH nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[Florian Westphal]

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...)

Re: [RFC PATCH nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[Pablo Neira Ayuso]

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.

[PATCHv2 nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[Bernhard Thaler]

230ac490f7fba introduced a dependency to CONFIG_IPV6 which breaks bridging
of IPv6 packets on a bridge with CONFIG_IPV6=n. This is due to the default
value 1 for sysctl entry /proc/sys/net/bridge/bridge-nf-call-ip6tables,
manually setting it to 0 makes IPv6 packets traverse bridge again.

Default /proc/sys/net/bridge/bridge-nf-call-ip6tables to 0 if
CONFIG_IP6_NF_IPTABLES is enabled as CONFIG_IP6_NF_IPTABLES is dependent on
CONFIG_IPV6 as well and is needed for ip6tales to work correclty anyway.

Do not expose sysctl entry /proc/sys/net/bridge/bridge-nf-call-ip6tables
and sysfs entry /sys/class/net/brXXX/bridge/nf_call_ip6tables if
CONFIG_IP6_NF_IPTABLES is not enabled.

Make br_netfilter_ipv6.o dependent on CONFIG_IP6_NF_IPTABLES instead of
CONFIG_IPV6.

Tested with a simple bridge with two interfaces and IPv6 packets trying
to pass from host of left side to host on right side of the bridge.

Fixes: 230ac490f7fba ("netfilter: bridge: split ipv6 code into separated file")

Signed-off-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
---
NOTE:
* checkpatch.pl throws error "ERROR: do not initialise statics to 0 or NULL"
but left for consistency with similar declarations
* dependency to CONFIG_IPV6 instead of CONFIG_IP6_NF_IPTABLES would be more
  "conservative" approach as br_netfilter_ipv6.o was introduced due to
  dependencies in br_validate_ipv6() to CONFIG_IPV6; but CONFIG_IP6_NF_IPTABLES
  will be needed for ip6tables so this dependency may be more "holistic"
* CONFIG_IP6_NF_IPTABLES=n makes br_validate_ipv6() being imported from
  br_netfilter.h; it is used in br_netfilter_hooks.c within br_nf_forward_ip()
  and br_nf_dev_queue_xmit() and returns -1 which will lead to NF_DROP;
  After defaulting /proc/sys/net/bridge/bridge-nf-call-ip6tables to 0 with
  CONFIG_IP6_NF_IPTABLES=n these two functions me never see IPV6 packets and 
  therefore this may not be a problem; I was not able to fully confirm this

Patch history

v2
* do not expose sysfs and sysctl if CONFIG_IP6_NF_IPTABLES=n 
* change dependency to CONFIG_IP6_NF_IPTABLES as suggested by Florian Westphal
* removed changes to br_validate_ipv6() in br_netfilter.h as test show it may
  not be needed

v1
* sysfs and sysctl entry were exposed but not writeable if CONFIG_IPV6=n

 include/net/netfilter/br_netfilter.h |    2 +-
 net/bridge/Makefile                  |    2 +-
 net/bridge/br_netfilter_hooks.c      |   10 ++++++++++
 net/bridge/br_sysfs_br.c             |    4 ++++
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/net/netfilter/br_netfilter.h b/include/net/netfilter/br_netfilter.h
index bab824b..0efbb26 100644
--- a/include/net/netfilter/br_netfilter.h
+++ b/include/net/netfilter/br_netfilter.h
@@ -44,7 +44,7 @@ static inline struct rtable *bridge_parent_rtable(const struct net_device *dev)
 struct net_device *setup_pre_routing(struct sk_buff *skb);
 void br_netfilter_enable(void);
 
-#if IS_ENABLED(CONFIG_IPV6)
+#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
 int br_validate_ipv6(struct sk_buff *skb);
 unsigned int br_nf_pre_routing_ipv6(const struct nf_hook_ops *ops,
 				    struct sk_buff *skb,
diff --git a/net/bridge/Makefile b/net/bridge/Makefile
index a1cda5d..3fd8beb 100644
--- a/net/bridge/Makefile
+++ b/net/bridge/Makefile
@@ -13,7 +13,7 @@ bridge-$(CONFIG_SYSFS) += br_sysfs_if.o br_sysfs_br.o
 bridge-$(subst m,y,$(CONFIG_BRIDGE_NETFILTER)) += br_nf_core.o
 
 br_netfilter-y := br_netfilter_hooks.o
-br_netfilter-$(subst m,y,$(CONFIG_IPV6)) += br_netfilter_ipv6.o
+br_netfilter-$(subst m,y,$(CONFIG_IP6_NF_IPTABLES)) += br_netfilter_ipv6.o
 obj-$(CONFIG_BRIDGE_NETFILTER) += br_netfilter.o
 
 bridge-$(CONFIG_BRIDGE_IGMP_SNOOPING) += br_multicast.o br_mdb.o
diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index c8b9bcf..29993e8 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_IP6_NF_IPTABLES)
 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_IP6_NF_IPTABLES)
 #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
@@ -986,6 +994,7 @@ static struct ctl_table brnf_table[] = {
 		.mode		= 0644,
 		.proc_handler	= brnf_sysctl_call_tables,
 	},
+#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
 	{
 		.procname	= "bridge-nf-call-ip6tables",
 		.data		= &brnf_call_ip6tables,
@@ -993,6 +1002,7 @@ static struct ctl_table brnf_table[] = {
 		.mode		= 0644,
 		.proc_handler	= brnf_sysctl_call_tables,
 	},
+#endif
 	{
 		.procname	= "bridge-nf-filter-vlan-tagged",
 		.data		= &brnf_filter_vlan_tagged,
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 4c97fc5..6113928 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -651,6 +651,7 @@ static ssize_t nf_call_iptables_store(
 }
 static DEVICE_ATTR_RW(nf_call_iptables);
 
+#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
 static ssize_t nf_call_ip6tables_show(
 	struct device *d, struct device_attribute *attr, char *buf)
 {
@@ -671,6 +672,7 @@ static ssize_t nf_call_ip6tables_store(
 	return store_bridge_parm(d, buf, len, set_nf_call_ip6tables);
 }
 static DEVICE_ATTR_RW(nf_call_ip6tables);
+#endif
 
 static ssize_t nf_call_arptables_show(
 	struct device *d, struct device_attribute *attr, char *buf)
@@ -781,7 +783,9 @@ static struct attribute *bridge_attrs[] = {
 #endif
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 	&dev_attr_nf_call_iptables.attr,
+#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
 	&dev_attr_nf_call_ip6tables.attr,
+#endif
 	&dev_attr_nf_call_arptables.attr,
 #endif
 #ifdef CONFIG_BRIDGE_VLAN_FILTERING
-- 
1.7.10.4

Re: [PATCHv2 nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[Pablo Neira Ayuso]

On Thu, Jul 16, 2015 at 02:34:44AM +0200, Bernhard Thaler wrote:
[...]
> * checkpatch.pl throws error "ERROR: do not initialise statics to 0 or NULL"
> but left for consistency with similar declarations

I'd appreciate if you can address this in first place, so we don't have
to undo what we've just done for the ip6tables LOCs.

> * dependency to CONFIG_IPV6 instead of CONFIG_IP6_NF_IPTABLES would be more
>   "conservative" approach as br_netfilter_ipv6.o was introduced due to
>   dependencies in br_validate_ipv6() to CONFIG_IPV6; but CONFIG_IP6_NF_IPTABLES
>   will be needed for ip6tables so this dependency may be more "holistic"

I'm fine if you want to restrict this to ip6tables. This may break
out-of-tree modules since they may be relying on these hooks, but we
don't care about those.

I think this s/CONFIG_IPV6/CONFIG_IP6_NF_IPTABLES should come in a
separated patch, just after the one that removes the checkpatch
errors I'd suggest.

> * CONFIG_IP6_NF_IPTABLES=n makes br_validate_ipv6() being imported from
>   br_netfilter.h; it is used in br_netfilter_hooks.c within br_nf_forward_ip()
>   and br_nf_dev_queue_xmit() and returns -1 which will lead to NF_DROP;
>   After defaulting /proc/sys/net/bridge/bridge-nf-call-ip6tables to 0 with
>   CONFIG_IP6_NF_IPTABLES=n these two functions me never see IPV6 packets and 
>   therefore this may not be a problem; I was not able to fully confirm this

It would be great if you can reduce #ifdef pollution. Now that we have
a br_netfilter_ipv6.c file, I think it should be possible to move the
IPv6 specific code there, at least the /proc code chunk.

Could you have a look into this? Thanks!

Re: [PATCHv2 nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[Bernhard Thaler]

On 16.07.2015 13:17, Pablo Neira Ayuso wrote:
> On Thu, Jul 16, 2015 at 02:34:44AM +0200, Bernhard Thaler wrote:
> [...]
>> * checkpatch.pl throws error "ERROR: do not initialise statics to 0 or NULL"
>> but left for consistency with similar declarations
> 
> I'd appreciate if you can address this in first place, so we don't have
> to undo what we've just done for the ip6tables LOCs.
> 

OK, will do so.

>> * dependency to CONFIG_IPV6 instead of CONFIG_IP6_NF_IPTABLES would be more
>>   "conservative" approach as br_netfilter_ipv6.o was introduced due to
>>   dependencies in br_validate_ipv6() to CONFIG_IPV6; but CONFIG_IP6_NF_IPTABLES
>>   will be needed for ip6tables so this dependency may be more "holistic"
> 
> I'm fine if you want to restrict this to ip6tables. This may break
> out-of-tree modules since they may be relying on these hooks, but we
> don't care about those.
> 

Wasn't aware of that. Don't really want to break functionality for
anybody...but if you are fine with the change as it is I leave it as it is.

> I think this s/CONFIG_IPV6/CONFIG_IP6_NF_IPTABLES should come in a
> separated patch, just after the one that removes the checkpatch
> errors I'd suggest.
> 

OK will do so.

>> * CONFIG_IP6_NF_IPTABLES=n makes br_validate_ipv6() being imported from
>>   br_netfilter.h; it is used in br_netfilter_hooks.c within br_nf_forward_ip()
>>   and br_nf_dev_queue_xmit() and returns -1 which will lead to NF_DROP;
>>   After defaulting /proc/sys/net/bridge/bridge-nf-call-ip6tables to 0 with
>>   CONFIG_IP6_NF_IPTABLES=n these two functions me never see IPV6 packets and 
>>   therefore this may not be a problem; I was not able to fully confirm this
> 
> It would be great if you can reduce #ifdef pollution. Now that we have
> a br_netfilter_ipv6.c file, I think it should be possible to move the
> IPv6 specific code there, at least the /proc code chunk.
> 
> Could you have a look into this? Thanks!
> 

I don't like the #ifdef pollution as well. But I do not find a good
solution to avoid it for brnf_call_ip6tables definition.
I had a look into putting the /proc entry code into br_netfilter_ipv6.c
but I may need some pointers/help how to do it:

- br_netfilter_init() calls register_net_sysctl() with brnf_table[] as
argument to setup the sysctl entries
- brnf_table[] is initialized with static number of elements in
br_netfilter_hooks.c, with the current code I see no way how to set this
single ip6tables array element in br_netfilter_ipv6.c

Currently I see not other solution than using the #ifdef in the
definition of ip6tables sysctl entry in brnf_table[].

Re: [PATCHv2 nf] netfilter: bridge: fix IPv6 packets not being bridged with CONFIG_IPV6=n

[Pablo Neira Ayuso]

On Sat, Jul 18, 2015 at 01:37:26AM +0200, Bernhard Thaler wrote:
> 
> 
> On 16.07.2015 13:17, Pablo Neira Ayuso wrote:
> > On Thu, Jul 16, 2015 at 02:34:44AM +0200, Bernhard Thaler wrote:
> > [...]
> >> * checkpatch.pl throws error "ERROR: do not initialise statics to 0 or NULL"
> >> but left for consistency with similar declarations
> > 
> > I'd appreciate if you can address this in first place, so we don't have
> > to undo what we've just done for the ip6tables LOCs.
> > 
> 
> OK, will do so.
> 
> >> * dependency to CONFIG_IPV6 instead of CONFIG_IP6_NF_IPTABLES would be more
> >>   "conservative" approach as br_netfilter_ipv6.o was introduced due to
> >>   dependencies in br_validate_ipv6() to CONFIG_IPV6; but CONFIG_IP6_NF_IPTABLES
> >>   will be needed for ip6tables so this dependency may be more "holistic"
> > 
> > I'm fine if you want to restrict this to ip6tables. This may break
> > out-of-tree modules since they may be relying on these hooks, but we
> > don't care about those.
> > 
> 
> Wasn't aware of that. Don't really want to break functionality for
> anybody...but if you are fine with the change as it is I leave it as it is.
> 
> > I think this s/CONFIG_IPV6/CONFIG_IP6_NF_IPTABLES should come in a
> > separated patch, just after the one that removes the checkpatch
> > errors I'd suggest.
> > 
> 
> OK will do so.
> 
> >> * CONFIG_IP6_NF_IPTABLES=n makes br_validate_ipv6() being imported from
> >>   br_netfilter.h; it is used in br_netfilter_hooks.c within br_nf_forward_ip()
> >>   and br_nf_dev_queue_xmit() and returns -1 which will lead to NF_DROP;
> >>   After defaulting /proc/sys/net/bridge/bridge-nf-call-ip6tables to 0 with
> >>   CONFIG_IP6_NF_IPTABLES=n these two functions me never see IPV6 packets and 
> >>   therefore this may not be a problem; I was not able to fully confirm this
> > 
> > It would be great if you can reduce #ifdef pollution. Now that we have
> > a br_netfilter_ipv6.c file, I think it should be possible to move the
> > IPv6 specific code there, at least the /proc code chunk.
> > 
> > Could you have a look into this? Thanks!
> > 
> 
> I don't like the #ifdef pollution as well. But I do not find a good
> solution to avoid it for brnf_call_ip6tables definition.
> I had a look into putting the /proc entry code into br_netfilter_ipv6.c
> but I may need some pointers/help how to do it:
> 
> - br_netfilter_init() calls register_net_sysctl() with brnf_table[] as
> argument to setup the sysctl entries
> - brnf_table[] is initialized with static number of elements in
> br_netfilter_hooks.c, with the current code I see no way how to set this
> single ip6tables array element in br_netfilter_ipv6.c
> 
> Currently I see not other solution than using the #ifdef in the
> definition of ip6tables sysctl entry in brnf_table[].

I think you can add a new brnf_table_ipv6[] to the br_netfilter_ipv6.c
file, then pass it to register_net_sysctl using "net/bridge".

Then in br_netfilter_hooks.c, from the init path, you can call
something like:

        br_nf_ipv6_proc_register();

>From the header file, you can define this like:

#ifdef IS_ENABLED(CONFIG_IPV6)
int br_nf_ipv6_proc_register(void);
#else
static inline int br_nf_ipv6_proc_register(void)
{
        return 0;
}
#endif

So it becomes noop if IPv6 is not set.