[PATCH nft 1/2] Revert "segtree: Check ranges when deleting elements"

[PATCH nft 1/2] Revert "segtree: Check ranges when deleting elements"

[Pablo Neira Ayuso]

This partially reverts commit decc12ec2dc3 ("segtree: Check ranges when
deleting elements").

The tests/shell/testcases/sets/0039delete_interval_0 file is left in
place.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/segtree.c                                    | 41 +++++++-----------------
 tests/shell/testcases/sets/0039delete_interval_0 |  0
 2 files changed, 11 insertions(+), 30 deletions(-)
 mode change 100755 => 100644 tests/shell/testcases/sets/0039delete_interval_0

diff --git a/src/segtree.c b/src/segtree.c
index 9f1eecc0ae7e..50e34050c167 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -334,13 +334,6 @@ static unsigned int expr_to_intervals(const struct expr *set,
 	return n;
 }
 
-static bool intervals_match(const struct elementary_interval *e1,
-			    const struct elementary_interval *e2)
-{
-	return mpz_cmp(e1->left, e2->left) == 0 &&
-	       mpz_cmp(e1->right, e2->right) == 0;
-}
-
 /* This function checks for overlaps in two ways:
  *
  * 1) A new interval end intersects an existing interval.
@@ -350,7 +343,8 @@ static bool intervals_match(const struct elementary_interval *e1,
 static bool interval_overlap(const struct elementary_interval *e1,
 			     const struct elementary_interval *e2)
 {
-	if (intervals_match(e1, e2))
+	if (mpz_cmp(e1->left, e2->left) == 0 &&
+	    mpz_cmp(e1->right, e2->right) == 0)
 		return false;
 
 	return (mpz_cmp(e1->left, e2->left) >= 0 &&
@@ -362,7 +356,7 @@ static bool interval_overlap(const struct elementary_interval *e1,
 }
 
 static int set_overlap(struct list_head *msgs, const struct set *set,
-		       struct expr *init, unsigned int keylen, bool add)
+		       struct expr *init, unsigned int keylen)
 {
 	struct elementary_interval *new_intervals[init->size];
 	struct elementary_interval *intervals[set->init->size];
@@ -373,28 +367,15 @@ static int set_overlap(struct list_head *msgs, const struct set *set,
 	m = expr_to_intervals(set->init, keylen, intervals);
 
 	for (i = 0; i < n; i++) {
-		bool found = false;
-
 		for (j = 0; j < m; j++) {
-			if (add && interval_overlap(new_intervals[i],
-						    intervals[j])) {
-				expr_error(msgs, new_intervals[i]->expr,
-					   "interval overlaps with an existing one");
-				errno = EEXIST;
-				ret = -1;
-				goto out;
-			} else if (!add && intervals_match(new_intervals[i],
-							   intervals[j])) {
-				found = true;
-				break;
-			}
-		}
-		if (!add && !found) {
+			if (!interval_overlap(new_intervals[i], intervals[j]))
+				continue;
+
 			expr_error(msgs, new_intervals[i]->expr,
-				   "interval not found in set");
-			errno = ENOENT;
+				   "interval overlaps with an existing one");
+			errno = EEXIST;
 			ret = -1;
-			break;
+			goto out;
 		}
 	}
 out:
@@ -418,8 +399,8 @@ static int set_to_segtree(struct list_head *msgs, struct set *set,
 	/* We are updating an existing set with new elements, check if the new
 	 * interval overlaps with any of the existing ones.
 	 */
-	if (set->init && set->init != init) {
-		err = set_overlap(msgs, set, init, tree->keylen, add);
+	if (add && set->init && set->init != init) {
+		err = set_overlap(msgs, set, init, tree->keylen);
 		if (err < 0)
 			return err;
 	}
diff --git a/tests/shell/testcases/sets/0039delete_interval_0 b/tests/shell/testcases/sets/0039delete_interval_0
old mode 100755
new mode 100644
-- 
2.11.0

[PATCH nft 2/2] segtree: restore automerge

[Pablo Neira Ayuso]

Always close interval in non-anonymous sets unless the auto-merge
feature is set on.

Fixes: a4ec05381261 ("segtree: always close interval in non-anonymous sets")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
Hi Phil,

this patch also supersedes https://patchwork.ozlabs.org/patch/1198896/.

Thanks.

 src/segtree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/segtree.c b/src/segtree.c
index 50e34050c167..b3c61fb088a5 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -497,7 +497,7 @@ static void segtree_linearize(struct list_head *list, const struct set *set,
 			 */
 			mpz_add_ui(p, prev->right, 1);
 			if (mpz_cmp(p, ei->left) < 0 ||
-			    !(set->flags & NFT_SET_ANONYMOUS)) {
+			    (!(set->flags & NFT_SET_ANONYMOUS) && !merge)) {
 				mpz_sub_ui(q, ei->left, 1);
 				nei = ei_alloc(p, q, NULL, EI_F_INTERVAL_END);
 				list_add_tail(&nei->list, list);
-- 
2.11.0

Re: [PATCH nft 2/2] segtree: restore automerge

[Phil Sutter]

Hi Pablo,

On Tue, Nov 26, 2019 at 11:34:22AM +0100, Pablo Neira Ayuso wrote:
> Always close interval in non-anonymous sets unless the auto-merge
> feature is set on.
> 
> Fixes: a4ec05381261 ("segtree: always close interval in non-anonymous sets")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> Hi Phil,
> 
> this patch also supersedes https://patchwork.ozlabs.org/patch/1198896/.

I fear this doesn't fix the problem at hand. With your two patches
applied, tests/shell/testcases/sets/0039delete_interval_0 still fails
for me. Your revert removes executable bit from that script, maybe
that's why you didn't notice?

Cheers, Phil

Re: [PATCH nft 2/2] segtree: restore automerge

[Pablo Neira Ayuso]

On Tue, Nov 26, 2019 at 01:09:08PM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> On Tue, Nov 26, 2019 at 11:34:22AM +0100, Pablo Neira Ayuso wrote:
> > Always close interval in non-anonymous sets unless the auto-merge
> > feature is set on.
> > 
> > Fixes: a4ec05381261 ("segtree: always close interval in non-anonymous sets")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > Hi Phil,
> > 
> > this patch also supersedes https://patchwork.ozlabs.org/patch/1198896/.
> 
> I fear this doesn't fix the problem at hand. With your two patches
> applied, tests/shell/testcases/sets/0039delete_interval_0 still fails
> for me. Your revert removes executable bit from that script, maybe
> that's why you didn't notice?

Indeed. Scratch this.