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

[Stefano Brivio <sbrivio@redhat.com>]

Hi Phil,

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.

-- 
Stefano