On Wed, Jun 26, 2019 at 11:47:50AM +0200, Fernando Fernandez Mancera wrote:
> On 6/26/19 11:24 AM, Pablo Neira Ayuso wrote:
[...]
> >> + break;
> >> +#if IS_ENABLED(CONFIG_IPV6)
> >> + case NFPROTO_IPV6:
> >> + err = nf_synproxy_ipv6_init(snet, ctx->net);
> >> + if (err)
> >> + goto nf_ct_failure;
> >> + snet->hook_ref6++;
> >
> > Same here.
> >
> >> + break;
> >
> > #endif /* CONFIG_IPV6 */
> >
> > Note that #endif should finish here, NFPROTO_INET and NFPROTO_BRIDGE
> > should not be wrapper by this.
> >
> > finishes here. You can probably replace this to CONFIG_NF_TABLES_IPV6
> > as above, right?
>
> Yes. In this case we can replace it with CONFIG_NF_TABLES_IPV6.
>
> >
> >> + case NFPROTO_INET:
> >> + case NFPROTO_BRIDGE:
> >> + err = nf_synproxy_ipv4_init(snet, ctx->net);
> >> + if (err)
> >> + goto nf_ct_failure;
> >
> > Missing ifdef.
> >
> >> + err = nf_synproxy_ipv6_init(snet, ctx->net);
> >> + if (err)
> >> + goto nf_ct_failure;
> >> + snet->hook_ref4++;
> >> + snet->hook_ref6++;
> >
> > Bumping refcnt manually?
> >
> >> + break;
> >> +#endif
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +nf_ct_failure:
> >> + nf_ct_netns_put(ctx->net, ctx->family);
> >> + return err;
> >> +}
> >> +
> >> +static void nft_synproxy_destroy(const struct nft_ctx *ctx,
> >> + const struct nft_expr *expr)
> >> +{
> >> + struct synproxy_net *snet = synproxy_pernet(ctx->net);
> >> +
> >> + switch (ctx->family) {
> >> + case NFPROTO_IPV4:
> >> + nf_synproxy_ipv4_fini(snet, ctx->net);
> >> + break;
> >> +#if IS_ENABLED(CONFIG_IPV6)
> >> + case NFPROTO_IPV6:
> >> + nf_synproxy_ipv6_fini(snet, ctx->net);
> >> + break;
> >> + case NFPROTO_INET:
> >> + case NFPROTO_BRIDGE:
> >> + nf_synproxy_ipv4_fini(snet, ctx->net);
> >
> > We should allow bridge to run only with IPv4, if CONFIG_IPV6 is unset.
> >
> > Just wrap this:
> >
> > #ifdef IS_ENABLED(...)
> >
> >> + nf_synproxy_ipv6_fini(snet, ctx->net);
> >
> > #endif
> >
> > Or there's another trick you can do, in the header file, you add:
> >
> > #ifdef IS_ENABLED(...)
> > void nf_synproxy_ipv6_fini(..., ...);
> > #else
> > static inline void nf_synproxy_ipv6_fini(..., ...) {}
> > #endid
> >
> > so we don't need this #ifdef in the code.
> >
>
> If there is no problem to have an inline definition with an empty body
> then this is a good trick to avoid the #ifdef.
This is fine, but use this only from .h file.
Thanks.
