On Tue, Jun 25, 2019 at 12:37:11PM +0200, Fernando Fernandez Mancera wrote:
[...]
> +static int nft_synproxy_init(const struct nft_ctx *ctx,
> + const struct nft_expr *expr,
> + const struct nlattr * const tb[])
> +{
> + struct synproxy_net *snet = synproxy_pernet(ctx->net);
> + struct nft_synproxy *priv = nft_expr_priv(expr);
> + u32 flags;
> + int err;
> +
> + if (tb[NFTA_SYNPROXY_MSS])
> + priv->info.mss = ntohs(nla_get_be16(tb[NFTA_SYNPROXY_MSS]));
> + if (tb[NFTA_SYNPROXY_WSCALE])
> + priv->info.wscale = nla_get_u8(tb[NFTA_SYNPROXY_WSCALE]);
> + if (tb[NFTA_SYNPROXY_FLAGS]) {
> + flags = ntohl(nla_get_be32(tb[NFTA_SYNPROXY_FLAGS]));
> + if (flags != 0 && (flags & NF_SYNPROXY_OPT_MASK) == 0)
Question: Is flag == 0 valid? If so, no need to return EINVAL in this
case.
> + return -EINVAL;
> + priv->info.options = flags;
> + }
> +
> + err = nf_ct_netns_get(ctx->net, ctx->family);
> + if (err)
> + return err;
> +
> + switch (ctx->family) {
> + case NFPROTO_IPV4:
> + err = nf_synproxy_ipv4_init(snet, ctx->net);
> + if (err)
> + goto nf_ct_failure;
> + snet->hook_ref4++;
nf_synproxy_ipv4_init() internally deals with bumping hook_ref4,
right?
> + 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?
> + 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.
