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

[Eric Leblond <eric@regit.org>]

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