Re: [PATCH] netfilter: nf_tables: restrict expression reduction to first expression

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

On Wed, May 18, 2022 at 12:51:00PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Wed, May 18, 2022 at 12:08:42PM +0200, Pablo Neira Ayuso wrote:
> > Either userspace or kernelspace need to pre-fetch keys inconditionally
> > before comparisons for this to work. Otherwise, register tracking data
> > is misleading and it might result in reducing expressions which are not
> > yet registers.
> > 
> > First expression is guaranteed to be evaluated always, therefore, keep
> > tracking registers and restrict reduction to first expression.
> > 
> > Fixes: b2d306542ff9 ("netfilter: nf_tables: do not reduce read-only expressions")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > @Phil, you mentioned about a way to simplify this patch, I don't see how,
> > just let me know.
> 
> Not a big one. Instead of:
> 
> |	if (nft_expr_reduce(&track, expr)) {
> |		if (reduce) {
> |			reduce = false;
> |			expr = track.cur;
> |			continue;
> |		}
> |	} else if (reduce) {
> |		reduce = false;
> |	}
> 
> One could do:
> 
> |	if (nft_expr_reduce(&track, expr) && reduce) {
> |		reduce = false;
> |		expr = track.cur;
> |		continue;
> |	}
> |	reduce = false;

I'll send v2 using this idiom.

> Regarding later pre-fetching, one should distinguish between expressions
> that (may) set verdict register and those that don't. There are pitfalls
> though, e.g. error conditions handled that way.
> 
> Maybe introduce a new nft_expr_type field and set reduce like so:
> 
> | reduce = reduce && expr->ops->type->reduce;

Could you elaborate?