Re: nft typeof keywork patch

Re: nft typeof keywork patch

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Sep 12, 2017 at 07:25:27PM +0200, Florian Westphal wrote:
> > I am looking into adding 'string' type for sets.
> > To do this I'd need to add the typeof command that you mentioned
> > last year:
> > https://marc.info/?l=netfilter-devel&m=145683813130409&w=2

[..]

> Missing part is to add a UDATA_SET_TYPEOF to store the original
> datatype. So, we dumping back the ruleset, we get same listing.

Wait, so
    nft add set ip filter set1 { typeof ip saddr;}

then it should not list
    nft add set ip filter set1 { type ipv4_addr;}

but the exact input using the typeof()?

I wonder how to encode this, especially given we also need to support
concatenation, e.g.

nft add set ip f s { type ipv4_addr .  typeof meta iifname . typeof tcp dport ;}

> See UDATA_SET_KEYBYTEORDER and UDATA_SET_DATABYTEORDER for instance.

They don't need to store a vector.

For the above we'd need to store the exact text input string to rebuild
cmdline.

I think this needs to store something like this in the kernel:
":4-meta iifname:16-tcp dport:2'.

and nft needs to chew through this to learn the typeof and the size of
the datatype.

Alternatively we need infra in nft to obtain the template that
was used to derive the size from the input string ("meta iifname").

I guess the latter should also be doable.  If we do this then we'd
only need to store something like "-meta iifname-tcp dport" to help
with splitting the key into its components.

> > meta mark set typeof(meta mark) ip saddr
> 
> Casting is something we should definitely explore, yes. Question here
> is that we would need to annotate somewhere that the user has
> specified typeof(), or any cast, so we dump back exactly what the user
> is asking for. Probably this needs userdata area for expressions.

Yes, seems like above needs to annotate payload expression (ip saddr)
with the typecast info.  Alternatively however this MIGHT be able to
be detectable on userspace side, e.g.

meta mark set ip saddr

We know that meta mark wants the packet mark, so we could infer
that user forced an override of the type mismatch, and that can
be determined by looking at the expressions' data type.

Re: nft typeof keywork patch

[Pablo Neira Ayuso]

On Wed, Sep 13, 2017 at 05:27:28PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Sep 12, 2017 at 07:25:27PM +0200, Florian Westphal wrote:
> > > I am looking into adding 'string' type for sets.
> > > To do this I'd need to add the typeof command that you mentioned
> > > last year:
> > > https://marc.info/?l=netfilter-devel&m=145683813130409&w=2
> 
> [..]
> 
> > Missing part is to add a UDATA_SET_TYPEOF to store the original
> > datatype. So, we dumping back the ruleset, we get same listing.
> 
> Wait, so
>     nft add set ip filter set1 { typeof ip saddr;}
> 
> then it should not list
>     nft add set ip filter set1 { type ipv4_addr;}
> 
> but the exact input using the typeof()?
> 
> I wonder how to encode this, especially given we also need to support
> concatenation, e.g.
> 
> nft add set ip f s { type ipv4_addr .  typeof meta iifname . typeof tcp dport ;}
> 
> > See UDATA_SET_KEYBYTEORDER and UDATA_SET_DATABYTEORDER for instance.
> 
> They don't need to store a vector.

These UDATA_SET_KEYBYTEORDER and UDATA_SET_DATABYTEORDER should be
split not to store a vector anymore. We should have one
UDATA_SET_KEYBYTEORDERX for each component of the tuple.

> For the above we'd need to store the exact text input string to rebuild
> cmdline.
> 
> I think this needs to store something like this in the kernel:
> ":4-meta iifname:16-tcp dport:2'.
> 
> and nft needs to chew through this to learn the typeof and the size of
> the datatype.
> 
> Alternatively we need infra in nft to obtain the template that
> was used to derive the size from the input string ("meta iifname").
> 
> I guess the latter should also be doable.  If we do this then we'd
> only need to store something like "-meta iifname-tcp dport" to help
> with splitting the key into its components.
> 
> > > meta mark set typeof(meta mark) ip saddr
> > 
> > Casting is something we should definitely explore, yes. Question here
> > is that we would need to annotate somewhere that the user has
> > specified typeof(), or any cast, so we dump back exactly what the user
> > is asking for. Probably this needs userdata area for expressions.
> 
> Yes, seems like above needs to annotate payload expression (ip saddr)
> with the typecast info.  Alternatively however this MIGHT be able to
> be detectable on userspace side, e.g.
> 
> meta mark set ip saddr
> 
> We know that meta mark wants the packet mark, so we could infer
> that user forced an override of the type mismatch, and that can
> be determined by looking at the expressions' data type.

That's another possibility, yes.

I would start with typeof() support for set, then we look at casting.

Re: nft typeof keywork patch

[Florian Westphal]

> > Wait, so
> >     nft add set ip filter set1 { typeof ip saddr;}
> > 
> > then it should not list
> >     nft add set ip filter set1 { type ipv4_addr;}
> > 
> > but the exact input using the typeof()?
> > 
> > I wonder how to encode this, especially given we also need to support
> > concatenation, e.g.

I've started with this, first item i am working on is to change
struct set to pass in struct expr *key instead of datatype+len to make
the original expression (meta iifname for example) available to the
linerize parts so we can stash this info in the udata.

For "typeof(meta iifname) . mark" case the parser will generate a concat
expression, and, since these keep the original expr around we can
iterate over that list to get back the original expressions.

For the normal 'type' case, i think we can simply use a dummy
constant expression to serve as a container for the data type.

What do you think?

Re: nft typeof keywork patch

[Pablo Neira Ayuso]

On Fri, Sep 15, 2017 at 01:02:43PM +0200, Florian Westphal wrote:
> > > Wait, so
> > >     nft add set ip filter set1 { typeof ip saddr;}
> > > 
> > > then it should not list
> > >     nft add set ip filter set1 { type ipv4_addr;}
> > > 
> > > but the exact input using the typeof()?
> > > 
> > > I wonder how to encode this, especially given we also need to support
> > > concatenation, e.g.
> 
> I've started with this, first item i am working on is to change
> struct set to pass in struct expr *key instead of datatype+len to make
> the original expression (meta iifname for example) available to the
> linerize parts so we can stash this info in the udata.
> 
> For "typeof(meta iifname) . mark" case the parser will generate a concat
> expression, and, since these keep the original expr around we can
> iterate over that list to get back the original expressions.

Yes, a list is going to be much cleaner and I would expect it simplies
the code. I remember Carlos Falgueras sent me a patch for this in a
batch he made, however, for whatever reason it never reached upstream.

> For the normal 'type' case, i think we can simply use a dummy
> constant expression to serve as a container for the data type.
> 
> What do you think?

Sounds reasonable, give a shot.