Re: [PATCH] segtree: bail out on concatenations

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 3 Apr 2020 15:33:31 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> 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.

Nope, sorry, I can't think of anything better than what you already
proposed.

-- 
Stefano