On Thu, 19 Mar 2020 20:32:11 +0100
Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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
