Re: [PATCH] segtree: bail out on concatenations

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 3 Apr 2020 14:50:29 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Fri, Apr 03, 2020 at 02:27:05PM +0200, Stefano Brivio wrote:
> > On Fri, 3 Apr 2020 14:03:51 +0200
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >   
> > > On Thu, Apr 02, 2020 at 11:49:41PM +0200, Pablo Neira Ayuso wrote:  
> > > > This patch adds a lazy check to validate that the first element is not a
> > > > concatenation. The segtree code does not support for concatenations,
> > > > bail out with EOPNOTSUPP.
> > > >
> > > >  # nft add element x y { 10.0.0.0/8 . 192.168.1.3-192.168.1.9 . 1024-65535 }
> > > >  Error: Could not process rule: Operation not supported
> > > >  add element x y { 10.0.0.0/8 . 192.168.1.3-192.168.1.9 . 1024-65535 }
> > > >  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >
> > > > Otherwise, the segtree code barfs with:
> > > >
> > > >  BUG: invalid range expression type concat    
> > > 
> > > Hm.
> > > 
> > > I'm afraid this patch is not enough, the following ruleset crashes
> > > in old kernels with recent nft:
> > > 
> > > flush ruleset
> > > 
> > > table inet filter {
> > >         set test {
> > >                 type ipv4_addr . ipv4_addr . inet_service
> > >                 flags interval,timeout
> > >                 elements = { 1.1.1.1 . 2.2.2.2 . 30 ,
> > >                              2.2.2.2 . 3.3.3.3 . 40 ,
> > >                              3.3.3.3 . 4.4.4.4 . 50 }
> > >         }
> > > 
> > >         chain output {
> > >                 type filter hook output priority 0; policy accept;
> > >                 ip saddr . ip daddr . tcp dport @test counter
> > >         }
> > > }  
> > 
> > First off, sorry, it didn't occur to me to run new tests on older
> > kernels. :/
> > 
> > I can't quickly run that on some older kernel right now. For my
> > understanding, where is it crashing?  
> 
> When listing via
> 
>         nft list ruleset
> 
> The segtree does not know how to handle this concatenation.

Ouch, I see.

> The only way I found to prevent this error is to bail out when adding
> the set, ie. old kernel checks that NFT_SET_CONCAT is not supported,
> hence it bails out.

Right, it took me a while to realise you're referring to the flag that
was present until v2 and I dropped in v3 (gold medal goes to truly
yours -- I actually proposed that :/).

> I'm going to prepare patches to submit this to nf.git.

I'm still wondering if we can come up with a userspace-only fix by
understanding kernel support level somehow (not via flags) and refuse
to add that type of set. I'm still checking.

If we let nft add that type of set with rbtree, it's already a bug, so
avoiding a crash on listing is actually worse than the current
situation.

-- 
Stefano