[RFC PATCH 0/1] add insert after to nf_tables

[RFC PATCH 0/1] add insert after to nf_tables

[Eric Leblond]

Hello,

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.

BR,
--
Eric

[PATCH] netfilter: nf_tables: add insert operation

[Eric Leblond]

By providing a create command and specifying an handle, the rule is
inserted after the rule with the provided handle.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 net/netfilter/nf_tables_api.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index cfe0c58..4f3e112 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1563,7 +1563,10 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 			return -EEXIST;
 		if (nlh->nlmsg_flags & NLM_F_REPLACE)
 			old_rule = rule;
-		else
+		else if (nlh->nlmsg_flags & NLM_F_CREATE) {
+			old_rule = rule;
+			handle = nf_tables_alloc_handle(table);
+		} else
 			return -EOPNOTSUPP;
 	} else {
 		if (!create || nlh->nlmsg_flags & NLM_F_REPLACE)
@@ -1626,8 +1629,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);
-- 
1.8.3.1

[libnftables PATCH] examples: add insert rule example

[Eric Leblond]

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

Signed-off-by: Eric Leblond <eric@regit.org>
---
 examples/Makefile.am       |   4 +
 examples/nft-rule-insert.c | 203 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 207 insertions(+)
 create mode 100644 examples/nft-rule-insert.c

diff --git a/examples/Makefile.am b/examples/Makefile.am
index dcf798a..1d6b98f 100644
--- a/examples/Makefile.am
+++ b/examples/Makefile.am
@@ -10,6 +10,7 @@ check_PROGRAMS = nft-table-add		\
 		 nft-chain-del		\
 		 nft-chain-get		\
 		 nft-rule-add		\
+		 nft-rule-insert	\
 		 nft-rule-xml-add	\
 		 nft-rule-del		\
 		 nft-rule-get		\
@@ -52,6 +53,9 @@ nft_chain_get_LDADD = ../src/libnftables.la ${LIBMNL_LIBS} ${LIBXML_LIBS}
 nft_rule_add_SOURCES = nft-rule-add.c
 nft_rule_add_LDADD = ../src/libnftables.la ${LIBMNL_LIBS} ${LIBXML_LIBS}
 
+nft_rule_insert_SOURCES = nft-rule-insert.c
+nft_rule_insert_LDADD = ../src/libnftables.la ${LIBMNL_LIBS} ${LIBXML_LIBS}
+
 nft_rule_xml_add_SOURCES = nft-rule-xml-add.c
 nft_rule_xml_add_LDADD = ../src/libnftables.la ${LIBMNL_LIBS} ${LIBXML_LIBS}
 
diff --git a/examples/nft-rule-insert.c b/examples/nft-rule-insert.c
new file mode 100644
index 0000000..ddb7ad2
--- /dev/null
+++ b/examples/nft-rule-insert.c
@@ -0,0 +1,203 @@
+/*
+ * (C) 2012 by Pablo Neira Ayuso <pablo@netfilter.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This software has been sponsored by Sophos Astaro <http://www.sophos.com>
+ */
+
+#include <stdlib.h>
+#include <time.h>
+#include <string.h>
+#include <stddef.h>	/* for offsetof */
+#include <netinet/in.h>
+#include <arpa/inet.h>
+
+#include <linux/netfilter/nf_tables.h>
+
+#include <libmnl/libmnl.h>
+#include <libnftables/rule.h>
+#include <libnftables/expr.h>
+
+#include <linux/netfilter_ipv4/ipt_LOG.h>
+#include <linux/netfilter/xt_iprange.h>
+
+#include <netinet/ip.h>
+
+static void add_target_log(struct nft_rule_expr *e)
+{
+	struct ipt_log_info *info;
+
+	nft_rule_expr_set(e, NFT_EXPR_TG_NAME, "LOG", strlen("LOG"));
+	nft_rule_expr_set_u32(e, NFT_EXPR_TG_REV, 0);
+
+	info = calloc(1, sizeof(struct ipt_log_info));
+	if (info == NULL)
+		return;
+
+	sprintf(info->prefix, "test: ");
+	info->prefix[sizeof(info->prefix)-1] = '\0';
+	info->logflags = 0x0f;
+	info->level = 5;
+
+	nft_rule_expr_set(e, NFT_EXPR_TG_INFO, info, sizeof(*info));
+}
+
+static void add_expr_target(struct nft_rule *r)
+{
+	struct nft_rule_expr *expr;
+
+	expr = nft_rule_expr_alloc("target");
+	if (expr == NULL)
+		return;
+
+	add_target_log(expr);
+
+	nft_rule_add_expr(r, expr);
+}
+
+static void add_match_iprange(struct nft_rule_expr *e)
+{
+	struct xt_iprange_mtinfo *info;
+
+	nft_rule_expr_set(e, NFT_EXPR_MT_NAME, "iprange", strlen("iprange"));
+	nft_rule_expr_set_u32(e, NFT_EXPR_MT_REV, 1);
+
+	info = calloc(1, sizeof(struct xt_iprange_mtinfo));
+	if (info == NULL)
+		return;
+
+	info->src_min.ip = info->dst_min.ip = inet_addr("127.0.0.1");
+	info->src_max.ip = info->dst_max.ip = inet_addr("127.0.0.1");
+	info->flags = IPRANGE_SRC;
+
+	nft_rule_expr_set(e, NFT_EXPR_MT_INFO, info, sizeof(*info));
+}
+
+static void add_expr_match(struct nft_rule *r)
+{
+	struct nft_rule_expr *expr;
+
+	expr = nft_rule_expr_alloc("match");
+	if (expr == NULL)
+		return;
+
+	add_match_iprange(expr);
+
+	nft_rule_add_expr(r, expr);
+}
+
+#define field_sizeof(t, f)	(sizeof(((t *)NULL)->f))
+
+static void add_payload2(struct nft_rule_expr *e)
+{
+	nft_rule_expr_set_u32(e, NFT_EXPR_PAYLOAD_BASE,
+			      NFT_PAYLOAD_NETWORK_HEADER);
+	nft_rule_expr_set_u32(e, NFT_EXPR_PAYLOAD_DREG, NFT_REG_1);
+	nft_rule_expr_set_u32(e, NFT_EXPR_PAYLOAD_OFFSET,
+			      offsetof(struct iphdr, protocol));
+	nft_rule_expr_set_u32(e, NFT_EXPR_PAYLOAD_LEN, 1);
+}
+
+static void add_payload(struct nft_rule *r)
+{
+	struct nft_rule_expr *expr;
+
+	expr = nft_rule_expr_alloc("payload");
+	if (expr == NULL)
+		return;
+
+	add_payload2(expr);
+
+	nft_rule_add_expr(r, expr);
+}
+
+int main(int argc, char *argv[])
+{
+	struct mnl_socket *nl;
+	char buf[MNL_SOCKET_BUFFER_SIZE];
+	struct nlmsghdr *nlh;
+	uint32_t portid, seq;
+	struct nft_rule *r = NULL;
+	int ret, family;
+	uint64_t handle;
+
+	if (argc != 5) {
+		fprintf(stderr, "Usage: %s <family> <table> <chain> <handle>\n",
+			argv[0]);
+		exit(EXIT_FAILURE);
+	}
+
+	r = nft_rule_alloc();
+	if (r == NULL) {
+		perror("OOM");
+		exit(EXIT_FAILURE);
+	}
+
+	if (strcmp(argv[1], "ip") == 0)
+		family = AF_INET;
+	else if (strcmp(argv[1], "ip6") == 0)
+		family = AF_INET6;
+	else if (strcmp(argv[1], "bridge") == 0)
+		family = AF_BRIDGE;
+	else {
+		fprintf(stderr, "Unknown family: ip, ip6, bridge\n");
+		exit(EXIT_FAILURE);
+	}
+
+	nft_rule_attr_set(r, NFT_RULE_ATTR_TABLE, argv[2]);
+	nft_rule_attr_set(r, NFT_RULE_ATTR_CHAIN, argv[3]);
+
+	handle = atoi(argv[4]);
+	nft_rule_attr_set(r, NFT_RULE_ATTR_HANDLE, &handle);
+
+	add_expr_match(r);
+	add_payload(r);
+	add_expr_target(r);
+
+	char tmp[1024];
+	nft_rule_snprintf(tmp, sizeof(tmp), r, 0, 0);
+	printf("%s\n", tmp);
+
+	seq = time(NULL);
+	nlh = nft_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, family,
+					NLM_F_ACK|NLM_F_CREATE,
+					seq);
+	nft_rule_nlmsg_build_payload(nlh, r);
+	nft_rule_free(r);
+
+	nl = mnl_socket_open(NETLINK_NETFILTER);
+	if (nl == NULL) {
+		perror("mnl_socket_open");
+		exit(EXIT_FAILURE);
+	}
+
+	if (mnl_socket_bind(nl, 0, MNL_SOCKET_AUTOPID) < 0) {
+		perror("mnl_socket_bind");
+		exit(EXIT_FAILURE);
+	}
+	portid = mnl_socket_get_portid(nl);
+
+	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
+		perror("mnl_socket_send");
+		exit(EXIT_FAILURE);
+	}
+
+	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+	while (ret > 0) {
+		ret = mnl_cb_run(buf, ret, seq, portid, NULL, NULL);
+		if (ret <= 0)
+			break;
+		ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+	}
+	if (ret == -1) {
+		perror("error");
+		exit(EXIT_FAILURE);
+	}
+	mnl_socket_close(nl);
+
+	return EXIT_SUCCESS;
+}
-- 
1.8.3.1

Re: [RFC PATCH 0/1] add insert after to nf_tables

[Tomasz Bursztyka]

Hi Eric,
> 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.

Here it's a one shot usage: only on creating you need this info, so it 
would be also provided on notification. Not on the dump obviously.
I don't see a direct solution here, since adding an attribute (or a new 
flag to NFTA_RULE_FLAGS) would be going against current API design.

We have to sort this out.

Tomasz

Re: [RFC PATCH 0/1] add insert after to nf_tables

[Pablo Neira Ayuso]

On Wed, Jun 19, 2013 at 12:47:18PM +0300, Tomasz Bursztyka wrote:
> Hi Eric,
>
> >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.

This can be implemented in user-space.

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.

Re: [RFC PATCH 0/1] add insert after to nf_tables

[Tomasz Bursztyka]

Hi Pablo,

> 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.

Tomasz

Re: [RFC PATCH 0/1] add insert after to nf_tables

[Pablo Neira Ayuso]

On Thu, Jun 20, 2013 at 12:52:24PM +0300, Tomasz Bursztyka wrote:
> Hi Pablo,
> 
> >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.

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

Eric, would you rework the patch to do so?

Re: [RFC PATCH 0/1] add insert after to nf_tables

[Tomasz Bursztyka]

Hi Pablo,

>> 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.


Tomasz

Re: [RFC PATCH 0/1] add insert after to nf_tables

[Patrick McHardy]

On Thu, Jun 20, 2013 at 01:36:00PM +0300, Tomasz Bursztyka wrote:
> Hi Pablo,
> 
> >>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.

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).

Re: [RFC PATCH 0/1] add insert after to nf_tables

[Tomasz Bursztyka]

Hi Patrick,
>> >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).

I like that, it follows the current API design logic and it's simpler to 
implement in both sides.
It's much simpler indeed!

Tomasz

Re: [RFC PATCH 0/1] add insert after to nf_tables

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

Hi,

Le jeudi 20 juin 2013 à 12:46 +0200, Patrick McHardy a écrit :
> On Thu, Jun 20, 2013 at 01:36:00PM +0300, Tomasz Bursztyka wrote:
> > Hi Pablo,
> > 
> > >>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)
> > 
> > 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.

I like that. Reusing NLA_RULE_HANDLE was making feel somehow
uncomfortable.

>  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).

Nice, reworking my patches in that direction.

BR,
--
Eric

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[RFC PATCHv2] netfilter: nf_tables: add insert operation

[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 <eric@regit.org>
---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 32 +++++++++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index d40a7f9..d9bf8ea 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -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)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b00aca8..a03aa9f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -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,
@@ -1298,6 +1299,17 @@ 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;
 
+	if (event & NLM_F_APPEND && rule->list.prev) {
+		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);
-- 
1.8.3.1

Re: [RFC PATCHv2] netfilter: nf_tables: add insert operation

[Pablo Neira Ayuso]

Hi Eric,

On Fri, Jun 28, 2013 at 11:05:18PM +0200, Eric Leblond wrote:
> 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 <eric@regit.org>
> ---
>  include/uapi/linux/netfilter/nf_tables.h |  1 +
>  net/netfilter/nf_tables_api.c            | 32 +++++++++++++++++++++++++++++---
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index d40a7f9..d9bf8ea 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -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)
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index b00aca8..a03aa9f 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -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,
> @@ -1298,6 +1299,17 @@ 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;
>  
> +	if (event & NLM_F_APPEND && rule->list.prev) {

.prev is always != NULL.

It points to the list head in the first element case. It is set to
LIST_POISON2 in case that rule has been deleted.

> +		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);
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 nftables insert operation]

[Eric Leblond]

Hello,

Here's a rework of previous patch. Kernel patch, libnftables
and nftables patches are to follow.

libnftables patchset statistics:
 examples/Makefile.am                |   4 +
 examples/nft-rule-insert.c          | 204 ++++++++++++++++++++++++++++++++++++
 include/libnftables/rule.h          |   3 +-
 include/linux/netfilter/nf_tables.h |   1 +
 src/rule.c                          |  29 ++++-
 5 files changed, 237 insertions(+), 4 deletions(-)

nftables patchset statistics:
 include/rule.h            |  2 ++
 src/netlink.c             |  2 ++
 src/netlink_delinearize.c |  2 ++
 src/parser.y              | 22 ++++++++++++++++++++--
 src/rule.c                |  2 ++
 src/scanner.l             |  3 +++
 6 files changed, 31 insertions(+), 2 deletions(-)

BR,
--
Eric

[PATCH] netfilter: nf_tables: add insert operation

[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 <eric@regit.org>
---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 47 +++++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index d40a7f9..d9bf8ea 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -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)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b00aca8..ce8a4f0 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -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;
 
+	if (event & NLM_F_APPEND) {
+		if ((rule->list.prev != LIST_POISON2) &&
+		    (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;
+		}
+	} else {
+		if ((rule->list.next != LIST_POISON1) &&
+		    (rule->list.next != &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,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;
@@ -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);
-- 
1.8.3.2

Re: [PATCH] netfilter: nf_tables: add insert operation

[Pablo Neira Ayuso]

Hi Eric,

Thanks for this new round, some comments below:

On Sat, Jul 06, 2013 at 05:31:17PM +0200, Eric Leblond wrote:
> 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 <eric@regit.org>
> ---
>  include/uapi/linux/netfilter/nf_tables.h |  1 +
>  net/netfilter/nf_tables_api.c            | 47 +++++++++++++++++++++++++++++---
>  2 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index d40a7f9..d9bf8ea 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -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)
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index b00aca8..ce8a4f0 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -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;
>  
> +	if (event & NLM_F_APPEND) {

This should be if (flags & NLM_F_APPEND)

> +		if ((rule->list.prev != LIST_POISON2) &&

You can use event instead to catch the deletion case:

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;
> +		}
> +	} else {
> +		if ((rule->list.next != LIST_POISON1) &&
> +		    (rule->list.next != &chain->rules)) {

You can use list_is_last instead of rule->list.next != &chain->rules.

Unfortunately, there is no list_is_first, but we could ask for
mainstream inclusion later on.

> +			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);
>  	}
>  
> +	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;

Better this checking before the rule lookup.

> +	}
> +
>  	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);
> -- 
> 1.8.3.2
> 

[PATCHv4 nftables insert operation 0/1]

[Eric Leblond]

Hello Pablo,

Thanks for the review, here's an update version of the kernel
patch. A nftables patch on top of the one I've already provide
is also mandatory to have the insert operation working correctly.

BR,
--
Eric

[PATCHv4] netfilter: nf_tables: add insert operation

[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 <eric@regit.org>
---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index d40a7f9..d9bf8ea 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -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)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b00aca8..012eb1f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -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;
 
+	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;
+		}
+	} 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);
-- 
1.8.3.2

Re: [PATCHv4] netfilter: nf_tables: add insert operation

[Pablo Neira Ayuso]

Hi Eric,

On Tue, Jul 09, 2013 at 12:56:17AM +0200, Eric Leblond wrote:
> 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 <eric@regit.org>
> ---
>  include/uapi/linux/netfilter/nf_tables.h |  1 +
>  net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
>  2 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index d40a7f9..d9bf8ea 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -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)
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index b00aca8..012eb1f 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -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;
>  
> +	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.

> +	} 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);
> -- 
> 1.8.3.2
> 

Re: [PATCHv4] netfilter: nf_tables: add insert operation

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 6281 bytes --]

Hi,

Le lundi 15 juillet 2013 à 12:48 +0200, Pablo Neira Ayuso a écrit :
> Hi Eric,
> 
> On Tue, Jul 09, 2013 at 12:56:17AM +0200, Eric Leblond wrote:
> > 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 <eric@regit.org>
> > ---
> >  include/uapi/linux/netfilter/nf_tables.h |  1 +
> >  net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
> >  2 files changed, 43 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> > index d40a7f9..d9bf8ea 100644
> > --- a/include/uapi/linux/netfilter/nf_tables.h
> > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > @@ -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)
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index b00aca8..012eb1f 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -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;
> >  
> > +	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.

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).

BR,

> 
> > +	} 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);
> > -- 
> > 1.8.3.2
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCHv4] netfilter: nf_tables: add insert operation

[Pablo Neira Ayuso]

Hi Eric,

On Mon, Jul 15, 2013 at 07:27:54PM +0200, Eric Leblond wrote:
> Hi,
> 
> Le lundi 15 juillet 2013 à 12:48 +0200, Pablo Neira Ayuso a écrit :
> > Hi Eric,
> > 
> > On Tue, Jul 09, 2013 at 12:56:17AM +0200, Eric Leblond wrote:
> > > 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 <eric@regit.org>
> > > ---
> > >  include/uapi/linux/netfilter/nf_tables.h |  1 +
> > >  net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
> > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> > > index d40a7f9..d9bf8ea 100644
> > > --- a/include/uapi/linux/netfilter/nf_tables.h
> > > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > > @@ -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)
> > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > index b00aca8..012eb1f 100644
> > > --- a/net/netfilter/nf_tables_api.c
> > > +++ b/net/netfilter/nf_tables_api.c
> > > @@ -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;
> > >  
> > > +	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.
> 
> 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).

> 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).

> 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.

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

Re: [PATCHv4] netfilter: nf_tables: add insert operation

[Eric Leblond]

[-- Attachment #1.1: Type: text/plain, Size: 5828 bytes --]

Hello Pablo,

Le mardi 16 juillet 2013 à 01:57 +0200, Pablo Neira Ayuso a écrit :
> Hi Eric,
> 
> On Mon, Jul 15, 2013 at 07:27:54PM +0200, Eric Leblond wrote:
> > Hi,
> > 
> > Le lundi 15 juillet 2013 à 12:48 +0200, Pablo Neira Ayuso a écrit :
> > > Hi Eric,
> > > 
> > > On Tue, Jul 09, 2013 at 12:56:17AM +0200, Eric Leblond wrote:
> > > > 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 <eric@regit.org>
> > > > ---
> > > >  include/uapi/linux/netfilter/nf_tables.h |  1 +
> > > >  net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
> > > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> > > > index d40a7f9..d9bf8ea 100644
> > > > --- a/include/uapi/linux/netfilter/nf_tables.h
> > > > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > > > @@ -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)
> > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > > index b00aca8..012eb1f 100644
> > > > --- a/net/netfilter/nf_tables_api.c
> > > > +++ b/net/netfilter/nf_tables_api.c
> > > > @@ -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;
> > > >  
> > > > +	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.
> > 
> > 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).


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.

So events get insert/append information as well as position. This should
be enough for event listeners to follow ruleset evolution.

> > 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).

Yes, getting insert/append information and position is better.

> > 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.

Cool :)

BR,
--
Eric

[-- Attachment #1.2: 0001-after-before.patch --]
[-- Type: text/x-patch, Size: 803 bytes --]

From 86e6e09ebbe2b770547034c2cb6a2239f79f1ce4 Mon Sep 17 00:00:00 2001
From: Eric Leblond <eric@regit.org>
Date: Tue, 16 Jul 2013 09:12:19 +0200
Subject: [PATCH] after before

Signed-off-by: Eric Leblond <eric@regit.org>
---
 examples/nft-events.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/examples/nft-events.c b/examples/nft-events.c
index 5550c70..5bbdce4 100644
--- a/examples/nft-events.c
+++ b/examples/nft-events.c
@@ -63,6 +63,10 @@ static int rule_cb(const struct nlmsghdr *nlh, int type)
 		goto err_free;
 	}
 
+	if (nlh->nlmsg_flags & NLM_F_APPEND)
+		printf("AFTER ");
+	else
+		printf("BEFORE ");
 	nft_rule_snprintf(buf, sizeof(buf), t, NFT_RULE_O_DEFAULT, 0);
 	printf("[%s]\t%s\n", type == NFT_MSG_NEWRULE ? "NEW" : "DEL", buf);
 
-- 
1.8.3.2


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCHv4] netfilter: nf_tables: add insert operation

[Pablo Neira Ayuso]

On Tue, Jul 16, 2013 at 09:35:58AM +0200, Eric Leblond wrote:
> Hello Pablo,
> 
> Le mardi 16 juillet 2013 à 01:57 +0200, Pablo Neira Ayuso a écrit :
> > Hi Eric,
> > 
> > On Mon, Jul 15, 2013 at 07:27:54PM +0200, Eric Leblond wrote:
> > > Hi,
> > > 
> > > Le lundi 15 juillet 2013 à 12:48 +0200, Pablo Neira Ayuso a écrit :
> > > > Hi Eric,
> > > > 
> > > > On Tue, Jul 09, 2013 at 12:56:17AM +0200, Eric Leblond wrote:
> > > > > 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 <eric@regit.org>
> > > > > ---
> > > > >  include/uapi/linux/netfilter/nf_tables.h |  1 +
> > > > >  net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
> > > > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> > > > > index d40a7f9..d9bf8ea 100644
> > > > > --- a/include/uapi/linux/netfilter/nf_tables.h
> > > > > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > > > > @@ -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)
> > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > > > index b00aca8..012eb1f 100644
> > > > > --- a/net/netfilter/nf_tables_api.c
> > > > > +++ b/net/netfilter/nf_tables_api.c
> > > > > @@ -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;
> > > > >  
> > > > > +	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.
> > > 
> > > 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).
> 
> 
> 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.

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.

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

Re: [PATCHv4] netfilter: nf_tables: add insert operation

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 5953 bytes --]

Hi,

Le mardi 16 juillet 2013 à 12:00 +0200, Pablo Neira Ayuso a écrit :
> On Tue, Jul 16, 2013 at 09:35:58AM +0200, Eric Leblond wrote:
> > Hello Pablo,
> > 
> > Le mardi 16 juillet 2013 à 01:57 +0200, Pablo Neira Ayuso a écrit :
> > > Hi Eric,
> > > 
> > > On Mon, Jul 15, 2013 at 07:27:54PM +0200, Eric Leblond wrote:
> > > > Hi,
> > > > 
> > > > Le lundi 15 juillet 2013 à 12:48 +0200, Pablo Neira Ayuso a écrit :
> > > > > Hi Eric,
> > > > > 
> > > > > On Tue, Jul 09, 2013 at 12:56:17AM +0200, Eric Leblond wrote:
> > > > > > 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 <eric@regit.org>
> > > > > > ---
> > > > > >  include/uapi/linux/netfilter/nf_tables.h |  1 +
> > > > > >  net/netfilter/nf_tables_api.c            | 46 +++++++++++++++++++++++++++++---
> > > > > >  2 files changed, 43 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> > > > > > index d40a7f9..d9bf8ea 100644
> > > > > > --- a/include/uapi/linux/netfilter/nf_tables.h
> > > > > > +++ b/include/uapi/linux/netfilter/nf_tables.h
> > > > > > @@ -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)
> > > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > > > > index b00aca8..012eb1f 100644
> > > > > > --- a/net/netfilter/nf_tables_api.c
> > > > > > +++ b/net/netfilter/nf_tables_api.c
> > > > > > @@ -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;
> > > > > >  
> > > > > > +	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.
> > > > 
> > > > 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).
> > 
> > 
> > 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.
> 
> 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.

Thanks a lot for the clarification. I'll update the patch.

BR,
--
Eric


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[PATCHv5] netfilter: nf_tables: add insert operation

[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.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 33 ++++++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index d40a7f9..d9bf8ea 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -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)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b00aca8..469818f 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -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,13 @@ 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;
 
+	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;
+	}
+
 	list = nla_nest_start(skb, NFTA_RULE_EXPRESSIONS);
 	if (list == NULL)
 		goto nla_put_failure;
@@ -1537,7 +1546,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 +1580,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 +1643,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);
-- 
1.8.3.2

Re: [PATCHv5] netfilter: nf_tables: add insert operation

[Pablo Neira Ayuso]

On Fri, Jul 19, 2013 at 09:45:46AM +0200, Eric Leblond wrote:
> 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.

Applied, thanks a lot Eric!

[nftables PATCH] rule: honor flag argument during rule creation

[Eric Leblond]

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

diff --git a/src/mnl.c b/src/mnl.c
index ea9637c..71fe73b 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -61,7 +61,7 @@ int mnl_nft_rule_add(struct mnl_socket *nf_sock, struct nft_rule *nlr,
 
 	nlh = nft_table_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE,
 			nft_rule_attr_get_u32(nlr, NFT_RULE_ATTR_FAMILY),
-			NLM_F_APPEND|NLM_F_ACK|NLM_F_CREATE, seq);
+			flags|NLM_F_ACK|NLM_F_CREATE, seq);
 	nft_rule_nlmsg_build_payload(nlh, nlr);
 
 	return mnl_talk(nf_sock, nlh, nlh->nlmsg_len, NULL, NULL);
-- 
1.8.3.2

[libnftables PATCH 1/4] rule: add support for position attribute

[Eric Leblond]

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

Signed-off-by: Eric Leblond <eric@regit.org>
---
 include/libnftables/rule.h          |  1 +
 include/linux/netfilter/nf_tables.h |  1 +
 src/rule.c                          | 23 +++++++++++++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/include/libnftables/rule.h b/include/libnftables/rule.h
index 186c82c..ab61eb8 100644
--- a/include/libnftables/rule.h
+++ b/include/libnftables/rule.h
@@ -22,6 +22,7 @@ enum {
 	NFT_RULE_ATTR_FLAGS,
 	NFT_RULE_ATTR_COMPAT_PROTO,
 	NFT_RULE_ATTR_COMPAT_FLAGS,
+	NFT_RULE_ATTR_POSITION,
 };
 
 void nft_rule_attr_unset(struct nft_rule *r, uint16_t attr);
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index c2dae4e..4fe91ef 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -99,6 +99,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)
diff --git a/src/rule.c b/src/rule.c
index 5a4ae91..a56df34 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -39,6 +39,7 @@ struct nft_rule {
 	uint8_t		family;
 	uint32_t	rule_flags;
 	uint64_t	handle;
+	uint64_t	position;
 	struct {
 			uint32_t	flags;
 			uint32_t	proto;
@@ -99,6 +100,7 @@ void nft_rule_attr_unset(struct nft_rule *r, uint16_t attr)
 	case NFT_RULE_ATTR_FLAGS:
 	case NFT_RULE_ATTR_COMPAT_PROTO:
 	case NFT_RULE_ATTR_COMPAT_FLAGS:
+	case NFT_RULE_ATTR_POSITION:
 	case NFT_RULE_ATTR_FAMILY:
 		break;
 	default:
@@ -127,6 +129,9 @@ void nft_rule_attr_set(struct nft_rule *r, uint16_t attr, const void *data)
 	case NFT_RULE_ATTR_HANDLE:
 		r->handle = *((uint64_t *)data);
 		break;
+	case NFT_RULE_ATTR_POSITION:
+		r->position = *((uint64_t *)data);
+		break;
 	case NFT_RULE_ATTR_FLAGS:
 		r->rule_flags = *((uint32_t *)data);
 		break;
@@ -208,6 +213,12 @@ const void *nft_rule_attr_get(const struct nft_rule *r, uint16_t attr)
 		else
 			return NULL;
 		break;
+	case NFT_RULE_ATTR_POSITION:
+		if (r->flags & (1 << NFT_RULE_ATTR_POSITION))
+			return &r->position;
+		else
+			return NULL;
+		break;
 	default:
 		return NULL;
 	}
@@ -273,6 +284,8 @@ void nft_rule_nlmsg_build_payload(struct nlmsghdr *nlh, struct nft_rule *r)
 		mnl_attr_put_strz(nlh, NFTA_RULE_CHAIN, r->chain);
 	if (r->flags & (1 << NFT_RULE_ATTR_HANDLE))
 		mnl_attr_put_u64(nlh, NFTA_RULE_HANDLE, htobe64(r->handle));
+	if (r->flags & (1 << NFT_RULE_ATTR_POSITION))
+		mnl_attr_put_u64(nlh, NFTA_RULE_POSITION, htobe64(r->position));
 	if (r->flags & (1 << NFT_RULE_ATTR_FLAGS))
 		mnl_attr_put_u32(nlh, NFTA_RULE_FLAGS, htonl(r->rule_flags));
 
@@ -335,6 +348,12 @@ static int nft_rule_parse_attr_cb(const struct nlattr *attr, void *data)
 			return MNL_CB_ERROR;
 		}
 		break;
+	case NFTA_RULE_POSITION:
+		if (mnl_attr_validate(attr, MNL_TYPE_U64) < 0) {
+			perror("mnl_attr_validate");
+			return MNL_CB_ERROR;
+		}
+		break;
 	}
 
 	tb[type] = attr;
@@ -469,6 +488,10 @@ int nft_rule_nlmsg_parse(const struct nlmsghdr *nlh, struct nft_rule *r)
 		ret = nft_rule_parse_expr(tb[NFTA_RULE_EXPRESSIONS], r);
 	if (tb[NFTA_RULE_COMPAT])
 		ret = nft_rule_parse_compat(tb[NFTA_RULE_COMPAT], r);
+	if (tb[NFTA_RULE_POSITION]) {
+		r->position = be64toh(mnl_attr_get_u64(tb[NFTA_RULE_POSITION]));
+		r->flags |= (1 << NFT_RULE_ATTR_POSITION);
+	}
 
 	r->family = nfg->nfgen_family;
 	r->flags |= (1 << NFT_RULE_ATTR_FAMILY);
-- 
1.8.3.2

[libnftables PATCH 2/4] examples: add insert rule example

[Eric Leblond]

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

Signed-off-by: Eric Leblond <eric@regit.org>
---
 examples/Makefile.am       |   4 +
 examples/nft-rule-insert.c | 204 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 208 insertions(+)
 create mode 100644 examples/nft-rule-insert.c

diff --git a/examples/Makefile.am b/examples/Makefile.am
index dcf798a..1d6b98f 100644
--- a/examples/Makefile.am
+++ b/examples/Makefile.am
@@ -10,6 +10,7 @@ check_PROGRAMS = nft-table-add		\
 		 nft-chain-del		\
 		 nft-chain-get		\
 		 nft-rule-add		\
+		 nft-rule-insert	\
 		 nft-rule-xml-add	\
 		 nft-rule-del		\
 		 nft-rule-get		\
@@ -52,6 +53,9 @@ nft_chain_get_LDADD = ../src/libnftables.la ${LIBMNL_LIBS} ${LIBXML_LIBS}
 nft_rule_add_SOURCES = nft-rule-add.c
 nft_rule_add_LDADD = ../src/libnftables.la ${LIBMNL_LIBS} ${LIBXML_LIBS}
 
+nft_rule_insert_SOURCES = nft-rule-insert.c
+nft_rule_insert_LDADD = ../src/libnftables.la ${LIBMNL_LIBS} ${LIBXML_LIBS}
+
 nft_rule_xml_add_SOURCES = nft-rule-xml-add.c
 nft_rule_xml_add_LDADD = ../src/libnftables.la ${LIBMNL_LIBS} ${LIBXML_LIBS}
 
diff --git a/examples/nft-rule-insert.c b/examples/nft-rule-insert.c
new file mode 100644
index 0000000..1418127
--- /dev/null
+++ b/examples/nft-rule-insert.c
@@ -0,0 +1,204 @@
+/*
+ * (C) 2012 by Pablo Neira Ayuso <pablo@netfilter.org>
+ * (C) 2013 by Eric Leblond <eric@regit.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This software has been sponsored by Sophos Astaro <http://www.sophos.com>
+ */
+
+#include <stdlib.h>
+#include <time.h>
+#include <string.h>
+#include <stddef.h>	/* for offsetof */
+#include <netinet/in.h>
+#include <arpa/inet.h>
+
+#include <linux/netfilter/nf_tables.h>
+
+#include <libmnl/libmnl.h>
+#include <libnftables/rule.h>
+#include <libnftables/expr.h>
+
+#include <linux/netfilter_ipv4/ipt_LOG.h>
+#include <linux/netfilter/xt_iprange.h>
+
+#include <netinet/ip.h>
+
+static void add_target_log(struct nft_rule_expr *e)
+{
+	struct ipt_log_info *info;
+
+	nft_rule_expr_set(e, NFT_EXPR_TG_NAME, "LOG", strlen("LOG"));
+	nft_rule_expr_set_u32(e, NFT_EXPR_TG_REV, 0);
+
+	info = calloc(1, sizeof(struct ipt_log_info));
+	if (info == NULL)
+		return;
+
+	sprintf(info->prefix, "test: ");
+	info->prefix[sizeof(info->prefix)-1] = '\0';
+	info->logflags = 0x0f;
+	info->level = 5;
+
+	nft_rule_expr_set(e, NFT_EXPR_TG_INFO, info, sizeof(*info));
+}
+
+static void add_expr_target(struct nft_rule *r)
+{
+	struct nft_rule_expr *expr;
+
+	expr = nft_rule_expr_alloc("target");
+	if (expr == NULL)
+		return;
+
+	add_target_log(expr);
+
+	nft_rule_add_expr(r, expr);
+}
+
+static void add_match_iprange(struct nft_rule_expr *e)
+{
+	struct xt_iprange_mtinfo *info;
+
+	nft_rule_expr_set(e, NFT_EXPR_MT_NAME, "iprange", strlen("iprange"));
+	nft_rule_expr_set_u32(e, NFT_EXPR_MT_REV, 1);
+
+	info = calloc(1, sizeof(struct xt_iprange_mtinfo));
+	if (info == NULL)
+		return;
+
+	info->src_min.ip = info->dst_min.ip = inet_addr("127.0.0.1");
+	info->src_max.ip = info->dst_max.ip = inet_addr("127.0.0.1");
+	info->flags = IPRANGE_SRC;
+
+	nft_rule_expr_set(e, NFT_EXPR_MT_INFO, info, sizeof(*info));
+}
+
+static void add_expr_match(struct nft_rule *r)
+{
+	struct nft_rule_expr *expr;
+
+	expr = nft_rule_expr_alloc("match");
+	if (expr == NULL)
+		return;
+
+	add_match_iprange(expr);
+
+	nft_rule_add_expr(r, expr);
+}
+
+#define field_sizeof(t, f)	(sizeof(((t *)NULL)->f))
+
+static void add_payload2(struct nft_rule_expr *e)
+{
+	nft_rule_expr_set_u32(e, NFT_EXPR_PAYLOAD_BASE,
+			      NFT_PAYLOAD_NETWORK_HEADER);
+	nft_rule_expr_set_u32(e, NFT_EXPR_PAYLOAD_DREG, NFT_REG_1);
+	nft_rule_expr_set_u32(e, NFT_EXPR_PAYLOAD_OFFSET,
+			      offsetof(struct iphdr, protocol));
+	nft_rule_expr_set_u32(e, NFT_EXPR_PAYLOAD_LEN, 1);
+}
+
+static void add_payload(struct nft_rule *r)
+{
+	struct nft_rule_expr *expr;
+
+	expr = nft_rule_expr_alloc("payload");
+	if (expr == NULL)
+		return;
+
+	add_payload2(expr);
+
+	nft_rule_add_expr(r, expr);
+}
+
+int main(int argc, char *argv[])
+{
+	struct mnl_socket *nl;
+	char buf[MNL_SOCKET_BUFFER_SIZE];
+	struct nlmsghdr *nlh;
+	uint32_t portid, seq;
+	struct nft_rule *r = NULL;
+	int ret, family;
+	uint64_t handle;
+
+	if (argc != 5) {
+		fprintf(stderr, "Usage: %s <family> <table> <chain> <handle>\n",
+			argv[0]);
+		exit(EXIT_FAILURE);
+	}
+
+	r = nft_rule_alloc();
+	if (r == NULL) {
+		perror("OOM");
+		exit(EXIT_FAILURE);
+	}
+
+	if (strcmp(argv[1], "ip") == 0)
+		family = AF_INET;
+	else if (strcmp(argv[1], "ip6") == 0)
+		family = AF_INET6;
+	else if (strcmp(argv[1], "bridge") == 0)
+		family = AF_BRIDGE;
+	else {
+		fprintf(stderr, "Unknown family: ip, ip6, bridge\n");
+		exit(EXIT_FAILURE);
+	}
+
+	nft_rule_attr_set(r, NFT_RULE_ATTR_TABLE, argv[2]);
+	nft_rule_attr_set(r, NFT_RULE_ATTR_CHAIN, argv[3]);
+
+	handle = atoi(argv[4]);
+	nft_rule_attr_set(r, NFT_RULE_ATTR_POSITION, &handle);
+
+	add_expr_match(r);
+	add_payload(r);
+	add_expr_target(r);
+
+	char tmp[1024];
+	nft_rule_snprintf(tmp, sizeof(tmp), r, 0, 0);
+	printf("%s\n", tmp);
+
+	seq = time(NULL);
+	nlh = nft_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, family,
+					NLM_F_ACK|NLM_F_CREATE,
+					seq);
+	nft_rule_nlmsg_build_payload(nlh, r);
+	nft_rule_free(r);
+
+	nl = mnl_socket_open(NETLINK_NETFILTER);
+	if (nl == NULL) {
+		perror("mnl_socket_open");
+		exit(EXIT_FAILURE);
+	}
+
+	if (mnl_socket_bind(nl, 0, MNL_SOCKET_AUTOPID) < 0) {
+		perror("mnl_socket_bind");
+		exit(EXIT_FAILURE);
+	}
+	portid = mnl_socket_get_portid(nl);
+
+	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
+		perror("mnl_socket_send");
+		exit(EXIT_FAILURE);
+	}
+
+	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+	while (ret > 0) {
+		ret = mnl_cb_run(buf, ret, seq, portid, NULL, NULL);
+		if (ret <= 0)
+			break;
+		ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+	}
+	if (ret == -1) {
+		perror("error");
+		exit(EXIT_FAILURE);
+	}
+	mnl_socket_close(nl);
+
+	return EXIT_SUCCESS;
+}
-- 
1.8.3.2

Re: [libnftables PATCH 2/4] examples: add insert rule example

[Pablo Neira Ayuso]

On Sat, Jul 06, 2013 at 05:33:14PM +0200, Eric Leblond wrote:
> This program can insert a rule after a rule given by
> its handle.

Applied, thanks Eric.

[libnftables PATCH 3/4] rule: display position in default printf

[Eric Leblond]

Signed-off-by: Eric Leblond <eric@regit.org>
---
 src/rule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index a56df34..9dc82b8 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -769,9 +769,9 @@ static int nft_rule_snprintf_default(char *buf, size_t size, struct nft_rule *r,
 	struct nft_rule_expr *expr;
 	int ret, len = size, offset = 0;
 
-	ret = snprintf(buf, size, "%s %s %s %"PRIu64"\n",
+	ret = snprintf(buf, size, "%s %s %s %"PRIu64" %"PRIu64"\n",
 			nft_family2str(r->family), r->table, r->chain,
-			r->handle);
+			r->handle, r->position);
 	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
 	list_for_each_entry(expr, &r->expr_list, head) {
-- 
1.8.3.2

Re: [libnftables PATCH 3/4] rule: display position in default printf

[Pablo Neira Ayuso]

Also applied, thanks.

[libnftables PATCH 4/4] rule: change type of function to use const

[Eric Leblond]

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

Signed-off-by: Eric Leblond <eric@regit.org>
---
 include/libnftables/rule.h | 2 +-
 src/rule.c                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/libnftables/rule.h b/include/libnftables/rule.h
index ab61eb8..5496561 100644
--- a/include/libnftables/rule.h
+++ b/include/libnftables/rule.h
@@ -26,7 +26,7 @@ enum {
 };
 
 void nft_rule_attr_unset(struct nft_rule *r, uint16_t attr);
-bool nft_rule_attr_is_set(struct nft_rule *r, uint16_t attr);
+bool nft_rule_attr_is_set(const struct nft_rule *r, uint16_t attr);
 void nft_rule_attr_set(struct nft_rule *r, uint16_t attr, const void *data);
 void nft_rule_attr_set_u32(struct nft_rule *r, uint16_t attr, uint32_t val);
 void nft_rule_attr_set_u64(struct nft_rule *r, uint16_t attr, uint64_t val);
diff --git a/src/rule.c b/src/rule.c
index 9dc82b8..dc98a8f 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -73,7 +73,7 @@ void nft_rule_free(struct nft_rule *r)
 }
 EXPORT_SYMBOL(nft_rule_free);
 
-bool nft_rule_attr_is_set(struct nft_rule *r, uint16_t attr)
+bool nft_rule_attr_is_set(const struct nft_rule *r, uint16_t attr)
 {
 	return r->flags & (1 << attr);
 }
-- 
1.8.3.2

Re: [libnftables PATCH 4/4] rule: change type of function to use const

[Pablo Neira Ayuso]

On Sat, Jul 06, 2013 at 05:33:16PM +0200, Eric Leblond wrote:
> The function nft_rule_attr_is_set() is doing no modification so it
> is possible to type it to const.

I have mangled this patch to adapt all other nft_*_attr_is_set as
well. This patch is master now, thanks.

Re: [libnftables PATCH 1/4] rule: add support for position attribute

[Pablo Neira Ayuso]

On Sat, Jul 06, 2013 at 05:33:13PM +0200, Eric Leblond wrote:
> This patch adds support for position attribute which can be used
> to insert a rule at a given position.

Applied, thanks.

[nftables PATCH] Add support for insertion inside rule list

[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

Signed-off-by: Eric Leblond <eric@regit.org>
---
 include/rule.h            |  2 ++
 src/netlink.c             |  2 ++
 src/netlink_delinearize.c |  2 ++
 src/parser.y              | 22 ++++++++++++++++++++--
 src/rule.c                |  2 ++
 src/scanner.l             |  3 +++
 6 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index e0debe3..2577cff 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -13,6 +13,7 @@
  * @chain:	chain name (chains and rules only)
  * @set:	set name (sets only)
  * @handle:	rule handle (rules only)
+ * @position:	rule position (rules only)
  */
 struct handle {
 	uint32_t		family;
@@ -20,6 +21,7 @@ struct handle {
 	const char		*chain;
 	const char		*set;
 	uint64_t		handle;
+	uint64_t		position;
 };
 
 extern void handle_merge(struct handle *dst, const struct handle *src);
diff --git a/src/netlink.c b/src/netlink.c
index 2a7bdb5..5129cac 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -105,6 +105,8 @@ struct nft_rule *alloc_nft_rule(const struct handle *h)
 		nft_rule_attr_set_str(nlr, NFT_RULE_ATTR_CHAIN, h->chain);
 	if (h->handle)
 		nft_rule_attr_set_u64(nlr, NFT_RULE_ATTR_HANDLE, h->handle);
+	if (h->position)
+		nft_rule_attr_set_u64(nlr, NFT_RULE_ATTR_POSITION, h->position);
 	return nlr;
 }
 
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 9348913..f92e83f 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -796,6 +796,8 @@ struct rule *netlink_delinearize_rule(struct netlink_ctx *ctx,
 	h.table  = xstrdup(nft_rule_attr_get_str(nlr, NFT_RULE_ATTR_TABLE));
 	h.chain  = xstrdup(nft_rule_attr_get_str(nlr, NFT_RULE_ATTR_CHAIN));
 	h.handle = nft_rule_attr_get_u64(nlr, NFT_RULE_ATTR_HANDLE);
+	if (nft_rule_attr_is_set(nlr, NFT_RULE_ATTR_POSITION))
+		h.position = nft_rule_attr_get_u64(nlr, NFT_RULE_ATTR_POSITION);
 
 	pctx->rule = rule_alloc(&internal_location, &h);
 	pctx->table = table_lookup(&h);
diff --git a/src/parser.y b/src/parser.y
index 2923b59..d52bd97 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -326,6 +326,9 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %token SNAT			"snat"
 %token DNAT			"dnat"
 
+%token BEFORE			"before"
+%token AFTER			"after"
+
 %type <string>			identifier string
 %destructor { xfree($$); }	identifier string
 
@@ -339,7 +342,7 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %destructor { handle_free(&$$); } table_spec tables_spec chain_spec chain_identifier ruleid_spec
 %type <handle>			set_spec set_identifier
 %destructor { handle_free(&$$); } set_spec set_identifier
-%type <val>			handle_spec family_spec
+%type <val>			handle_spec family_spec position_spec
 
 %type <table>			table_block_alloc table_block
 %destructor { table_free($$); }	table_block_alloc
@@ -842,10 +845,25 @@ handle_spec		:	/* empty */
 			}
 			;
 
-ruleid_spec		:	chain_spec	handle_spec
+position_spec		:	/* empty */
+			{
+				$$ = 0;
+			}
+			|	BEFORE		NUM
+			{
+				$$ = $2;
+			}
+			|	AFTER		NUM
+			{
+				$$ = $2;
+			}
+			;
+
+ruleid_spec		:	chain_spec	handle_spec	position_spec
 			{
 				$$		= $1;
 				$$.handle	= $2;
+				$$.position	= $3;
 			}
 			;
 
diff --git a/src/rule.c b/src/rule.c
index 5a894cc..8368624 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -41,6 +41,8 @@ void handle_merge(struct handle *dst, const struct handle *src)
 		dst->set = xstrdup(src->set);
 	if (dst->handle == 0)
 		dst->handle = src->handle;
+	if (dst->position == 0)
+		dst->position = src->position;
 }
 
 struct set *set_alloc(const struct location *loc)
diff --git a/src/scanner.l b/src/scanner.l
index fe7b86c..519e1fc 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -249,6 +249,9 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "flush"			{ return FLUSH; }
 "rename"		{ return RENAME; }
 
+"before"		{ return BEFORE; }
+"after"			{ return AFTER; }
+
 "counter"		{ return COUNTER; }
 "packets"		{ return PACKETS; }
 "bytes"			{ return BYTES; }
-- 
1.8.3.2

Re: [nftables PATCH] Add support for insertion inside rule list

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

Hi Eric,

On Sat, Jul 06, 2013 at 05:33:57PM +0200, Eric Leblond wrote:
> 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.

Please, find the new patch attached. Thanks.

[-- Attachment #2: 0001-src-Add-support-for-insertion-inside-rule-list.patch --]
[-- Type: text/x-diff, Size: 5049 bytes --]

>From f8afcae4af949c1f8c71fc4dbbffdbddbedb7adf Mon Sep 17 00:00:00 2001
From: Eric Leblond <eric@regit.org>
Date: Sat, 6 Jul 2013 17:33:57 +0200
Subject: [PATCH] src: Add support for insertion inside rule list

This patch adds support to insert and to add rule using a rule
handle as reference. The rule handle syntax has an new optional
position field which take a handle as argument.

Two examples:

  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

Signed-off-by: Eric Leblond <eric@regit.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/rule.h            |    2 ++
 src/mnl.c                 |    2 +-
 src/netlink.c             |    2 ++
 src/netlink_delinearize.c |    2 ++
 src/parser.y              |   17 +++++++++++++++--
 src/rule.c                |    2 ++
 src/scanner.l             |    2 ++
 7 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index e0debe3..2577cff 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -13,6 +13,7 @@
  * @chain:	chain name (chains and rules only)
  * @set:	set name (sets only)
  * @handle:	rule handle (rules only)
+ * @position:	rule position (rules only)
  */
 struct handle {
 	uint32_t		family;
@@ -20,6 +21,7 @@ struct handle {
 	const char		*chain;
 	const char		*set;
 	uint64_t		handle;
+	uint64_t		position;
 };
 
 extern void handle_merge(struct handle *dst, const struct handle *src);
diff --git a/src/mnl.c b/src/mnl.c
index a58f7ea..928d692 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -61,7 +61,7 @@ int mnl_nft_rule_add(struct mnl_socket *nf_sock, struct nft_rule *nlr,
 
 	nlh = nft_table_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE,
 			nft_rule_attr_get_u32(nlr, NFT_RULE_ATTR_FAMILY),
-			NLM_F_APPEND|NLM_F_ACK|NLM_F_CREATE, seq);
+			flags|NLM_F_ACK|NLM_F_CREATE, seq);
 	nft_rule_nlmsg_build_payload(nlh, nlr);
 
 	return mnl_talk(nf_sock, nlh, nlh->nlmsg_len, NULL, NULL);
diff --git a/src/netlink.c b/src/netlink.c
index 2a7bdb5..5129cac 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -105,6 +105,8 @@ struct nft_rule *alloc_nft_rule(const struct handle *h)
 		nft_rule_attr_set_str(nlr, NFT_RULE_ATTR_CHAIN, h->chain);
 	if (h->handle)
 		nft_rule_attr_set_u64(nlr, NFT_RULE_ATTR_HANDLE, h->handle);
+	if (h->position)
+		nft_rule_attr_set_u64(nlr, NFT_RULE_ATTR_POSITION, h->position);
 	return nlr;
 }
 
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 9348913..f92e83f 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -796,6 +796,8 @@ struct rule *netlink_delinearize_rule(struct netlink_ctx *ctx,
 	h.table  = xstrdup(nft_rule_attr_get_str(nlr, NFT_RULE_ATTR_TABLE));
 	h.chain  = xstrdup(nft_rule_attr_get_str(nlr, NFT_RULE_ATTR_CHAIN));
 	h.handle = nft_rule_attr_get_u64(nlr, NFT_RULE_ATTR_HANDLE);
+	if (nft_rule_attr_is_set(nlr, NFT_RULE_ATTR_POSITION))
+		h.position = nft_rule_attr_get_u64(nlr, NFT_RULE_ATTR_POSITION);
 
 	pctx->rule = rule_alloc(&internal_location, &h);
 	pctx->table = table_lookup(&h);
diff --git a/src/parser.y b/src/parser.y
index 2923b59..91981e9 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -326,6 +326,8 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %token SNAT			"snat"
 %token DNAT			"dnat"
 
+%token POSITION			"position"
+
 %type <string>			identifier string
 %destructor { xfree($$); }	identifier string
 
@@ -339,7 +341,7 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %destructor { handle_free(&$$); } table_spec tables_spec chain_spec chain_identifier ruleid_spec
 %type <handle>			set_spec set_identifier
 %destructor { handle_free(&$$); } set_spec set_identifier
-%type <val>			handle_spec family_spec
+%type <val>			handle_spec family_spec position_spec
 
 %type <table>			table_block_alloc table_block
 %destructor { table_free($$); }	table_block_alloc
@@ -842,10 +844,21 @@ handle_spec		:	/* empty */
 			}
 			;
 
-ruleid_spec		:	chain_spec	handle_spec
+position_spec		:	/* empty */
+			{
+				$$ = 0;
+			}
+			|	POSITION	NUM
+			{
+				$$ = $2;
+			}
+			;
+
+ruleid_spec		:	chain_spec	handle_spec	position_spec
 			{
 				$$		= $1;
 				$$.handle	= $2;
+				$$.position	= $3;
 			}
 			;
 
diff --git a/src/rule.c b/src/rule.c
index 5a894cc..8368624 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -41,6 +41,8 @@ void handle_merge(struct handle *dst, const struct handle *src)
 		dst->set = xstrdup(src->set);
 	if (dst->handle == 0)
 		dst->handle = src->handle;
+	if (dst->position == 0)
+		dst->position = src->position;
 }
 
 struct set *set_alloc(const struct location *loc)
diff --git a/src/scanner.l b/src/scanner.l
index fe7b86c..7946e94 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -249,6 +249,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "flush"			{ return FLUSH; }
 "rename"		{ return RENAME; }
 
+"position"		{ return POSITION; }
+
 "counter"		{ return COUNTER; }
 "packets"		{ return PACKETS; }
 "bytes"			{ return BYTES; }
-- 
1.7.10.4

Re: [nftables PATCH] Add support for insertion inside rule list

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

Hello,

Le vendredi 19 juillet 2013 à 14:28 +0200, Pablo Neira Ayuso a écrit :
> Hi Eric,
> 
> On Sat, Jul 06, 2013 at 05:33:57PM +0200, Eric Leblond wrote:
> > 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.

Yes, forgot to mention that.

> 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.

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

Patch tested. It works well.

BR,
--
Eric

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [nftables PATCH] Add support for insertion inside rule list

[Pablo Neira Ayuso]

On Fri, Jul 19, 2013 at 04:31:27PM +0200, Eric Leblond wrote:
[...]
> > 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.
> 
> I agree with the following modification. I did not find better than this
> so, it is ok for me :)
> 
> Patch tested. It works well.

I have applied this patch, thanks for testing.

Re: [RFC PATCHv2] netfilter: nf_tables: add insert operation

[Tomasz Bursztyka]

Hi Eric,

There is 2 long lines it seems,

> +		if (nla_put_be64(skb, NFTA_RULE_POSITION,
> +				 cpu_to_be64(((struct nft_rule *)rule->list.prev)->handle)))

Here,

> +			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)))

and there.

Tomasz