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 21:21:43 +0100
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> Hi Stefano,
> 
> On Tue, Feb 25, 2020 at 03:34:35PM +0100, Stefano Brivio wrote:
> [...]
> > This is the problem Phil reported:  
> [...]
> > 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?  
> 
>                         NLM_F_EXCL      !NLM_F_EXCL
>         exact match       EEXIST             0 [*]
>         partial match     EEXIST           EEXIST
> 
> The [*] case would allow for element timeout/expiration updates from
> the control plane for exact matches.

A-ha. I didn't even consider that.

> Note that element updates are not
> supported yet, so this check for !NLM_F_EXCL is a stub. I don't think
> we should allow for updates on partial matches
> 
> I think what it is missing is a error to report "partial match" from
> pipapo. Then, the core translates this "partial match" error to EEXIST
> whether NLM_F_EXCL is set or not.

Yes, given what you explained, I also think it's the case.

> Would this work for you?

It would. I need to write a few more lines in nft_pipapo_insert(),
because right now I don't have a special case for "entirely
overlapping". Something on the lines of:

	dup = pipapo_get(net, set, start, genmask);
	if (PTR_ERR(dup) == -ENOENT) {

-->		compare start and end key for this entry with
		start and end key from 'ext'

Let me know if you want me to post a patch with a placeholder for
whatever you have in mind, or if I can help implementing this, etc.

Thanks!

-- 
Stefano