Re: [nf-next PATCH] netfilter: nf_tables: add support for inverted login in nft_lookup

[Arturo Borrero Gonzalez <arturo.borrero.glez@xxxxxxxxx>]

On 31 May 2016 at 18:18, Arturo Borrero Gonzalez
<arturo.borrero.glez@xxxxxxxxx> wrote:
> On 31 May 2016 at 17:50, Arturo Borrero Gonzalez
> <arturo.borrero.glez@xxxxxxxxx> wrote:
>> On 31 May 2016 at 16:44, Florian Westphal <fw@xxxxxxxxx> wrote:
>>> Arturo Borrero Gonzalez <arturo.borrero.glez@xxxxxxxxx> wrote:
>>>> -     if (set->ops->lookup(set, &regs->data[priv->sreg], &ext)) {
>>>> +     if (set->ops->lookup(set, &regs->data[priv->sreg], &ext) ^
>>>> +         priv->invert) {
>>>>               if (set->flags & NFT_SET_MAP)
>>>>                       nft_data_copy(&regs->data[priv->dreg],
>>>>                                     nft_set_ext_data(ext), set->dlen);
>>>
>>> Whats the plan for SET_MAP here?
>>> You enter 'lookup found a result' branch here in case we did not find
>>> anything and invert is set.
>>>
>>> I think its better to use a
>>>
>>> } else if (priv->invert) {
>>>         return;
>>> }
>>>
>
> Not that easy, we should consider 4 cases:
>  * lookup false, invert false -> NFT_BREAK
>  * lookup false, invert true -> return w/o NFT_BREAK
>  * lookup true, invert false -> return w/o NFT_BREAK
>  * lookup true, invert true -> NFT_BREAK
>
> The XOR approach in my original patch seems to over these cases, ie,
> we enter the return branch only if lookup&invert are different.
> I think the nft_data_copy() should not worry us if we prevent the
> combination from happening in _init()
> The only penalty seems the additional boolean check for NFT_SET_MAP
> (which must always fail if priv->invert is true)
>
> So, I'm updating the _init() function with the additional restrictions.
>
> Am I missing something else?

We could be also extra defensive a do something like this:

static void nft_lookup_eval(const struct nft_expr *expr,
                            struct nft_regs *regs,
                            const struct nft_pktinfo *pkt)
{
        const struct nft_lookup *priv = nft_expr_priv(expr);
        const struct nft_set *set = priv->set;
        const struct nft_set_ext *ext;
        bool found;

        found = set->ops->lookup(set, &regs->data[priv->sreg], &ext);

        if (!(found ^ priv->invert)) {
                regs->verdict.code = NFT_BREAK;
                return;
        }

        if (set->flags & NFT_SET_MAP && found && !priv->invert)
                nft_data_copy(&regs->data[priv->dreg],
                              nft_set_ext_data(ext), set->dlen);

}

( web version http://paste.debian.net/713014 )

But, once _init() has been updated to prevent the combination of flags
and dreg/map, I don't know if it worth these extra checks in _eval().

please let me know your thoughts

-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html