Re: netfilter: nft_ct: add zone id set support

[Geert Uytterhoeven]

Hi Florian,

On Wed, Feb 22, 2017 at 8:02 PM, Linux Kernel Mailing List
<linux-kernel@xxxxxxxxxxxxxxx> wrote:
> Web:        https://git.kernel.org/torvalds/c/edee4f1e92458299505ff007733f676b00c516a1
> Commit:     edee4f1e92458299505ff007733f676b00c516a1
> Parent:     5c178d81b69f08ca3195427a6ea9a46d9af23127
> Refname:    refs/heads/master
> Author:     Florian Westphal <fw@xxxxxxxxx>
> AuthorDate: Fri Feb 3 13:35:50 2017 +0100
> Committer:  Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> CommitDate: Wed Feb 8 14:16:23 2017 +0100
>
>     netfilter: nft_ct: add zone id set support
>
>     zones allow tracking multiple connections sharing identical tuples,
>     this is needed e.g. when tracking distinct vlans with overlapping ip
>     addresses (conntrack is l2 agnostic).
>
>     Thus the zone has to be set before the packet is picked up by the
>     connection tracker.  This is done by means of 'conntrack templates' which
>     are conntrack structures used solely to pass this info from one netfilter
>     hook to the next.
>
>     The iptables CT target instantiates these connection tracking templates
>     once per rule, i.e. the template is fixed/tied to particular zone, can
>     be read-only and therefore be re-used by as many skbs simultaneously as
>     needed.
>
>     We can't follow this model because we want to take the zone id from
>     an sreg at rule eval time so we could e.g. fill in the zone id from
>     the packets vlan id or a e.g. nftables key : value maps.
>
>     To avoid cost of per packet alloc/free of the template, use a percpu
>     template 'scratch' object and use the refcount to detect the (unlikely)
>     case where the template is still attached to another skb (i.e., previous
>     skb was nfqueued ...).
>
>     Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
>     Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>

> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c

> @@ -407,6 +503,7 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
>         unsigned int len;

With gcc-4.1.2 and -Os:

    net/netfilter/nft_ct.c: In function ânft_ct_set_initâ:
    net/netfilter/nft_ct.c:503: warning: âlenâ may be used
uninitialized in this function

>         int err;
>
> +       priv->dir = IP_CT_DIR_MAX;
>         priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
>         switch (priv->key) {
>  #ifdef CONFIG_NF_CONNTRACK_MARK
> @@ -426,10 +523,28 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
>                         return err;
>                 break;
>  #endif
> +#ifdef CONFIG_NF_CONNTRACK_ZONES
> +       case NFT_CT_ZONE:
> +               if (!nft_ct_tmpl_alloc_pcpu())
> +                       return -ENOMEM;
> +               nft_ct_pcpu_template_refcnt++;

Unlike for the other cases of the switch statement, "len" is not initialized
here...

> +               break;
> +#endif
>         default:
>                 return -EOPNOTSUPP;
>         }
>
> +       if (tb[NFTA_CT_DIRECTION]) {
> +               priv->dir = nla_get_u8(tb[NFTA_CT_DIRECTION]);
> +               switch (priv->dir) {
> +               case IP_CT_DIR_ORIGINAL:
> +               case IP_CT_DIR_REPLY:
> +                       break;
> +               default:
> +                       return -EINVAL;
> +               }
> +       }
> +
>         priv->sreg = nft_parse_register(tb[NFTA_CT_SREG]);
>         err = nft_validate_register_load(priv->sreg, len);

... and used here, which may lead to spurious failures of
nft_validate_register_load().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds