Re: [RFC PATCH] netfilter: nf_tables: add new write expression

[Patrick McHardy <kaber@trash.net>]

On Sun, Feb 16, 2014 at 11:49:12AM +0100, Nikolay Aleksandrov wrote:
> On 02/16/2014 11:36 AM, Patrick McHardy wrote:
> >>
> >>     netfilter: nf_tables: nft_meta module get/set ops
> >>
> >> That patch is similar to what you propose, but it sets the meta fields
> >> of a packet.
> > 
> > Actually I'd propose two different init functions, that's just not pretty.
> > 
> Hm, okay.
> How about something else, since I wanted to make use of the inlined payload
> fast op, couldn't I just break the dreg/sreg in separate variables and
> based on whether sreg is set act in the fast op (i.e. get/set based on
> that) ? That way we can save some code duplication and keep the ops as
> they're. (That'll work for the slow op as well actually)

I don't agree to adding a set fast op. The get fast op is meant to be
small since its the most common case and is inlined into the main loop.
Anything added there needs a *really* good justification. Modifying
packet data isn't a very common operation and should be kept seperate.

Outside of the main loop, there's no need for a fast op as well since
memcpy *is* fast and any optimized implementation will already do the
same thing you do.

> Also, there's a small problem for payload because the code in the
> select_ops function:
>         if (len <= 4 && IS_ALIGNED(offset, len) && base !=
> NFT_PAYLOAD_LL_HEADER)
>                 return &nft_payload_fast_ops;
>         else
>                 return &nft_payload_ops;
> 
> Has a problem when the offset ends in 101b and length of 3 is used, then
> the fast ops get selected but since that case isn't handled there, we'll
> only load 1 byte from the offset, which is fine for loading since we can
> just switch to 4 bytes and mask out later the unneeded byte when comparing
> for example, but for writing it's a problem since someone might actually
> want to write out 3 bytes. Of course one can always add 2 expressions (1
> byte + 2 byte write) :-)

Good catch, we should make sure the offset is a power of two since the
fast version is only intended for well aligned loads.

Would you care to send a patch?