[PATCH nft 1/3] rule: use netlink_add_setelems() when creating literal sets

[PATCH nft 1/3] rule: use netlink_add_setelems() when creating literal sets

[Pablo Neira Ayuso]

Thus, do_add_setelems() is only used for set declarations.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/rule.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/rule.c b/src/rule.c
index b2090dd..a4c685c 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -706,7 +706,7 @@ static int do_add_set(struct netlink_ctx *ctx, const struct handle *h,
 		if (set->flags & SET_F_INTERVAL &&
 		    set_to_intervals(ctx->msgs, set) < 0)
 			return -1;
-		if (do_add_setelems(ctx, &set->handle, set->init) < 0)
+		if (netlink_add_setelems(ctx, &set->handle, set->init) < 0)
 			return -1;
 	}
 	return 0;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

[PATCH nft 2/3] segtree: pass element expression as parameter to set_to_intervals()

[Pablo Neira Ayuso]

So we can reuse this code from set declarations.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/expression.h |    3 ++-
 src/rule.c           |    2 +-
 src/segtree.c        |   15 ++++++++-------
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/expression.h b/include/expression.h
index 010cb95..bc17762 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -362,7 +362,8 @@ extern struct expr *concat_expr_alloc(const struct location *loc);
 extern struct expr *list_expr_alloc(const struct location *loc);
 
 extern struct expr *set_expr_alloc(const struct location *loc);
-extern int set_to_intervals(struct list_head *msgs, struct set *set);
+extern int set_to_intervals(struct list_head *msgs, struct set *set,
+			    struct expr *init);
 
 extern struct expr *mapping_expr_alloc(const struct location *loc,
 				       struct expr *from, struct expr *to);
diff --git a/src/rule.c b/src/rule.c
index a4c685c..495aebc 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -704,7 +704,7 @@ static int do_add_set(struct netlink_ctx *ctx, const struct handle *h,
 		return -1;
 	if (set->init != NULL) {
 		if (set->flags & SET_F_INTERVAL &&
-		    set_to_intervals(ctx->msgs, set) < 0)
+		    set_to_intervals(ctx->msgs, set, set->init) < 0)
 			return -1;
 		if (netlink_add_setelems(ctx, &set->handle, set->init) < 0)
 			return -1;
diff --git a/src/segtree.c b/src/segtree.c
index 060951c..429d35e 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -64,11 +64,12 @@ struct elementary_interval {
 	struct expr			*expr;
 };
 
-static void seg_tree_init(struct seg_tree *tree, const struct set *set)
+static void seg_tree_init(struct seg_tree *tree, const struct set *set,
+			  struct expr *init)
 {
 	struct expr *first;
 
-	first = list_entry(set->init->expressions.next, struct expr, list);
+	first = list_entry(init->expressions.next, struct expr, list);
 	tree->root	= RB_ROOT;
 	tree->keytype	= set->keytype;
 	tree->keylen	= set->keylen;
@@ -431,14 +432,14 @@ static void set_insert_interval(struct expr *set, struct seg_tree *tree,
 	compound_expr_add(set, expr);
 }
 
-int set_to_intervals(struct list_head *errs, struct set *set)
+int set_to_intervals(struct list_head *errs, struct set *set, struct expr *init)
 {
 	struct elementary_interval *ei, *next;
 	struct seg_tree tree;
 	LIST_HEAD(list);
 
-	seg_tree_init(&tree, set);
-	if (set_to_segtree(errs, set->init, &tree) < 0)
+	seg_tree_init(&tree, set, init);
+	if (set_to_segtree(errs, init, &tree) < 0)
 		return -1;
 	segtree_linearize(&list, &tree);
 
@@ -448,12 +449,12 @@ int set_to_intervals(struct list_head *errs, struct set *set)
 				     2 * tree.keylen / BITS_PER_BYTE, ei->left,
 				     2 * tree.keylen / BITS_PER_BYTE, ei->right);
 		}
-		set_insert_interval(set->init, &tree, ei);
+		set_insert_interval(init, &tree, ei);
 		ei_destroy(ei);
 	}
 
 	if (segtree_debug()) {
-		expr_print(set->init);
+		expr_print(init);
 		pr_gmp_debug("\n");
 	}
 	return 0;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

[PATCH nft 3/3] rule: fix use of intervals in set declarations

[Pablo Neira Ayuso]

 # nft add table set
 # nft add set test myset { type ipv4_addr\; flags interval\; }
 # nft add element test myset2 { 1.2.3.0/24 }

Then the listing shows:

 set myset2 {
         type ipv4_addr
         flags interval
         elements = { 1.2.3.0/24}
 }

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=994
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/rule.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index 495aebc..e784d4c 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -690,10 +690,21 @@ static int do_add_chain(struct netlink_ctx *ctx, const struct handle *h,
 }
 
 static int do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
-			   const struct expr *expr)
+			   const struct location *loc, struct expr *expr)
 {
-	if (netlink_add_setelems(ctx, h, expr) < 0)
+	struct set *set;
+
+	if (netlink_get_set(ctx, h, loc) < 0)
 		return -1;
+
+	list_for_each_entry(set, &ctx->list, list) {
+		if (set->flags & SET_F_INTERVAL &&
+		    set_to_intervals(ctx->msgs, set, expr) < 0)
+			return -1;
+
+		if (netlink_add_setelems(ctx, h, expr) < 0)
+			return -1;
+	}
 	return 0;
 }
 
@@ -756,7 +767,8 @@ static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
 	case CMD_OBJ_SET:
 		return do_add_set(ctx, &cmd->handle, cmd->set);
 	case CMD_OBJ_SETELEM:
-		return do_add_setelems(ctx, &cmd->handle, cmd->expr);
+		return do_add_setelems(ctx, &cmd->handle, &cmd->location,
+				       cmd->expr);
 	default:
 		BUG("invalid command object type %u\n", cmd->obj);
 	}
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Patrick McHardy]

>  static int do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
> -			   const struct expr *expr)
> +			   const struct location *loc, struct expr *expr)
>  {
> -	if (netlink_add_setelems(ctx, h, expr) < 0)
> +	struct set *set;
> +
> +	if (netlink_get_set(ctx, h, loc) < 0)

I think we should get it from the internal list and not from the
kernel. We can't add intervals to existing sets so far anyways,
and this would allow it, but it wouldn't work.

>  		return -1;
> +
> +	list_for_each_entry(set, &ctx->list, list) {
> +		if (set->flags & SET_F_INTERVAL &&
> +		    set_to_intervals(ctx->msgs, set, expr) < 0)
> +			return -1;
> +
> +		if (netlink_add_setelems(ctx, h, expr) < 0)
> +			return -1;
> +	}
>  	return 0;
>  }
>  
> @@ -756,7 +767,8 @@ static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
>  	case CMD_OBJ_SET:
>  		return do_add_set(ctx, &cmd->handle, cmd->set);
>  	case CMD_OBJ_SETELEM:
> -		return do_add_setelems(ctx, &cmd->handle, cmd->expr);
> +		return do_add_setelems(ctx, &cmd->handle, &cmd->location,
> +				       cmd->expr);
>  	default:
>  		BUG("invalid command object type %u\n", cmd->obj);
>  	}
> -- 
> 1.7.10.4
> 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Pablo Neira Ayuso]

On Fri, Jun 19, 2015 at 03:13:36PM +0200, Patrick McHardy wrote:
> >  static int do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
> > -			   const struct expr *expr)
> > +			   const struct location *loc, struct expr *expr)
> >  {
> > -	if (netlink_add_setelems(ctx, h, expr) < 0)
> > +	struct set *set;
> > +
> > +	if (netlink_get_set(ctx, h, loc) < 0)
> 
> I think we should get it from the internal list and not from the
> kernel.

There is no such internal list at this moment, we retrieve the list
only for do_command_list(). Are you suggesting to add the code to
retrieve it inconditionally initially?

> We can't add intervals to existing sets so far anyways, and this
> would allow it, but it wouldn't work.

Not sure what you mean with this.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Patrick McHardy]

On 19.06, Pablo Neira Ayuso wrote:
> On Fri, Jun 19, 2015 at 03:13:36PM +0200, Patrick McHardy wrote:
> > >  static int do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
> > > -			   const struct expr *expr)
> > > +			   const struct location *loc, struct expr *expr)
> > >  {
> > > -	if (netlink_add_setelems(ctx, h, expr) < 0)
> > > +	struct set *set;
> > > +
> > > +	if (netlink_get_set(ctx, h, loc) < 0)
> > 
> > I think we should get it from the internal list and not from the
> > kernel.
> 
> There is no such internal list at this moment, we retrieve the list
> only for do_command_list(). Are you suggesting to add the code to
> retrieve it inconditionally initially?

We do have the table->sets list. Yeah, but we don't add it to that
list in the creation part, I see. Actually we should be doing that,
since otherwise we also won't support creating a set and adding
new elements in seperate commands but a single transaction.

> > We can't add intervals to existing sets so far anyways, and this
> > would allow it, but it wouldn't work.
> 
> Not sure what you mean with this.

Intervals need to know the entire set content to be created correctly.
We don't handle incremental updates with intervals correctly ATM.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Pablo Neira Ayuso]

On Fri, Jun 19, 2015 at 03:44:44PM +0200, Patrick McHardy wrote:
> On 19.06, Pablo Neira Ayuso wrote:
> > On Fri, Jun 19, 2015 at 03:13:36PM +0200, Patrick McHardy wrote:
> > > >  static int do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
> > > > -			   const struct expr *expr)
> > > > +			   const struct location *loc, struct expr *expr)
> > > >  {
> > > > -	if (netlink_add_setelems(ctx, h, expr) < 0)
> > > > +	struct set *set;
> > > > +
> > > > +	if (netlink_get_set(ctx, h, loc) < 0)
> > > 
> > > I think we should get it from the internal list and not from the
> > > kernel.
> > 
> > There is no such internal list at this moment, we retrieve the list
> > only for do_command_list(). Are you suggesting to add the code to
> > retrieve it inconditionally initially?
> 
> We do have the table->sets list. Yeah, but we don't add it to that
> list in the creation part, I see. Actually we should be doing that,
> since otherwise we also won't support creating a set and adding
> new elements in seperate commands but a single transaction.

Right.

> > > We can't add intervals to existing sets so far anyways, and this
> > > would allow it, but it wouldn't work.
> > 
> > Not sure what you mean with this.
> 
> Intervals need to know the entire set content to be created correctly.
> We don't handle incremental updates with intervals correctly ATM.

What's the problem?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Patrick McHardy]

On 19.06, Pablo Neira Ayuso wrote:
> On Fri, Jun 19, 2015 at 03:44:44PM +0200, Patrick McHardy wrote:
> > On 19.06, Pablo Neira Ayuso wrote:
> > > On Fri, Jun 19, 2015 at 03:13:36PM +0200, Patrick McHardy wrote:
> > > > >  static int do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
> > > > > -			   const struct expr *expr)
> > > > > +			   const struct location *loc, struct expr *expr)
> > > > >  {
> > > > > -	if (netlink_add_setelems(ctx, h, expr) < 0)
> > > > > +	struct set *set;
> > > > > +
> > > > > +	if (netlink_get_set(ctx, h, loc) < 0)
> > > > 
> > > > I think we should get it from the internal list and not from the
> > > > kernel.
> > > 
> > > There is no such internal list at this moment, we retrieve the list
> > > only for do_command_list(). Are you suggesting to add the code to
> > > retrieve it inconditionally initially?
> > 
> > We do have the table->sets list. Yeah, but we don't add it to that
> > list in the creation part, I see. Actually we should be doing that,
> > since otherwise we also won't support creating a set and adding
> > new elements in seperate commands but a single transaction.
> 
> Right.
> 
> > > > We can't add intervals to existing sets so far anyways, and this
> > > > would allow it, but it wouldn't work.
> > > 
> > > Not sure what you mean with this.
> > 
> > Intervals need to know the entire set content to be created correctly.
> > We don't handle incremental updates with intervals correctly ATM.
> 
> What's the problem?

Consider a set with an interval 0-10. We add an interval 9-11. This
effectively means 0-10 needs to be transformed to 0-8 and we don't
do that.

The root cause is that the kernel must not have overlapping intervals.
They need to be transformed before adding them. This is what segtree
does.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Pablo Neira Ayuso]

On Fri, Jun 19, 2015 at 03:59:48PM +0200, Patrick McHardy wrote:
> On 19.06, Pablo Neira Ayuso wrote:
> > On Fri, Jun 19, 2015 at 03:44:44PM +0200, Patrick McHardy wrote:
> > > On 19.06, Pablo Neira Ayuso wrote:
> > > > On Fri, Jun 19, 2015 at 03:13:36PM +0200, Patrick McHardy wrote:
> > > > > >  static int do_add_setelems(struct netlink_ctx *ctx, const struct handle *h,
> > > > > > -			   const struct expr *expr)
> > > > > > +			   const struct location *loc, struct expr *expr)
> > > > > >  {
> > > > > > -	if (netlink_add_setelems(ctx, h, expr) < 0)
> > > > > > +	struct set *set;
> > > > > > +
> > > > > > +	if (netlink_get_set(ctx, h, loc) < 0)
> > > > > 
> > > > > I think we should get it from the internal list and not from the
> > > > > kernel.
> > > > 
> > > > There is no such internal list at this moment, we retrieve the list
> > > > only for do_command_list(). Are you suggesting to add the code to
> > > > retrieve it inconditionally initially?
> > > 
> > > We do have the table->sets list. Yeah, but we don't add it to that
> > > list in the creation part, I see. Actually we should be doing that,
> > > since otherwise we also won't support creating a set and adding
> > > new elements in seperate commands but a single transaction.
> > 
> > Right.
> > 
> > > > > We can't add intervals to existing sets so far anyways, and this
> > > > > would allow it, but it wouldn't work.
> > > > 
> > > > Not sure what you mean with this.
> > > 
> > > Intervals need to know the entire set content to be created correctly.
> > > We don't handle incremental updates with intervals correctly ATM.
> > 
> > What's the problem?
> 
> Consider a set with an interval 0-10. We add an interval 9-11. This
> effectively means 0-10 needs to be transformed to 0-8 and we don't
> do that.
> 
> The root cause is that the kernel must not have overlapping intervals.
> They need to be transformed before adding them. This is what segtree
> does.

OK, so that transformation would look like:

1) Fetch the existing elements in the set via netlink.
2) Handle merges with the elements that the user has passed through
   command line.
3) Build the segtree.
4) Push it into the kernel. We need to mark all existing elements for
   the deletion plus add the new elements, all that in one single
   transaction.

Is this your idea? So it looks like we need a bit more userspace code.

With the existing approach, the kernel rejects overlapping segments
with -EEXIST, so if the user is careful to avoid them there should be
no problem. It's more restrictive than what the logic above, but set
declarations with intervals will work until that code lands in the
tree.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Patrick McHardy]

Am 19. Juni 2015 19:40:46 MESZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>On Fri, Jun 19, 2015 at 03:59:48PM +0200, Patrick McHardy wrote:
>> On 19.06, Pablo Neira Ayuso wrote:
>> > On Fri, Jun 19, 2015 at 03:44:44PM +0200, Patrick McHardy wrote:
>> > > On 19.06, Pablo Neira Ayuso wrote:
>> > > > On Fri, Jun 19, 2015 at 03:13:36PM +0200, Patrick McHardy
>wrote:
>> > > > > >  static int do_add_setelems(struct netlink_ctx *ctx, const
>struct handle *h,
>> > > > > > -			   const struct expr *expr)
>> > > > > > +			   const struct location *loc, struct expr *expr)
>> > > > > >  {
>> > > > > > -	if (netlink_add_setelems(ctx, h, expr) < 0)
>> > > > > > +	struct set *set;
>> > > > > > +
>> > > > > > +	if (netlink_get_set(ctx, h, loc) < 0)
>> > > > > 
>> > > > > I think we should get it from the internal list and not from
>the
>> > > > > kernel.
>> > > > 
>> > > > There is no such internal list at this moment, we retrieve the
>list
>> > > > only for do_command_list(). Are you suggesting to add the code
>to
>> > > > retrieve it inconditionally initially?
>> > > 
>> > > We do have the table->sets list. Yeah, but we don't add it to
>that
>> > > list in the creation part, I see. Actually we should be doing
>that,
>> > > since otherwise we also won't support creating a set and adding
>> > > new elements in seperate commands but a single transaction.
>> > 
>> > Right.
>> > 
>> > > > > We can't add intervals to existing sets so far anyways, and
>this
>> > > > > would allow it, but it wouldn't work.
>> > > > 
>> > > > Not sure what you mean with this.
>> > > 
>> > > Intervals need to know the entire set content to be created
>correctly.
>> > > We don't handle incremental updates with intervals correctly ATM.
>> > 
>> > What's the problem?
>> 
>> Consider a set with an interval 0-10. We add an interval 9-11. This
>> effectively means 0-10 needs to be transformed to 0-8 and we don't
>> do that.
>> 
>> The root cause is that the kernel must not have overlapping
>intervals.
>> They need to be transformed before adding them. This is what segtree
>> does.
>
>OK, so that transformation would look like:
>
>1) Fetch the existing elements in the set via netlink.
>2) Handle merges with the elements that the user has passed through
>   command line.
>3) Build the segtree.
>4) Push it into the kernel. We need to mark all existing elements for
>   the deletion plus add the new elements, all that in one single
>   transaction.
>
>Is this your idea? So it looks like we need a bit more userspace code.

Yep, almost. The segtree will take care of overlaps, but it priotizes based on interval size, in case of incremental updates I think the newer intervals should always take precedence.

Regarding updating the kernel, we need a Set generation id to make sure we're transforming the correct contents.

>
>With the existing approach, the kernel rejects overlapping segments
>with -EEXIST, so if the user is careful to avoid them there should be
>no problem. It's more restrictive than what the logic above, but set
>declarations with intervals will work until that code lands in the
>tree.
>
>Thanks.

-- 
Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Patrick McHardy]

Am 19. Juni 2015 19:40:46 MESZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>On Fri, Jun 19, 2015 at 03:59:48PM +0200, Patrick McHardy wrote:
>> On 19.06, Pablo Neira Ayuso wrote:
>> > On Fri, Jun 19, 2015 at 03:44:44PM +0200, Patrick McHardy wrote:
>> > > On 19.06, Pablo Neira Ayuso wrote:
>> > > > On Fri, Jun 19, 2015 at 03:13:36PM +0200, Patrick McHardy
>wrote:
>> > > > > >  static int do_add_setelems(struct netlink_ctx *ctx, const
>struct handle *h,
>> > > > > > -			   const struct expr *expr)
>> > > > > > +			   const struct location *loc, struct expr *expr)
>> > > > > >  {
>> > > > > > -	if (netlink_add_setelems(ctx, h, expr) < 0)
>> > > > > > +	struct set *set;
>> > > > > > +
>> > > > > > +	if (netlink_get_set(ctx, h, loc) < 0)
>> > > > > 
>> > > > > I think we should get it from the internal list and not from
>the
>> > > > > kernel.
>> > > > 
>> > > > There is no such internal list at this moment, we retrieve the
>list
>> > > > only for do_command_list(). Are you suggesting to add the code
>to
>> > > > retrieve it inconditionally initially?
>> > > 
>> > > We do have the table->sets list. Yeah, but we don't add it to
>that
>> > > list in the creation part, I see. Actually we should be doing
>that,
>> > > since otherwise we also won't support creating a set and adding
>> > > new elements in seperate commands but a single transaction.
>> > 
>> > Right.
>> > 
>> > > > > We can't add intervals to existing sets so far anyways, and
>this
>> > > > > would allow it, but it wouldn't work.
>> > > > 
>> > > > Not sure what you mean with this.
>> > > 
>> > > Intervals need to know the entire set content to be created
>correctly.
>> > > We don't handle incremental updates with intervals correctly ATM.
>> > 
>> > What's the problem?
>> 
>> Consider a set with an interval 0-10. We add an interval 9-11. This
>> effectively means 0-10 needs to be transformed to 0-8 and we don't
>> do that.
>> 
>> The root cause is that the kernel must not have overlapping
>intervals.
>> They need to be transformed before adding them. This is what segtree
>> does.
>
>OK, so that transformation would look like:
>
>1) Fetch the existing elements in the set via netlink.
>2) Handle merges with the elements that the user has passed through
>   command line.
>3) Build the segtree.
>4) Push it into the kernel. We need to mark all existing elements for
>   the deletion plus add the new elements, all that in one single
>   transaction.
>
>Is this your idea? So it looks like we need a bit more userspace code.
>
>With the existing approach, the kernel rejects overlapping segments
>with -EEXIST, so if the user is careful to avoid them there should be
>no problem. It's more restrictive than what the logic above, but set
>declarations with intervals will work until that code lands in the
>tree.

Sorry, missed this part. Are you sure about that? I'm pretty sure we only reject exact duplicates. Otherwise I'd agree, that would be fine for now.


>
>Thanks.

-- 
Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Pablo Neira Ayuso]

On Fri, Jun 19, 2015 at 08:15:01PM +0200, Patrick McHardy wrote:
[...]
> >OK, so that transformation would look like:
> >
> >1) Fetch the existing elements in the set via netlink.
> >2) Handle merges with the elements that the user has passed through
> >   command line.
> >3) Build the segtree.
> >4) Push it into the kernel. We need to mark all existing elements for
> >   the deletion plus add the new elements, all that in one single
> >   transaction.
> >
> >Is this your idea? So it looks like we need a bit more userspace code.
> >
> >With the existing approach, the kernel rejects overlapping segments
> >with -EEXIST, so if the user is careful to avoid them there should be
> >no problem. It's more restrictive than what the logic above, but set
> >declarations with intervals will work until that code lands in the
> >tree.
> 
> Sorry, missed this part. Are you sure about that? I'm pretty sure we
> only reject exact duplicates. Otherwise I'd agree, that would be
> fine for now.

Yes, overlapping segments are rejected:

# nft add element test myset { 1.2.3.0/24 }
# nft add element test myset { 1.2.3.1 }
<cmdline>:1:1-34: Error: Could not process rule: File exists
add element test myset { 1.2.3.1 }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# nft add element test myset { 1.2.3.0 }
<cmdline>:1:1-34: Error: Could not process rule: File exists
add element test myset { 1.2.3.0 }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# nft add element test myset { 1.2.3.255 }
<cmdline>:1:1-36: Error: Could not process rule: File exists
add element test myset { 1.2.3.255 }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
# nft add element test myset { 1.2.3.30-1.2.4.30 }
<cmdline>:1:1-44: Error: Could not process rule: File exists
add element test myset { 1.2.3.30-1.2.4.30 }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I'll send a v2 of this patch to replace the netlink_get_set() call so
this also works for set declarations in one single transaction.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Patrick McHardy]

On 20.06, Pablo Neira Ayuso wrote:
> On Fri, Jun 19, 2015 at 08:15:01PM +0200, Patrick McHardy wrote:
> [...]
> > >OK, so that transformation would look like:
> > >
> > >1) Fetch the existing elements in the set via netlink.
> > >2) Handle merges with the elements that the user has passed through
> > >   command line.
> > >3) Build the segtree.
> > >4) Push it into the kernel. We need to mark all existing elements for
> > >   the deletion plus add the new elements, all that in one single
> > >   transaction.
> > >
> > >Is this your idea? So it looks like we need a bit more userspace code.
> > >
> > >With the existing approach, the kernel rejects overlapping segments
> > >with -EEXIST, so if the user is careful to avoid them there should be
> > >no problem. It's more restrictive than what the logic above, but set
> > >declarations with intervals will work until that code lands in the
> > >tree.
> > 
> > Sorry, missed this part. Are you sure about that? I'm pretty sure we
> > only reject exact duplicates. Otherwise I'd agree, that would be
> > fine for now.
> 
> Yes, overlapping segments are rejected:
> 
> # nft add element test myset { 1.2.3.0/24 }
> # nft add element test myset { 1.2.3.1 }
> <cmdline>:1:1-34: Error: Could not process rule: File exists
> add element test myset { 1.2.3.1 }
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> # nft add element test myset { 1.2.3.0 }
> <cmdline>:1:1-34: Error: Could not process rule: File exists
> add element test myset { 1.2.3.0 }
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> # nft add element test myset { 1.2.3.255 }
> <cmdline>:1:1-36: Error: Could not process rule: File exists
> add element test myset { 1.2.3.255 }
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> # nft add element test myset { 1.2.3.30-1.2.4.30 }
> <cmdline>:1:1-44: Error: Could not process rule: File exists
> add element test myset { 1.2.3.30-1.2.4.30 }
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think this is basically by accident. Does it also reject if the
second segment starts *before* the first one? IOW, 192.168.1.0/24
and 192.168.0.0/16?

> I'll send a v2 of this patch to replace the netlink_get_set() call so
> this also works for set declarations in one single transaction.

That is a good change in any case.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in

Re: [PATCH nft 3/3] rule: fix use of intervals in set declarations

[Pablo Neira Ayuso]

On Sat, Jun 20, 2015 at 01:41:34PM +0200, Patrick McHardy wrote:
> On 20.06, Pablo Neira Ayuso wrote:
> > On Fri, Jun 19, 2015 at 08:15:01PM +0200, Patrick McHardy wrote:
> > [...]
> > > >OK, so that transformation would look like:
> > > >
> > > >1) Fetch the existing elements in the set via netlink.
> > > >2) Handle merges with the elements that the user has passed through
> > > >   command line.
> > > >3) Build the segtree.
> > > >4) Push it into the kernel. We need to mark all existing elements for
> > > >   the deletion plus add the new elements, all that in one single
> > > >   transaction.
> > > >
> > > >Is this your idea? So it looks like we need a bit more userspace code.
> > > >
> > > >With the existing approach, the kernel rejects overlapping segments
> > > >with -EEXIST, so if the user is careful to avoid them there should be
> > > >no problem. It's more restrictive than what the logic above, but set
> > > >declarations with intervals will work until that code lands in the
> > > >tree.
> > > 
> > > Sorry, missed this part. Are you sure about that? I'm pretty sure we
> > > only reject exact duplicates. Otherwise I'd agree, that would be
> > > fine for now.
> > 
> > Yes, overlapping segments are rejected:
> > 
> > # nft add element test myset { 1.2.3.0/24 }
> > # nft add element test myset { 1.2.3.1 }
> > <cmdline>:1:1-34: Error: Could not process rule: File exists
> > add element test myset { 1.2.3.1 }
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > # nft add element test myset { 1.2.3.0 }
> > <cmdline>:1:1-34: Error: Could not process rule: File exists
> > add element test myset { 1.2.3.0 }
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > # nft add element test myset { 1.2.3.255 }
> > <cmdline>:1:1-36: Error: Could not process rule: File exists
> > add element test myset { 1.2.3.255 }
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > # nft add element test myset { 1.2.3.30-1.2.4.30 }
> > <cmdline>:1:1-44: Error: Could not process rule: File exists
> > add element test myset { 1.2.3.30-1.2.4.30 }
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I think this is basically by accident. Does it also reject if the
> second segment starts *before* the first one? IOW, 192.168.1.0/24
> and 192.168.0.0/16?

At quick test it does:

# nft add element test myset2 { 192.168.1.0/24 }
# nft add element test myset2 { 192.168.1.0/16 }
<cmdline>:1:1-42: Error: Could not process rule: File exists
add element test myset2 { 192.168.1.0/16 }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

# nft add element test myset3 { 192.168.1.0/16 }
# nft add element test myset3 { 192.168.1.0/24 }
<cmdline>:1:1-42: Error: Could not process rule: File exists
add element test myset4 { 192.168.1.0/24 }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in