Re: [PATCH nf 0/2] nft_set_pipapo: Fix crash due to dangling entries in mapping table

[Phil Sutter <phil@nwl.cc>]

Hi Stefano,

On Fri, Feb 21, 2020 at 11:22:18PM +0100, Stefano Brivio wrote:
> On Fri, 21 Feb 2020 22:17:04 +0100
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > Hi Stefano,
> > 
> > On Fri, Feb 21, 2020 at 03:04:20AM +0100, Stefano Brivio wrote:
> > > Patch 1/2 fixes the issue recently reported by Phil on a sequence of
> > > add/flush/add operations, and patch 2/2 introduces a test case
> > > covering that.  
> > 
> > This fixes my test case, thanks!
> > 
> > I found another problem, but it's maybe on user space side (and not a
> > crash this time ;):
> > 
> > | # nft add table t
> > | # nft add set t s '{ type inet_service . inet_service ; flags interval ; }
> > | # nft add element t s '{ 20-30 . 40, 25-35 . 40 }'
> > | # nft list ruleset
> > | table ip t {
> > | 	set s {
> > | 		type inet_service . inet_service
> > | 		flags interval
> > | 		elements = { 20-30 . 40 }
> > | 	}
> > | }
> > 
> > As you see, the second element disappears. It happens only if ranges
> > overlap and non-range parts are identical.
> >
> > Looking at do_add_setelems(), set_to_intervals() should not be called
> > for concatenated ranges, although I *think* range merging happens only
> > there. So user space should cover for that already?!
> 
> Yes. I didn't consider the need for this kind of specification, given
> that you can obtain the same result by simply adding two elements:
> separate, partially overlapping elements can be inserted (which is, if I
> recall correctly, not the case for rbtree).
> 
> If I recall correctly, we had a short discussion with Florian about
> this, but I don't remember the conclusion.
> 
> However, I see the ugliness, and how this breaks probably legitimate
> expectations. I guess we could call set_to_intervals() in this case,
> that function might need some minor adjustments.
> 
> An alternative, and I'm not sure which one is the most desirable, would
> be to refuse that kind of insertion.

I don't think having concatenated ranges not merge even if possible is a
problem. It's just a "nice feature" with some controversial aspects.

The bug I'm reporting is that Above 'add element' command passes two
elements to nftables but only the first one makes it into the set. If
overlapping elements are fine in pipapo, they should both be there. If
not (or otherwise unwanted), we better error out instead of silently
dropping the second one.

Cheers, Phil