Re: netfilter: nft_ct: add zone id set support

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@vger.kernel.org> wrote:
> Web:        https://git.kernel.org/torvalds/c/edee4f1e92458299505ff007733f676b00c516a1
> Commit:     edee4f1e92458299505ff007733f676b00c516a1
> Parent:     5c178d81b69f08ca3195427a6ea9a46d9af23127
> Refname:    refs/heads/master
> Author:     Florian Westphal <fw@strlen.de>
> AuthorDate: Fri Feb 3 13:35:50 2017 +0100
> Committer:  Pablo Neira Ayuso <pablo@netfilter.org>
> 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@strlen.de>
>     Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

> --- 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@linux-m68k.org

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

Re: netfilter: nft_ct: add zone id set support

[Florian Westphal]

Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Feb 22, 2017 at 8:02 PM, Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org> wrote:
> > Web:        https://git.kernel.org/torvalds/c/edee4f1e92458299505ff007733f676b00c516a1
> > Commit:     edee4f1e92458299505ff007733f676b00c516a1
> > Parent:     5c178d81b69f08ca3195427a6ea9a46d9af23127
> > Refname:    refs/heads/master
> > Author:     Florian Westphal <fw@strlen.de>
> > AuthorDate: Fri Feb 3 13:35:50 2017 +0100
> > Committer:  Pablo Neira Ayuso <pablo@netfilter.org>
> > CommitDate: Wed Feb 8 14:16:23 2017 +0100
> >
> Unlike for the other cases of the switch statement, "len" is not initialized
> here...
> 
> > +               break;
> >         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().

Yes, Dan reported this and a patch is queued at
http://patchwork.ozlabs.org/patch/727573/

Pablo, any reason why this is still waiting?
Do you want me to run more tests?

Re: netfilter: nft_ct: add zone id set support

[Pablo Neira Ayuso]

On Thu, Feb 23, 2017 at 12:34:35PM +0100, Florian Westphal wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Feb 22, 2017 at 8:02 PM, Linux Kernel Mailing List
> > <linux-kernel@vger.kernel.org> wrote:
> > > Web:        https://git.kernel.org/torvalds/c/edee4f1e92458299505ff007733f676b00c516a1
> > > Commit:     edee4f1e92458299505ff007733f676b00c516a1
> > > Parent:     5c178d81b69f08ca3195427a6ea9a46d9af23127
> > > Refname:    refs/heads/master
> > > Author:     Florian Westphal <fw@strlen.de>
> > > AuthorDate: Fri Feb 3 13:35:50 2017 +0100
> > > Committer:  Pablo Neira Ayuso <pablo@netfilter.org>
> > > CommitDate: Wed Feb 8 14:16:23 2017 +0100
> > >
> > Unlike for the other cases of the switch statement, "len" is not initialized
> > here...
> > 
> > > +               break;
> > >         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().
> 
> Yes, Dan reported this and a patch is queued at
> http://patchwork.ozlabs.org/patch/727573/
> 
> Pablo, any reason why this is still waiting?

I just flushing out my nf.git tree via pull request.

Once these changes are pulled, I'll fetch recent net-next changes that
were just merged via net. Then, I'll pick this so we can calm down
these compilation warnings.

Are you OK with this procedure? Thanks!

Re: netfilter: nft_ct: add zone id set support

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Feb 23, 2017 at 12:34:35PM +0100, Florian Westphal wrote:
> > Yes, Dan reported this and a patch is queued at
> > http://patchwork.ozlabs.org/patch/727573/
> > 
> > Pablo, any reason why this is still waiting?
> 
> I just flushing out my nf.git tree via pull request.
> 
> Once these changes are pulled, I'll fetch recent net-next changes that
> were just merged via net. Then, I'll pick this so we can calm down
> these compilation warnings.
> 
> Are you OK with this procedure? Thanks!

Sure.