add insert after to nf_tables

The patch
netfilter: nf_tables: add insert operation
adds support for inserting a rule after a handle.

It is followed by the patch
examples: add insert rule example
which is the libnftables example.

I choose to reuse the CREATE operation in the kernel code to avoid to add a new message to netlink. This way we have a sort of 'create after' syntax. This is almost natural IMHO.

Hi Eric,

Quoted 11 lines

The patch
netfilter: nf_tables: add insert operation
adds support for inserting a rule after a handle.

It is followed by the patch
examples: add insert rule example
which is the libnftables example.

I choose to reuse the CREATE operation in the kernel code to avoid to add a new message to netlink. This way we have a sort of 'create after' syntax. This is almost natural IMHO.

There is an issue however: notification.
I don't see how other clients are going to know where to put the rule when updating there own list when they get notified.

They will be notified that a rule as been added successfully, but they will get it as an appended rule.

The user-space daemon can keep a cache of the ordered rule-set. Thus, it can guess the position of the rule-set from the handle.

Hi Pablo,

Quoted 2 lines

The user-space daemon can keep a cache of the ordered rule-set. Thus, it can guess the position of the rule-set from the handle.

Hum, how?
The handle it will get from the notification is the handle of the newly created rule, not the one used to identify the rule for insertion.

Quoted 4 lines

Hum, how?
The handle it will get from the notification is the handle of the newly created rule, not the one used to identify the rule for insertion.

That's right. I don't come with any other way to make it rather than adding a new attribute.

Hi Pablo,

Quoted 6 lines

Quoted 4 lines

Hum, how?
The handle it will get from the notification is the handle of the newly created rule, not the one used to identify the rule for insertion.

That's right. I don't come with any other way to make it rather than adding a new attribute.

Yes, though it breaks the design logic of the current API, somehow.

I mean, attributes are currently reflecting the rule as it is, and are used symmetrically in queries/replies.

Here what we need is a temporary extra attribute, or some sort, only used for the notification.

Either via:

we don't add an element to enum nft_rule_attributes {}, instead we create another enum for attributes only used on notification. like enum nft_rule_extras_notifications_attributes {}

Or via (maybe better for nla policy way of working?):

Adding a nft_rule_attributes as NFTA_RULE_EXTRAS_NOTIFICATIONS as a nested attribute
and then enum nft_rule_extras_notifications_attributes {} again, etc etc...

It's just a quick proposal, but my point here is to keep the API semantically sane.
So it won't require extra guesses to understand how it's supposed to work (as it is right now: it's a sane API, besides the lonely NFT_RULE_F_COMMIT in its anonymous enum)

Maybe there is a better way, probably. But you get my point.

Quoted 1 line

Quoted 1 line

Maybe there is a better way, probably. But you get my point.

We could instead of using NLA_RULE_HANDLE for the position add a new attribute NLA_RULE_POSITION and use that both for creating rules and for notifications. It would always be set and contain the handle of the rule preceeding the new rule (for NLM_F_APPEND) or the one following it (for !NLM_F_APPEND).

Quoted 6 lines · Tomasz Bursztyka

Hi Pablo,

Quoted 4 lines

Quoted 4 lines

Hum, how?
The handle it will get from the notification is the handle of the newly created rule, not the one used to identify the rule for insertion.

LE_F_COMMIT in its anonymous enum)

Quoted 2 lines

Maybe there is a better way, probably. But you get my point.

We could instead of using NLA_RULE_HANDLE for the position add a new attribute NLA_RULE_POSITION and use that both for creating rules and for notifications.

It would always be set and contain the handle of
the rule preceeding the new rule (for NLM_F_APPEND) or the one following it (for !NLM_F_APPEND).

This patch adds a new rule attribute NFTA_RULE_POSITION which is used to store the position of a rule relatively to the others. By providing a create command and specifying a position, the rule is inserted after the rule with the handle equal to the provided position.
Regarding notification, the position attribute is added to specify the handle of the previous rule in append mode and the handle of the next rule in the other case.

Signed-off-by Eric Leblond <[email protected]>

---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 32 +++++++++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 3 deletions(-)

include/uapi/linux/netfilter/nf_tables.h +1 -0

index d40a7f9..d9bf8ea 100644
@@ -143,6 +143,7 @@ enum nft_rule_attributes {
 	NFTA_RULE_EXPRESSIONS,
 	NFTA_RULE_FLAGS,
 	NFTA_RULE_COMPAT,
+	NFTA_RULE_POSITION,
 	__NFTA_RULE_MAX
 };
 #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)

net/netfilter/nf_tables_api.c +1 -0

index b00aca8..a03aa9f 100644
@@ -1267,6 +1267,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
 	[NFTA_RULE_FLAGS]	= { .type = NLA_U32 },
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
+	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
 };
 
 static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,

net/netfilter/nf_tables_api.c +27 -3

+		if (nla_put_be64(skb, NFTA_RULE_POSITION,
+				 cpu_to_be64(((struct nft_rule *)rule->list.prev)->handle)))
+			goto nla_put_failure;
+	} else if (rule->list.next) {
+		if (nla_put_be64(skb, NFTA_RULE_POSITION,
+				 cpu_to_be64(((struct nft_rule *)rule->list.next)->handle)))
+			goto nla_put_failure;
+	} else
+		goto nla_put_failure;
+
 	list = nla_nest_start(skb, NFTA_RULE_EXPRESSIONS);
 	if (list == NULL)
 		goto nla_put_failure;

@@ -1537,7 +1549,7 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 	int err, rem;
 	bool create;
 	u32 flags = 0;
-	u64 handle;
+	u64 handle, pos_handle;
 
 	create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
 

@@ -1571,6 +1583,16 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 		handle = nf_tables_alloc_handle(table);
 	}
 
+	if (nla[NFTA_RULE_POSITION]) {
+		pos_handle = be64_to_cpu(nla_get_be64(nla[NFTA_RULE_POSITION]));
+		old_rule = __nf_tables_rule_lookup(chain, pos_handle);
+		if (IS_ERR(old_rule))
+			return PTR_ERR(old_rule);
+
+		if (! (nlh->nlmsg_flags & NLM_F_CREATE))
+			return -EOPNOTSUPP;
+	}
+
 	nft_ctx_init(&ctx, skb, nlh, afi, table, chain, nla);
 
 	n = 0;

@@ -1626,8 +1648,12 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 		}
 	} else if (nlh->nlmsg_flags & NLM_F_APPEND)
 		list_add_tail_rcu(&rule->list, &chain->rules);
-	else
-		list_add_rcu(&rule->list, &chain->rules);
+	else {
+		if (old_rule)
+			list_add_rcu(&rule->list, &old_rule->list);
+		else
+			list_add_rcu(&rule->list, &chain->rules);
+	}
 
 	if (flags & NFT_RULE_F_COMMIT)
 		list_add(&rule->dirty_list, &chain->dirty_rules);

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

This patch adds a new rule attribute NFTA_RULE_POSITION which is used to store the position of a rule relatively to the others. By providing a create command and specifying a position, the rule is inserted after the rule with the handle equal to the provided position.
Regarding notification, the position attribute is added to specify the handle of the previous rule in append mode and the handle of the next rule in the other case.

Signed-off-by Eric Leblond <[email protected]>

---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 47 +++++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 4 deletions(-)

include/uapi/linux/netfilter/nf_tables.h +1 -0

index d40a7f9..d9bf8ea 100644
@@ -143,6 +143,7 @@ enum nft_rule_attributes {
 	NFTA_RULE_EXPRESSIONS,
 	NFTA_RULE_FLAGS,
 	NFTA_RULE_COMPAT,
+	NFTA_RULE_POSITION,
 	__NFTA_RULE_MAX
 };
 #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)

net/netfilter/nf_tables_api.c +2 -0

index b00aca8..ce8a4f0 100644
@@ -1267,6 +1267,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
 	[NFTA_RULE_FLAGS]	= { .type = NLA_U32 },
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
+	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
 };
 
 static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,

@@ -1279,6 +1280,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	struct nfgenmsg *nfmsg;
 	const struct nft_expr *expr, *next;
 	struct nlattr *list;
+	const struct nft_rule *prule;
 
 	event |= NFNL_SUBSYS_NFTABLES << 8;
 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),

net/netfilter/nf_tables_api.c +9 -1

+			prule = list_entry(rule->list.next,
+					   struct nft_rule, list);
+			if (nla_put_be64(skb, NFTA_RULE_POSITION,
+					 cpu_to_be64(prule->handle)))
+				goto nla_put_failure;
+		}
+	}
+
 	list = nla_nest_start(skb, NFTA_RULE_EXPRESSIONS);
 	if (list == NULL)
 		goto nla_put_failure;

@@ -1537,7 +1559,7 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 	int err, rem;
 	bool create;
 	u32 flags = 0;
-	u64 handle;
+	u64 handle, pos_handle;
 
 	create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
 

@@ -1571,6 +1593,16 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 		handle = nf_tables_alloc_handle(table);

net/netfilter/nf_tables_api.c +12 -3

+	}
+
 	nft_ctx_init(&ctx, skb, nlh, afi, table, chain, nla);
 
 	n = 0;

@@ -1625,9 +1657,16 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 			nf_tables_rule_destroy(old_rule);
 		}
 	} else if (nlh->nlmsg_flags & NLM_F_APPEND)
-		list_add_tail_rcu(&rule->list, &chain->rules);
-	else
-		list_add_rcu(&rule->list, &chain->rules);
+		if (old_rule)
+			list_add_rcu(&rule->list, &old_rule->list);
+		else
+			list_add_tail_rcu(&rule->list, &chain->rules);
+	else {
+		if (old_rule)
+			list_add_tail_rcu(&rule->list, &old_rule->list);
+		else
+			list_add_rcu(&rule->list, &chain->rules);
+	}
 
 	if (flags & NFT_RULE_F_COMMIT)
 		list_add(&rule->dirty_list, &chain->dirty_rules);

Signature · 1 line

1.8.3.2

This patch adds a new rule attribute NFTA_RULE_POSITION which is used to store the position of a rule relatively to the others. By providing a create command and specifying a position, the rule is inserted after the rule with the handle equal to the provided position.
Regarding notification, the position attribute is added to specify the handle of the previous rule in append mode and the handle of the next rule in the other case.

Signed-off-by Eric Leblond <[email protected]>

---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 4 deletions(-)

include/uapi/linux/netfilter/nf_tables.h +1 -0

index d40a7f9..d9bf8ea 100644
@@ -143,6 +143,7 @@ enum nft_rule_attributes {
 	NFTA_RULE_EXPRESSIONS,
 	NFTA_RULE_FLAGS,
 	NFTA_RULE_COMPAT,
+	NFTA_RULE_POSITION,
 	__NFTA_RULE_MAX
 };
 #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)

net/netfilter/nf_tables_api.c +2 -0

index b00aca8..012eb1f 100644
@@ -1267,6 +1267,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
 	[NFTA_RULE_FLAGS]	= { .type = NLA_U32 },
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
+	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
 };
 
 static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,

@@ -1279,6 +1280,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	struct nfgenmsg *nfmsg;
 	const struct nft_expr *expr, *next;
 	struct nlattr *list;
+	const struct nft_rule *prule;
 
 	event |= NFNL_SUBSYS_NFTABLES << 8;
 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),

@@ -1298,6 +1300,26 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	if (nla_put_be64(skb, NFTA_RULE_HANDLE, cpu_to_be64(rule->handle)))
 		goto nla_put_failure;

net/netfilter/nf_tables_api.c +31 -4

+	} else {
+		if ((event != NFT_MSG_DELRULE) &&
+		    list_is_last(&rule->list, &chain->rules)) {
+			prule = list_entry(rule->list.next,
+					   struct nft_rule, list);
+			if (nla_put_be64(skb, NFTA_RULE_POSITION,
+					 cpu_to_be64(prule->handle)))
+				goto nla_put_failure;
+		}
+	}
+
 	list = nla_nest_start(skb, NFTA_RULE_EXPRESSIONS);
 	if (list == NULL)
 		goto nla_put_failure;

@@ -1537,7 +1559,7 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 	int err, rem;
 	bool create;
 	u32 flags = 0;
-	u64 handle;
+	u64 handle, pos_handle;
 
 	create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
 

@@ -1571,6 +1593,15 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 		handle = nf_tables_alloc_handle(table);
 	}
 
+	if (nla[NFTA_RULE_POSITION]) {
+		if (!(nlh->nlmsg_flags & NLM_F_CREATE))
+			return -EOPNOTSUPP;
+		pos_handle = be64_to_cpu(nla_get_be64(nla[NFTA_RULE_POSITION]));
+		old_rule = __nf_tables_rule_lookup(chain, pos_handle);
+		if (IS_ERR(old_rule))
+			return PTR_ERR(old_rule);
+	}
+
 	nft_ctx_init(&ctx, skb, nlh, afi, table, chain, nla);
 
 	n = 0;

@@ -1625,9 +1656,16 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 			nf_tables_rule_destroy(old_rule);
 		}
 	} else if (nlh->nlmsg_flags & NLM_F_APPEND)
-		list_add_tail_rcu(&rule->list, &chain->rules);
-	else
-		list_add_rcu(&rule->list, &chain->rules);
+		if (old_rule)
+			list_add_rcu(&rule->list, &old_rule->list);
+		else
+			list_add_tail_rcu(&rule->list, &chain->rules);
+	else {
+		if (old_rule)
+			list_add_tail_rcu(&rule->list, &old_rule->list);
+		else
+			list_add_rcu(&rule->list, &chain->rules);
+	}
 
 	if (flags & NFT_RULE_F_COMMIT)
 		list_add(&rule->dirty_list, &chain->dirty_rules);

Signature · 1 line

1.8.3.2

Hi Eric,

Quoted 50 lines · Eric Leblond

This patch adds a new rule attribute NFTA_RULE_POSITION which is used to store the position of a rule relatively to the others. By providing a create command and specifying a position, the rule is inserted after the rule with the handle equal to the provided position.
Regarding notification, the position attribute is added to specify the handle of the previous rule in append mode and the handle of the next rule in the other case.

Signed-off-by Eric Leblond <[email protected]>

---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 4 deletions(-)

include/uapi/linux/netfilter/nf_tables.h +1 -0

index d40a7f9..d9bf8ea 100644
@@ -143,6 +143,7 @@ enum nft_rule_attributes {
 	NFTA_RULE_EXPRESSIONS,
 	NFTA_RULE_FLAGS,
 	NFTA_RULE_COMPAT,
+	NFTA_RULE_POSITION,
 	__NFTA_RULE_MAX
 };
 #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)

net/netfilter/nf_tables_api.c +2 -0

index b00aca8..012eb1f 100644
@@ -1267,6 +1267,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
 	[NFTA_RULE_FLAGS]	= { .type = NLA_U32 },
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
+	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
 };
 
 static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,

@@ -1279,6 +1280,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	struct nfgenmsg *nfmsg;
 	const struct nft_expr *expr, *next;
 	struct nlattr *list;
+	const struct nft_rule *prule;
 
 	event |= NFNL_SUBSYS_NFTABLES << 8;
 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),

@@ -1298,6 +1300,26 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	if (nla_put_be64(skb, NFTA_RULE_HANDLE, cpu_to_be64(rule->handle)))
 		goto nla_put_failure;

net/netfilter/nf_tables_api.c static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq, · line 1311

Quoted 10 lines

net/netfilter/nf_tables_api.c +9 -0

 
+	if (flags & NLM_F_APPEND) {
+		if ((event != NFT_MSG_DELRULE) &&
+		    (rule->list.prev != &chain->rules)) {
+			prule = list_entry(rule->list.prev,
+					   struct nft_rule, list);
+			if (nla_put_be64(skb, NFTA_RULE_POSITION,
+					 cpu_to_be64(prule->handle)))
+				goto nla_put_failure;
+		}

This looks good but I think we can simplify this to always use .prev.

1) Append: we use .prev. If it's the first rule in the list, skip position attribute.

2) Insert: never dump position attribute as it is always the first rule in the list.

3) At position X: use .prev. If it's the first rule in the list, skip.

That should allows us to remove the else branch below and it should simplify the semantics of NFTA_RULE_POSITION as well.

Quoted 63 lines

net/netfilter/nf_tables_api.c +31 -4

+	} else {
+		if ((event != NFT_MSG_DELRULE) &&
+		    list_is_last(&rule->list, &chain->rules)) {
+			prule = list_entry(rule->list.next,
+					   struct nft_rule, list);
+			if (nla_put_be64(skb, NFTA_RULE_POSITION,
+					 cpu_to_be64(prule->handle)))
+				goto nla_put_failure;
+		}
+	}
+
 	list = nla_nest_start(skb, NFTA_RULE_EXPRESSIONS);
 	if (list == NULL)
 		goto nla_put_failure;

@@ -1537,7 +1559,7 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 	int err, rem;
 	bool create;
 	u32 flags = 0;
-	u64 handle;
+	u64 handle, pos_handle;
 
 	create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
 

@@ -1571,6 +1593,15 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 		handle = nf_tables_alloc_handle(table);
 	}
 
+	if (nla[NFTA_RULE_POSITION]) {
+		if (!(nlh->nlmsg_flags & NLM_F_CREATE))
+			return -EOPNOTSUPP;
+		pos_handle = be64_to_cpu(nla_get_be64(nla[NFTA_RULE_POSITION]));
+		old_rule = __nf_tables_rule_lookup(chain, pos_handle);
+		if (IS_ERR(old_rule))
+			return PTR_ERR(old_rule);
+	}
+
 	nft_ctx_init(&ctx, skb, nlh, afi, table, chain, nla);
 
 	n = 0;

@@ -1625,9 +1656,16 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 			nf_tables_rule_destroy(old_rule);
 		}
 	} else if (nlh->nlmsg_flags & NLM_F_APPEND)
-		list_add_tail_rcu(&rule->list, &chain->rules);
-	else
-		list_add_rcu(&rule->list, &chain->rules);
+		if (old_rule)
+			list_add_rcu(&rule->list, &old_rule->list);
+		else
+			list_add_tail_rcu(&rule->list, &chain->rules);
+	else {
+		if (old_rule)
+			list_add_tail_rcu(&rule->list, &old_rule->list);
+		else
+			list_add_rcu(&rule->list, &chain->rules);
+	}
 
 	if (flags & NFT_RULE_F_COMMIT)
 		list_add(&rule->dirty_list, &chain->dirty_rules);

Signature · 1 line

1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

Quoted 69 lines · Pablo Neira Ayuso

Hi Eric,

Quoted 50 lines · Eric Leblond

This patch adds a new rule attribute NFTA_RULE_POSITION which is used to store the position of a rule relatively to the others. By providing a create command and specifying a position, the rule is inserted after the rule with the handle equal to the provided position.
Regarding notification, the position attribute is added to specify the handle of the previous rule in append mode and the handle of the next rule in the other case.

Signed-off-by Eric Leblond <[email protected]>

---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 4 deletions(-)

include/uapi/linux/netfilter/nf_tables.h +1 -0

index d40a7f9..d9bf8ea 100644
@@ -143,6 +143,7 @@ enum nft_rule_attributes {
 	NFTA_RULE_EXPRESSIONS,
 	NFTA_RULE_FLAGS,
 	NFTA_RULE_COMPAT,
+	NFTA_RULE_POSITION,
 	__NFTA_RULE_MAX
 };
 #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)

net/netfilter/nf_tables_api.c +2 -0

index b00aca8..012eb1f 100644
@@ -1267,6 +1267,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
 	[NFTA_RULE_FLAGS]	= { .type = NLA_U32 },
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
+	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
 };
 
 static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,

@@ -1279,6 +1280,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	struct nfgenmsg *nfmsg;
 	const struct nft_expr *expr, *next;
 	struct nlattr *list;
+	const struct nft_rule *prule;
 
 	event |= NFNL_SUBSYS_NFTABLES << 8;
 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),

@@ -1298,6 +1300,26 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	if (nla_put_be64(skb, NFTA_RULE_HANDLE, cpu_to_be64(rule->handle)))
 		goto nla_put_failure;

net/netfilter/nf_tables_api.c static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq, · line 1311

Quoted 10 lines

net/netfilter/nf_tables_api.c +9 -0

 
+	if (flags & NLM_F_APPEND) {
+		if ((event != NFT_MSG_DELRULE) &&
+		    (rule->list.prev != &chain->rules)) {
+			prule = list_entry(rule->list.prev,
+					   struct nft_rule, list);
+			if (nla_put_be64(skb, NFTA_RULE_POSITION,
+					 cpu_to_be64(prule->handle)))
+				goto nla_put_failure;
+		}

This looks good but I think we can simplify this to always use .prev.

1) Append: we use .prev. If it's the first rule in the list, skip position attribute.

Quoted 7 lines

2) Insert: never dump position attribute as it is always the first rule in the list.

3) At position X: use .prev. If it's the first rule in the list, skip.

That should allows us to remove the else branch below and it should simplify the semantics of NFTA_RULE_POSITION as well.

This code is not pretty and I understand your point but we have two type of operations:
* Insert before
* Append after
In both cases, the presence of the NFTA_RULE_POSITION attribute is triggering the switch to this mode. And I think this is a convenient way to update the API.

Furthermore, inside nf_tables_fill_rule_info() we don't have any information to decide that we are in "position X".

Only solution to switch to the method you describe would be to introduce a new operation and it seems that was is wanted (it was said in initial discussion).

Hi Eric,

Quoted 80 lines · Eric Leblond

Hi,

Quoted 69 lines · Pablo Neira Ayuso

Hi Eric,

Quoted 50 lines · Eric Leblond

This patch adds a new rule attribute NFTA_RULE_POSITION which is used to store the position of a rule relatively to the others. By providing a create command and specifying a position, the rule is inserted after the rule with the handle equal to the provided position.
Regarding notification, the position attribute is added to specify the handle of the previous rule in append mode and the handle of the next rule in the other case.

Signed-off-by Eric Leblond <[email protected]>

---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 4 deletions(-)

include/uapi/linux/netfilter/nf_tables.h +1 -0

index d40a7f9..d9bf8ea 100644
@@ -143,6 +143,7 @@ enum nft_rule_attributes {
 	NFTA_RULE_EXPRESSIONS,
 	NFTA_RULE_FLAGS,
 	NFTA_RULE_COMPAT,
+	NFTA_RULE_POSITION,
 	__NFTA_RULE_MAX
 };
 #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)

net/netfilter/nf_tables_api.c +2 -0

index b00aca8..012eb1f 100644
@@ -1267,6 +1267,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
 	[NFTA_RULE_FLAGS]	= { .type = NLA_U32 },
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
+	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
 };
 
 static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,

@@ -1279,6 +1280,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	struct nfgenmsg *nfmsg;
 	const struct nft_expr *expr, *next;
 	struct nlattr *list;
+	const struct nft_rule *prule;
 
 	event |= NFNL_SUBSYS_NFTABLES << 8;
 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),

@@ -1298,6 +1300,26 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	if (nla_put_be64(skb, NFTA_RULE_HANDLE, cpu_to_be64(rule->handle)))
 		goto nla_put_failure;

net/netfilter/nf_tables_api.c static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq, · line 1311

Quoted 10 lines

net/netfilter/nf_tables_api.c +9 -0

 
+	if (flags & NLM_F_APPEND) {
+		if ((event != NFT_MSG_DELRULE) &&
+		    (rule->list.prev != &chain->rules)) {
+			prule = list_entry(rule->list.prev,
+					   struct nft_rule, list);
+			if (nla_put_be64(skb, NFTA_RULE_POSITION,
+					 cpu_to_be64(prule->handle)))
+				goto nla_put_failure;
+		}

This looks good but I think we can simplify this to always use .prev.

1) Append: we use .prev. If it's the first rule in the list, skip position attribute.

Quoted 7 lines

2) Insert: never dump position attribute as it is always the first rule in the list.

3) At position X: use .prev. If it's the first rule in the list, skip.

That should allows us to remove the else branch below and it should simplify the semantics of NFTA_RULE_POSITION as well.

Quoted 7 lines

This code is not pretty and I understand your point but we have two type of operations:
* Insert before
* Append after
In both cases, the presence of the NFTA_RULE_POSITION attribute is triggering the switch to this mode. And I think this is a convenient way to update the API.

I see. Then we need to save the append flag to report events correctly in the commit path for the "insert after" case, according to this approach (that's should be easy to resolve though with a follow up patch).

Quoted 2 lines

Furthermore, inside nf_tables_fill_rule_info() we don't have any information to decide that we are in "position X".

But we know the handle of our .prev node for that new rule we added, that can be used to report back our relative location to it, ie. always return the previous node.

This however results in reporting back a different handle via the event notification in the "insert after" case (instead of the original handle passed via the rule_position attribute).

Quoted 3 lines

Only solution to switch to the method you describe would be to introduce a new operation and it seems that was is wanted (it was said in initial discussion).

Sure, I just wanted to discuss the nf_tables_fill_rule_info path, not questioning the entire patch.

Hello Pablo,

Quoted 91 lines · Pablo Neira Ayuso

Hi Eric,

Quoted 80 lines · Eric Leblond

Hi,

Quoted 69 lines · Pablo Neira Ayuso

Hi Eric,

Quoted 50 lines · Eric Leblond

This patch adds a new rule attribute NFTA_RULE_POSITION which is used to store the position of a rule relatively to the others. By providing a create command and specifying a position, the rule is inserted after the rule with the handle equal to the provided position.
Regarding notification, the position attribute is added to specify the handle of the previous rule in append mode and the handle of the next rule in the other case.

Signed-off-by Eric Leblond <[email protected]>

---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 4 deletions(-)

include/uapi/linux/netfilter/nf_tables.h +1 -0

index d40a7f9..d9bf8ea 100644
@@ -143,6 +143,7 @@ enum nft_rule_attributes {
 	NFTA_RULE_EXPRESSIONS,
 	NFTA_RULE_FLAGS,
 	NFTA_RULE_COMPAT,
+	NFTA_RULE_POSITION,
 	__NFTA_RULE_MAX
 };
 #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)

net/netfilter/nf_tables_api.c +2 -0

index b00aca8..012eb1f 100644
@@ -1267,6 +1267,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
 	[NFTA_RULE_FLAGS]	= { .type = NLA_U32 },
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
+	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
 };
 
 static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,

@@ -1279,6 +1280,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	struct nfgenmsg *nfmsg;
 	const struct nft_expr *expr, *next;
 	struct nlattr *list;
+	const struct nft_rule *prule;
 
 	event |= NFNL_SUBSYS_NFTABLES << 8;
 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),

@@ -1298,6 +1300,26 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	if (nla_put_be64(skb, NFTA_RULE_HANDLE, cpu_to_be64(rule->handle)))
 		goto nla_put_failure;

net/netfilter/nf_tables_api.c static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq, · line 1311

Quoted 10 lines

net/netfilter/nf_tables_api.c +9 -0

 
+	if (flags & NLM_F_APPEND) {
+		if ((event != NFT_MSG_DELRULE) &&
+		    (rule->list.prev != &chain->rules)) {
+			prule = list_entry(rule->list.prev,
+					   struct nft_rule, list);
+			if (nla_put_be64(skb, NFTA_RULE_POSITION,
+					 cpu_to_be64(prule->handle)))
+				goto nla_put_failure;
+		}

This looks good but I think we can simplify this to always use .prev.

1) Append: we use .prev. If it's the first rule in the list, skip position attribute.

Quoted 7 lines

2) Insert: never dump position attribute as it is always the first rule in the list.

3) At position X: use .prev. If it's the first rule in the list, skip.

That should allows us to remove the else branch below and it should simplify the semantics of NFTA_RULE_POSITION as well.

Quoted 7 lines

This code is not pretty and I understand your point but we have two type of operations:
* Insert before
* Append after
In both cases, the presence of the NFTA_RULE_POSITION attribute is triggering the switch to this mode. And I think this is a convenient way to update the API.

Quoted 4 lines

I see. Then we need to save the append flag to report events correctly in the commit path for the "insert after" case, according to this approach (that's should be easy to resolve though with a follow up patch).

IMHO, it is already there. At start of nf_tables_fill_rule_info, we are doing:

	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),
			flags);

So flags is used as flag inside nlh. And we can get the information during event by checking (nlh->nlmsg_flags & NLM_F_APPEND). I've done some tests with the attached patch and it seems to work well.

Quoted 103 lines · Eric Leblond

Hello Pablo,

Quoted 91 lines · Pablo Neira Ayuso

Hi Eric,

Quoted 80 lines · Eric Leblond

Hi,

Quoted 69 lines · Pablo Neira Ayuso

Hi Eric,

Quoted 50 lines · Eric Leblond

This patch adds a new rule attribute NFTA_RULE_POSITION which is used to store the position of a rule relatively to the others. By providing a create command and specifying a position, the rule is inserted after the rule with the handle equal to the provided position.
Regarding notification, the position attribute is added to specify the handle of the previous rule in append mode and the handle of the next rule in the other case.

Signed-off-by Eric Leblond <[email protected]>

---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 4 deletions(-)

include/uapi/linux/netfilter/nf_tables.h +1 -0

index d40a7f9..d9bf8ea 100644
@@ -143,6 +143,7 @@ enum nft_rule_attributes {
 	NFTA_RULE_EXPRESSIONS,
 	NFTA_RULE_FLAGS,
 	NFTA_RULE_COMPAT,
+	NFTA_RULE_POSITION,
 	__NFTA_RULE_MAX
 };
 #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)

net/netfilter/nf_tables_api.c +2 -0

index b00aca8..012eb1f 100644
@@ -1267,6 +1267,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
 	[NFTA_RULE_FLAGS]	= { .type = NLA_U32 },
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
+	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
 };
 
 static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,

@@ -1279,6 +1280,7 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	struct nfgenmsg *nfmsg;
 	const struct nft_expr *expr, *next;
 	struct nlattr *list;
+	const struct nft_rule *prule;
 
 	event |= NFNL_SUBSYS_NFTABLES << 8;
 	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),

@@ -1298,6 +1300,26 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
 	if (nla_put_be64(skb, NFTA_RULE_HANDLE, cpu_to_be64(rule->handle)))
 		goto nla_put_failure;

net/netfilter/nf_tables_api.c static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq, · line 1311

Quoted 10 lines

net/netfilter/nf_tables_api.c +9 -0

 
+	if (flags & NLM_F_APPEND) {
+		if ((event != NFT_MSG_DELRULE) &&
+		    (rule->list.prev != &chain->rules)) {
+			prule = list_entry(rule->list.prev,
+					   struct nft_rule, list);
+			if (nla_put_be64(skb, NFTA_RULE_POSITION,
+					 cpu_to_be64(prule->handle)))
+				goto nla_put_failure;
+		}

This looks good but I think we can simplify this to always use .prev.

1) Append: we use .prev. If it's the first rule in the list, skip position attribute.

Quoted 7 lines

2) Insert: never dump position attribute as it is always the first rule in the list.

3) At position X: use .prev. If it's the first rule in the list, skip.

That should allows us to remove the else branch below and it should simplify the semantics of NFTA_RULE_POSITION as well.

Quoted 7 lines

This code is not pretty and I understand your point but we have two type of operations:
* Insert before
* Append after
In both cases, the presence of the NFTA_RULE_POSITION attribute is triggering the switch to this mode. And I think this is a convenient way to update the API.

Quoted 4 lines

I see. Then we need to save the append flag to report events correctly in the commit path for the "insert after" case, according to this approach (that's should be easy to resolve though with a follow up patch).

IMHO, it is already there. At start of nf_tables_fill_rule_info, we are doing:

Quoted 6 lines

	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),
			flags);

So flags is used as flag inside nlh. And we can get the information during event by checking (nlh->nlmsg_flags & NLM_F_APPEND). I've done some tests with the attached patch and it seems to work well.

Please, see nf_tables_commit. In that path nf_tables_rule_notify does not get those flags at this moment, so it's returning its position in the insert fashion, ie. pointing to .next.

The problem with pointing to the .next node in the commit path is we are sure that .prev points to an existing rule. However, .next may point to stale rule. So I think we have to notify the position always relative to the .prev pointer to get this working in the commit path.

This patch adds a new rule attribute NFTA_RULE_POSITION which is used to store the position of a rule relatively to the others. By providing a create command and specifying a position, the rule is inserted after the rule with the handle equal to the provided position.
Regarding notification, the position attribute is added to specify the handle of the previous rule.

This program can insert a rule after a rule given by its handle.

The function nft_rule_attr_is_set() is doing no modification so it is possible to type it to const.

This patch adds support for position attribute which can be used to insert a rule at a given position.

This patch adds support for "insert before" and "add after" rule operation.
The rule handle syntax has an new optional after/before field which take a handle as argument.
Here is two examples:
nft add rule filter output after 5 ip daddr 1.2.3.1 drop nft insert rule filter output before 5 ip daddr 1.2.3.1 drop

From f8afcae4af949c1f8c71fc4dbbffdbddbedb7adf Mon Sep 17 00:00:00 2001

Hi Eric,

Quoted 7 lines · Eric Leblond

This patch adds support for "insert before" and "add after" rule operation.
The rule handle syntax has an new optional after/before field which take a handle as argument.
Here is two examples:
nft add rule filter output after 5 ip daddr 1.2.3.1 drop nft insert rule filter output before 5 ip daddr 1.2.3.1 drop

While testing this new feature, I noticed that the parser was accepting this:

nft add rule filter output after 5 ip daddr 1.2.3.1 drop nft insert rule filter output after 5 ip daddr 1.2.3.1 drop

Note that 'add' and 'insert' become semantically equivalent, which seems inconsistent to me.

While fixing it using the 'before' and 'after', I noticed that 'add' and 'insert' already tell us where to put the new rule, so 'after' and 'before' were repeating again what we want to do. I have reworked this patch to change this initial syntax:

nft add rule filter output position 5 ip daddr 1.2.3.1 drop nft insert rule filter output position 5 ip daddr 1.2.3.1 drop

We can support the after and before, but that would imply some extra evaluation after the parsing that would make the patch bigger. So I prefered to go the simpler solution.

Quoted 8 lines

While fixing it using the 'before' and 'after', I noticed that 'add' and 'insert' already tell us where to put the new rule, so 'after' and 'before' were repeating again what we want to do. I have reworked this patch to change this initial syntax:

nft add rule filter output position 5 ip daddr 1.2.3.1 drop nft insert rule filter output position 5 ip daddr 1.2.3.1 drop

Quoted 3 lines

We can support the after and before, but that would imply some extra evaluation after the parsing that would make the patch bigger. So I prefered to go the simpler solution.

I agree with the following modification. I did not find better than this so, it is ok for me :)

Patch tested. It works well.