Re: [PATCH nf-next,v3] netfilter: nft_ct: add ct expectations support

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, May 22, 2019 at 01:39:57PM +0200, Stéphane Veyret wrote:
[...]
> Le mer. 22 mai 2019 à 10:46, Pablo Neira Ayuso <pablo@netfilter.org> a écrit :
> > I think we should set a maximum number of expectations to be created,
> > as a mandatory field, eg.
> >
> >           size 10;
>
> I feel it would be complicated to set, as it would require to keep
> track of all expectations set using this definition, and moreover,
> check if those expectations are still alive, or deleted because
> already used or timed out.

You can use the 'expecting[0]' counter in the ct helper extension to
limit the number of expectations per conntrack entry:

struct nf_conn_help {
[...]
        /* Current number of expected connections */
        u8 expecting[NF_CT_MAX_EXPECT_CLASSES];
};

You have to check if the ct helper area exists in first place.

> > > +     priv->l3num = ctx->family;
> >
> > priv->l3num is only set and never used, remove it. You'll also have to
>
> priv->l3num is used for setting expectation, in function
> nft_ct_expect_obj_eval (see the call to nf_ct_expect_init).

OK, thanks for explaining.

Still this new expectation extension won't work with NFPROTO_INET
tables though, since the expectation infrastructure does not know what
to do with NFPROTO_INET.

If NFPROTO_INET is specified, you could just fetch the l3proto from
the ct object, from the packet path by when you call
nf_ct_expect_init().

> > > +     nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> >
> > I think you don't need nf_ct_helper_ext_add(...);
>
> Actually, I had to add this instruction. While testing the feature, i
> saw that, even if no helper is really set on the connection,
> expectation functions require NF_CT_EXT_HELPER to be set on master
> connection. Without it, there would be some null pointer exception,
> which fortunately is checked at expectation creation by
> __nf_ct_expect_check.

Thanks again for explaining.

You still have to check if the conntrack already has a helper
extensions, otherwise I'm afraid this resets it for this conntrack.