Re: [PATCH nf 4/4] nft_set_rbtree: Detect partial overlaps on insertion

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 19 Mar 2020 20:32:11 +0100
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> Hi Stefano,
> 
> Sorry for the late response to this one.
> 
> On Thu, Mar 05, 2020 at 09:33:05PM +0100, Stefano Brivio wrote:
> > @@ -223,17 +258,40 @@ static int __nft_rbtree_insert(const struct net *net, const struct nft_set *set,
> >  		d = memcmp(nft_set_ext_key(&rbe->ext),
> >  			   nft_set_ext_key(&new->ext),
> >  			   set->klen);
> > -		if (d < 0)
> > +		if (d < 0) {
> >  			p = &parent->rb_left;
> > -		else if (d > 0)
> > +
> > +			if (nft_rbtree_interval_start(new)) {
> > +				overlap = nft_rbtree_interval_start(rbe) &&
> > +					  nft_set_elem_active(&rbe->ext,
> > +							      genmask);
> > +			} else {
> > +				overlap = nft_rbtree_interval_end(rbe) &&
> > +					  nft_set_elem_active(&rbe->ext,
> > +							      genmask);
> > +			}
> > +		} else if (d > 0) {
> >  			p = &parent->rb_right;
> > -		else {
> > +
> > +			if (nft_rbtree_interval_end(new)) {
> > +				overlap = nft_rbtree_interval_end(rbe) &&
> > +					  nft_set_elem_active(&rbe->ext,
> > +							      genmask);
> > +			} else if (nft_rbtree_interval_end(rbe) &&
> > +				   nft_set_elem_active(&rbe->ext, genmask)) {
> > +				overlap = true;
> > +			}
> > +		} else {
> >  			if (nft_rbtree_interval_end(rbe) &&
> >  			    nft_rbtree_interval_start(new)) {
> >  				p = &parent->rb_left;
> > +  
> 
> Instead of this inconditional reset of 'overlap':
> 
> > +				overlap = false;  
> 
> I think this should be:
> 
>                         if (nft_set_elem_active(&rbe->ext, genmask))
>         			overlap = false;
> 
> if the existing rbe is active, then reset 'overlap' to false.

I think you're right (also for the case just below this), and, if
you're not, then a comment on why I'm not checking it is clearly
needed, because I have a vague memory about the fact we could *perhaps*
skip it in this particular case, and I can't remember that myself :)

I'll fix either problem in v2.

-- 
Stefano