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@xxxxxxxxx>
> > > > > > ---
> > > > > > 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:
signature.asc
Description: This is a digitally signed message part
