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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
