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

[Phil Sutter <phil@nwl.cc>]

Hi,

Sorry for jumping back into the discussion this late.

On Tue, Feb 25, 2020 at 03:34:35PM +0100, Stefano Brivio wrote:
[...]
> Or also simply with:
> 
> # nft add element t s '{ 20-30 . 40 }'
> # nft add element t s '{ 25-35 . 40 }'
> 
> the second element is silently ignored. I'm returning -EEXIST from
> nft_pipapo_insert(), but nft_add_set_elem() clears it because NLM_F_EXCL
> is not set.
> 
> Are you suggesting that this is consistent and therefore not a problem?
> 
> Or are you proposing that I should handle this in userspace as it's done
> for non-concatenated ranges?

The problem is that user tried to add a new element which is not yet
contained and the 'add element' command is the same as if it was
identical to an existing one. We must not ignore this situation as the
user needs to know: In the above case e.g., element '35 . 40' won't
match after the zero-return from 'add element' command.

At first I assumed we could merge e.g.:

| { 20-30 . 40-50, 25-35 . 45-55 }

into:

| { 20-35 . 40-55 }

But now I realize this is wrong. We would match e.g. '{ 20 . 55 }', a
combination the user never specified.

Given that merging multiple concatenated ranges is a non-trivial task, I
guess the only sane thing to do (for now at least) is to perform overlap
detection in user space and reject the command if an overlap is
detected. Stefano, do you see any problems with that?

Thanks, Phil