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

[Bernhard Thaler <bernhard.thaler@wvnet.at>]

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[].