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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 25 Feb 2020 19:48:57 +0100
Phil Sutter <phil@nwl.cc> wrote:

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

Functionally, it's doable. The downsides I see are:

1. This logic is already implemented by pipapo, so I would duplicate
   it.

   Other than code duplication itself, the worst part is the risk of
   (accidental) mismatch between the two implementations, and the fact
   that if we ever want to change this logic, we'll have to change it in
   two places (taking care of not breaking API, etc.)

2. It's going to be a bit more complicated than interval_overlap(),
   expect perhaps 50 LoCs, plus conditionals to select
   interval_overlap() or something_else_overlap()

3. [very, very debatable] I consider accepting already existing
   entries, without returning -EEXIST, a bug, no matter if NLM_F_EXCL
   was not passed: NLM_F_EXCL should simply mean what RFC 3549 says,
   that is, "Don't replace the config object if it already exists.". By
   leaving that "error clearing" in the API, we maintain this. On the
   other hand, even in the unlikely case we agree it's a bug, "fixing"
   it comes with UAPI breakage risks, too.

So, yes, I would like to avoid that, but if:

a. I can't return anything else than -EEXIST from nft_pipapo_insert()

b. I can't remove that "if (err == -EEXIST) err = 0;" part

then I don't see any other solution than implementing it in userspace.
I hope somebody had better ideas, but if not, I would go ahead and
implement it in userspace.

-- 
Stefano