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@xxxxxxxxxxxxx>
> > ---
> > @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?
