Hi Pablo,
On 6/26/19 11:24 AM, Pablo Neira Ayuso wrote:
> 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.
Yes, following the iptables behavior, in nftables we can set something
like this:
table ip x {
chain y {
type filter hook prerouting priority raw; policy accept;
tcp flags syn notrack
}
chain z {
type filter hook input priority filter; policy accept;
ct state { invalid, untracked } synproxy
ct state invalid drop
}
}
In this case, flags == 0 because there is anything set.
>
>> + 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?
>
Yes, sorry I forgot to remove it. Same for the IPv6 bump.
>> + 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.
