Re: [PATCH RFC nf-next] netfilter: nf_tables: add math expression support

[Florian Westphal <fw@strlen.de>]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> > Why?  Any particular reason for this?
> > 
> 
> It is for simplicity reasons. Just to avoid a big payload with byteorder 
> conversions per math expression.

Not sold yet. I'd like to hear Pablos opinion.

> > Should it set NFT_BREAK?
> > 
> I don't think so. If the user wants to increase or decrease a field but 
> it already reached the limit IMO it shouldn't do anything but continue 
> with the next expression instead of setting NFT_BREAK. Anyway, not a 
> deal breaker for me..

I was just curious.  No objections from me.

> > I assume this says 'host' because its limited to 8 bits?
> 
> Yes, well it says "host" because I used NFT_MATH_BYTEORDER_HOST to 
> generate it, anyway it doesn't matter as it is 8 bits. Would it be 
> better to hide byteorder if it doesn't matter?

No, I would prefer simpler code, no need to special case this IMO.

> > AFAICS you need to add support for a displacement
> > offset inside the register to support this (or I should write:
> > work-around-align-fetch-mangling ...)

[..]

> What would be the best way to implement this? A pure bit offset 
> (NFT_MATH_OFFSET) or a bitmask (NFT_MATH_BITMASK)?

Bitmask would allow to remove MATH_LEN, correct?

Users that want pure U8 increment can do 0xff000000
resp.  0x000000ff.

Same for U16.  So I'd say a mask would be better.

> > Can you make it NLA_U32?  NLA_U8 doesn't buy anything except
> > limiting us to 255 supported options (i don't see a use case
> > for ever having so many, but if we ever have we don't need new OP32
> > attribute).
> 
> Sure, no problems about that. Also NFTA_MATH_LEN? I do not see a U32 
> bits operation happening in the future tho.

Yes, good question.  Due to netlink padding neither u8 nor u16 saves
space in the message encoding.  I'll leave it up to you, I don't see
2**32 math len making any sense.

> By default I thought that a module made sense.. but it is true it is 
> "general" purpose code and small. I don't really mind.
> 
> > I would also be open to just extending bitwise expression with this
> > inc/dec, both bitwise & math expr do register manipulations.
> 
> While both do register manipulation, I do not think they fit together 
> from a user perspective. Especially if we extend the number of math 
> operations in the future.

Users don't interact with the expressions directly.
But I understand your point.

Pablo, whats your take?