[PATCH RFC 0/3] conntrack: add interface information to accounting extend

[PATCH RFC 0/3] conntrack: add interface information to accounting extend

[Holger Eitzenberger]

Hi,

there are quite some IPFIX collectors (both commercial and not),
which rely on the exported accounting data to contain the interface
index the packet arrived, as well as the interface index the packet
left the box.  This is AFAIK not mandated by the RFC, but just a
de facto standard set by those vendors.

Those collectors then often don't work if interface data is missing.

I try to solve this by adding the missing interface data to the
accounting extend.  See following three patches.

First patch encapsulates both directions of the extend data into a
newly introduced nf_conn_acct.  This could make sense even if not
adding interface indices later.

2nd patch coalesces two calls of ctnetlink_dump_counters() into a
single call of newly created ctnetlink_dump_acct(), with the intent
to add interface info in the following patch.  ABI stays the same.

The 3rd patch then adds the interface indices to the accounting
extend.  I chose to wrap both inside a newly introduced CTA_ACCT
attribute in the hope to improve the situation for readers not using
this extend.  At the cost of adding another 'nlattr' to the output:

 <CTA_ACCT>
   <CTA_ACCT_INDEV, u32>
   <CTA_ACCT_OUTDEV, u32>

Please check the patches, I appreciate your comments.

Thanks.

 /Holger

[PATCH RFC 1/3] acct: introduce nf_conn_acct

[Holger Eitzenberger]

[-- Attachment #1: conntrack-acct-introduce-nf_conn_acct.diff --]
[-- Type: text/plain, Size: 6968 bytes --]

Encapsulate counters for both directions into nf_conn_acct, with
the intent to add data later.

During that process also consistently name pointers to the
extend 'acct', not 'counters'.

The size of the extend is not increased.

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Index: net-next-ipfix/include/net/netfilter/nf_conntrack_acct.h
===================================================================
--- net-next-ipfix.orig/include/net/netfilter/nf_conntrack_acct.h
+++ net-next-ipfix/include/net/netfilter/nf_conntrack_acct.h
@@ -19,17 +19,21 @@ struct nf_conn_counter {
 	atomic64_t bytes;
 };
 
+struct nf_conn_acct {
+	struct nf_conn_counter counter[IP_CT_DIR_MAX];
+};
+
 static inline
-struct nf_conn_counter *nf_conn_acct_find(const struct nf_conn *ct)
+struct nf_conn_acct *nf_conn_acct_find(const struct nf_conn *ct)
 {
 	return nf_ct_ext_find(ct, NF_CT_EXT_ACCT);
 }
 
 static inline
-struct nf_conn_counter *nf_ct_acct_ext_add(struct nf_conn *ct, gfp_t gfp)
+struct nf_conn_acct *nf_ct_acct_ext_add(struct nf_conn *ct, gfp_t gfp)
 {
 	struct net *net = nf_ct_net(ct);
-	struct nf_conn_counter *acct;
+	struct nf_conn_acct *acct;
 
 	if (!net->ct.sysctl_acct)
 		return NULL;
Index: net-next-ipfix/net/netfilter/xt_connbytes.c
===================================================================
--- net-next-ipfix.orig/net/netfilter/xt_connbytes.c
+++ net-next-ipfix/net/netfilter/xt_connbytes.c
@@ -26,16 +26,18 @@ connbytes_mt(const struct sk_buff *skb,
 	u_int64_t what = 0;	/* initialize to make gcc happy */
 	u_int64_t bytes = 0;
 	u_int64_t pkts = 0;
+	const struct nf_conn_acct *acct;
 	const struct nf_conn_counter *counters;
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (!ct)
 		return false;
 
-	counters = nf_conn_acct_find(ct);
-	if (!counters)
+	acct = nf_conn_acct_find(ct);
+	if (!acct)
 		return false;
 
+	counters = acct->counter;
 	switch (sinfo->what) {
 	case XT_CONNBYTES_PKTS:
 		switch (sinfo->direction) {
Index: net-next-ipfix/net/netfilter/nf_conntrack_acct.c
===================================================================
--- net-next-ipfix.orig/net/netfilter/nf_conntrack_acct.c
+++ net-next-ipfix/net/netfilter/nf_conntrack_acct.c
@@ -39,21 +39,23 @@ static struct ctl_table acct_sysctl_tabl
 unsigned int
 seq_print_acct(struct seq_file *s, const struct nf_conn *ct, int dir)
 {
-	struct nf_conn_counter *acct;
+	struct nf_conn_acct *acct;
+	struct nf_conn_counter *counter;
 
 	acct = nf_conn_acct_find(ct);
 	if (!acct)
 		return 0;
 
+	counter = acct->counter;
 	return seq_printf(s, "packets=%llu bytes=%llu ",
-			  (unsigned long long)atomic64_read(&acct[dir].packets),
-			  (unsigned long long)atomic64_read(&acct[dir].bytes));
+			  (unsigned long long)atomic64_read(&counter[dir].packets),
+			  (unsigned long long)atomic64_read(&counter[dir].bytes));
 };
 EXPORT_SYMBOL_GPL(seq_print_acct);
 
 static struct nf_ct_ext_type acct_extend __read_mostly = {
-	.len	= sizeof(struct nf_conn_counter[IP_CT_DIR_MAX]),
-	.align	= __alignof__(struct nf_conn_counter[IP_CT_DIR_MAX]),
+	.len	= sizeof(struct nf_conn_acct),
+	.align	= __alignof__(struct nf_conn_acct),
 	.id	= NF_CT_EXT_ACCT,
 };
 
Index: net-next-ipfix/net/netfilter/nf_conntrack_core.c
===================================================================
--- net-next-ipfix.orig/net/netfilter/nf_conntrack_core.c
+++ net-next-ipfix/net/netfilter/nf_conntrack_core.c
@@ -1109,12 +1109,14 @@ void __nf_ct_refresh_acct(struct nf_conn
 
 acct:
 	if (do_acct) {
-		struct nf_conn_counter *acct;
+		struct nf_conn_acct *acct;
 
 		acct = nf_conn_acct_find(ct);
 		if (acct) {
-			atomic64_inc(&acct[CTINFO2DIR(ctinfo)].packets);
-			atomic64_add(skb->len, &acct[CTINFO2DIR(ctinfo)].bytes);
+			struct nf_conn_counter *counter = acct->counter;
+
+			atomic64_inc(&counter[CTINFO2DIR(ctinfo)].packets);
+			atomic64_add(skb->len, &counter[CTINFO2DIR(ctinfo)].bytes);
 		}
 	}
 }
@@ -1126,13 +1128,15 @@ bool __nf_ct_kill_acct(struct nf_conn *c
 		       int do_acct)
 {
 	if (do_acct) {
-		struct nf_conn_counter *acct;
+		struct nf_conn_acct *acct;
 
 		acct = nf_conn_acct_find(ct);
 		if (acct) {
-			atomic64_inc(&acct[CTINFO2DIR(ctinfo)].packets);
+			struct nf_conn_counter *counter = acct->counter;
+
+			atomic64_inc(&counter[CTINFO2DIR(ctinfo)].packets);
 			atomic64_add(skb->len - skb_network_offset(skb),
-				     &acct[CTINFO2DIR(ctinfo)].bytes);
+				     &counter[CTINFO2DIR(ctinfo)].bytes);
 		}
 	}
 
Index: net-next-ipfix/include/net/netfilter/nf_conntrack_extend.h
===================================================================
--- net-next-ipfix.orig/include/net/netfilter/nf_conntrack_extend.h
+++ net-next-ipfix/include/net/netfilter/nf_conntrack_extend.h
@@ -36,7 +36,7 @@ enum nf_ct_ext_id {
 #define NF_CT_EXT_HELPER_TYPE struct nf_conn_help
 #define NF_CT_EXT_NAT_TYPE struct nf_conn_nat
 #define NF_CT_EXT_SEQADJ_TYPE struct nf_conn_seqadj
-#define NF_CT_EXT_ACCT_TYPE struct nf_conn_counter
+#define NF_CT_EXT_ACCT_TYPE struct nf_conn_acct
 #define NF_CT_EXT_ECACHE_TYPE struct nf_conntrack_ecache
 #define NF_CT_EXT_ZONE_TYPE struct nf_conntrack_zone
 #define NF_CT_EXT_TSTAMP_TYPE struct nf_conn_tstamp
Index: net-next-ipfix/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-ipfix.orig/net/netfilter/nf_conntrack_netlink.c
+++ net-next-ipfix/net/netfilter/nf_conntrack_netlink.c
@@ -237,19 +237,21 @@ static int
 ctnetlink_dump_counters(struct sk_buff *skb, const struct nf_conn *ct,
 			enum ip_conntrack_dir dir, int type)
 {
-	struct nf_conn_counter *acct;
+	struct nf_conn_acct *acct;
+	struct nf_conn_counter *counter;
 	u64 pkts, bytes;
 
 	acct = nf_conn_acct_find(ct);
 	if (!acct)
 		return 0;
 
+	counter = acct->counter;
 	if (type == IPCTNL_MSG_CT_GET_CTRZERO) {
-		pkts = atomic64_xchg(&acct[dir].packets, 0);
-		bytes = atomic64_xchg(&acct[dir].bytes, 0);
+		pkts = atomic64_xchg(&counter[dir].packets, 0);
+		bytes = atomic64_xchg(&counter[dir].bytes, 0);
 	} else {
-		pkts = atomic64_read(&acct[dir].packets);
-		bytes = atomic64_read(&acct[dir].bytes);
+		pkts = atomic64_read(&counter[dir].packets);
+		bytes = atomic64_read(&counter[dir].bytes);
 	}
 	return dump_counters(skb, pkts, bytes, dir);
 }
@@ -530,7 +532,7 @@ ctnetlink_proto_size(const struct nf_con
 }
 
 static inline size_t
-ctnetlink_counters_size(const struct nf_conn *ct)
+ctnetlink_acct_size(const struct nf_conn *ct)
 {
 	if (!nf_ct_ext_exist(ct, NF_CT_EXT_ACCT))
 		return 0;
@@ -579,7 +581,7 @@ ctnetlink_nlmsg_size(const struct nf_con
 	       + 3 * nla_total_size(sizeof(u_int8_t)) /* CTA_PROTO_NUM */
 	       + nla_total_size(sizeof(u_int32_t)) /* CTA_ID */
 	       + nla_total_size(sizeof(u_int32_t)) /* CTA_STATUS */
-	       + ctnetlink_counters_size(ct)
+	       + ctnetlink_acct_size(ct)
 	       + ctnetlink_timestamp_size(ct)
 	       + nla_total_size(sizeof(u_int32_t)) /* CTA_TIMEOUT */
 	       + nla_total_size(0) /* CTA_PROTOINFO */

[PATCH RFC 2/3] ctnetlink: account both directions in one step

[Holger Eitzenberger]

[-- Attachment #1: conntrack-acct-introduce-ctnetlink_dump_acct.diff --]
[-- Type: text/plain, Size: 3206 bytes --]

With the intent to dump other accounting data later.

This patch does not change the ABI.

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Index: net-next-ipfix/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-ipfix.orig/net/netfilter/nf_conntrack_netlink.c
+++ net-next-ipfix/net/netfilter/nf_conntrack_netlink.c
@@ -211,13 +211,24 @@ nla_put_failure:
 }
 
 static int
-dump_counters(struct sk_buff *skb, u64 pkts, u64 bytes,
-	      enum ip_conntrack_dir dir)
+dump_counters(struct sk_buff *skb, struct nf_conn_acct *acct,
+	      enum ip_conntrack_dir dir, int type)
 {
-	enum ctattr_type type = dir ? CTA_COUNTERS_REPLY: CTA_COUNTERS_ORIG;
+	enum ctattr_type attr = dir ? CTA_COUNTERS_REPLY: CTA_COUNTERS_ORIG;
+	struct nf_conn_counter *counter;
 	struct nlattr *nest_count;
+	u64 pkts, bytes;
 
-	nest_count = nla_nest_start(skb, type | NLA_F_NESTED);
+	counter = acct->counter;
+	if (type == IPCTNL_MSG_CT_GET_CTRZERO) {
+		pkts = atomic64_xchg(&counter[dir].packets, 0);
+		bytes = atomic64_xchg(&counter[dir].bytes, 0);
+	} else {
+		pkts = atomic64_read(&counter[dir].packets);
+		bytes = atomic64_read(&counter[dir].bytes);
+	}
+
+	nest_count = nla_nest_start(skb, attr | NLA_F_NESTED);
 	if (!nest_count)
 		goto nla_put_failure;
 
@@ -234,26 +245,19 @@ nla_put_failure:
 }
 
 static int
-ctnetlink_dump_counters(struct sk_buff *skb, const struct nf_conn *ct,
-			enum ip_conntrack_dir dir, int type)
+ctnetlink_dump_acct(struct sk_buff *skb, const struct nf_conn *ct, int type)
 {
-	struct nf_conn_acct *acct;
-	struct nf_conn_counter *counter;
-	u64 pkts, bytes;
+	struct nf_conn_acct *acct = nf_conn_acct_find(ct);
 
-	acct = nf_conn_acct_find(ct);
-	if (!acct)
+	if (acct == NULL)
 		return 0;
 
-	counter = acct->counter;
-	if (type == IPCTNL_MSG_CT_GET_CTRZERO) {
-		pkts = atomic64_xchg(&counter[dir].packets, 0);
-		bytes = atomic64_xchg(&counter[dir].bytes, 0);
-	} else {
-		pkts = atomic64_read(&counter[dir].packets);
-		bytes = atomic64_read(&counter[dir].bytes);
-	}
-	return dump_counters(skb, pkts, bytes, dir);
+	if (dump_counters(skb, acct, IP_CT_DIR_ORIGINAL, type) < 0)
+		return -1;
+	if (dump_counters(skb, acct, IP_CT_DIR_REPLY, type) < 0)
+		return -1;
+
+	return 0;
 }
 
 static int
@@ -490,8 +494,7 @@ ctnetlink_fill_info(struct sk_buff *skb,
 
 	if (ctnetlink_dump_status(skb, ct) < 0 ||
 	    ctnetlink_dump_timeout(skb, ct) < 0 ||
-	    ctnetlink_dump_counters(skb, ct, IP_CT_DIR_ORIGINAL, type) < 0 ||
-	    ctnetlink_dump_counters(skb, ct, IP_CT_DIR_REPLY, type) < 0 ||
+	    ctnetlink_dump_acct(skb, ct, type) < 0 ||
 	    ctnetlink_dump_timestamp(skb, ct) < 0 ||
 	    ctnetlink_dump_protoinfo(skb, ct) < 0 ||
 	    ctnetlink_dump_helpinfo(skb, ct) < 0 ||
@@ -675,10 +678,7 @@ ctnetlink_conntrack_event(unsigned int e
 		goto nla_put_failure;
 
 	if (events & (1 << IPCT_DESTROY)) {
-		if (ctnetlink_dump_counters(skb, ct,
-					    IP_CT_DIR_ORIGINAL, type) < 0 ||
-		    ctnetlink_dump_counters(skb, ct,
-					    IP_CT_DIR_REPLY, type) < 0 ||
+		if (ctnetlink_dump_acct(skb, ct, type) < 0 ||
 		    ctnetlink_dump_timestamp(skb, ct) < 0)
 			goto nla_put_failure;
 	} else {

[PATCH RFC 3/3] acct: add input and output interface index

[Holger Eitzenberger]

[-- Attachment #1: conntrack-acct-add-in-out-ifindex.diff --]
[-- Type: text/plain, Size: 3812 bytes --]

The interface indices are exported as uint32_t, although being
signed integer inside the kernel, which goes in line with
what nfnetlink_queue does.

Both interface indices are wrapped inside CTA_ACCT.

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Index: net-next-ipfix/include/net/netfilter/nf_conntrack_acct.h
===================================================================
--- net-next-ipfix.orig/include/net/netfilter/nf_conntrack_acct.h
+++ net-next-ipfix/include/net/netfilter/nf_conntrack_acct.h
@@ -21,6 +21,8 @@ struct nf_conn_counter {
 
 struct nf_conn_acct {
 	struct nf_conn_counter counter[IP_CT_DIR_MAX];
+	int indev;
+	int outdev;
 };
 
 static inline
Index: net-next-ipfix/net/netfilter/nf_conntrack_core.c
===================================================================
--- net-next-ipfix.orig/net/netfilter/nf_conntrack_core.c
+++ net-next-ipfix/net/netfilter/nf_conntrack_core.c
@@ -33,6 +33,7 @@
 #include <linux/mm.h>
 #include <linux/nsproxy.h>
 #include <linux/rculist_nulls.h>
+#include <net/dst.h>
 
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_l3proto.h>
@@ -1110,6 +1111,7 @@ void __nf_ct_refresh_acct(struct nf_conn
 acct:
 	if (do_acct) {
 		struct nf_conn_acct *acct;
+		struct dst_entry *dst;
 
 		acct = nf_conn_acct_find(ct);
 		if (acct) {
@@ -1117,6 +1119,13 @@ acct:
 
 			atomic64_inc(&counter[CTINFO2DIR(ctinfo)].packets);
 			atomic64_add(skb->len, &counter[CTINFO2DIR(ctinfo)].bytes);
+
+			if (acct->indev == 0 && skb->dev)
+				acct->indev = skb->dev->ifindex;
+
+			dst = skb_dst(skb);
+			if (acct->outdev == 0 && dst && dst->dev)
+				acct->outdev = dst->dev->ifindex;
 		}
 	}
 }
Index: net-next-ipfix/include/uapi/linux/netfilter/nfnetlink_conntrack.h
===================================================================
--- net-next-ipfix.orig/include/uapi/linux/netfilter/nfnetlink_conntrack.h
+++ net-next-ipfix/include/uapi/linux/netfilter/nfnetlink_conntrack.h
@@ -53,6 +53,7 @@ enum ctattr_type {
 	CTA_MARK_MASK,
 	CTA_LABELS,
 	CTA_LABELS_MASK,
+	CTA_ACCT,
 	__CTA_MAX
 };
 #define CTA_MAX (__CTA_MAX - 1)
@@ -138,6 +139,14 @@ enum ctattr_counters {
 };
 #define CTA_COUNTERS_MAX (__CTA_COUNTERS_MAX - 1)
 
+enum ctattr_acct {
+	CTA_ACCT_UNSPEC,
+	CTA_ACCT_INDEV,
+	CTA_ACCT_OUTDEV,
+	__CTA_ACCT_MAX
+};
+#define CTA_ACCT_MAX (__CTA_ACCT_MAX - 1)
+
 enum ctattr_tstamp {
 	CTA_TIMESTAMP_UNSPEC,
 	CTA_TIMESTAMP_START,
Index: net-next-ipfix/net/netfilter/nf_conntrack_netlink.c
===================================================================
--- net-next-ipfix.orig/net/netfilter/nf_conntrack_netlink.c
+++ net-next-ipfix/net/netfilter/nf_conntrack_netlink.c
@@ -248,15 +248,27 @@ static int
 ctnetlink_dump_acct(struct sk_buff *skb, const struct nf_conn *ct, int type)
 {
 	struct nf_conn_acct *acct = nf_conn_acct_find(ct);
+	struct nlattr *nla_acct;
 
 	if (acct == NULL)
 		return 0;
 
+	/* counters are not nested in CTA_ACCT for backward compatibility */
 	if (dump_counters(skb, acct, IP_CT_DIR_ORIGINAL, type) < 0)
 		return -1;
 	if (dump_counters(skb, acct, IP_CT_DIR_REPLY, type) < 0)
 		return -1;
 
+	nla_acct = nla_nest_start(skb, CTA_ACCT | NLA_F_NESTED);
+	if (!nla_acct)
+		return -1;
+
+	if (nla_put_be32(skb, CTA_ACCT_INDEV, htonl(acct->indev)) ||
+		nla_put_be32(skb, CTA_ACCT_OUTDEV, htonl(acct->outdev)))
+		return -1;
+
+	nla_nest_end(skb, nla_acct);
+
 	return 0;
 }
 
@@ -542,6 +554,8 @@ ctnetlink_acct_size(const struct nf_conn
 	return 2 * nla_total_size(0) /* CTA_COUNTERS_ORIG|REPL */
 	       + 2 * nla_total_size(sizeof(uint64_t)) /* CTA_COUNTERS_PACKETS */
 	       + 2 * nla_total_size(sizeof(uint64_t)) /* CTA_COUNTERS_BYTES */
+		   + nla_total_size(0)					  /* CTA_ACCT */
+	       + 2 * nla_total_size(sizeof(uint32_t)) /* CTA_(IN|OUT)DEV */
 	       ;
 }
 

Re: [PATCH RFC 3/3] acct: add input and output interface index

[Pablo Neira Ayuso]

Hi Holger,

I like patches 1/3 and 2/3, they are nice cleanups.

Some comments regarding this patch.

On Thu, Sep 26, 2013 at 05:31:53PM +0200, Holger Eitzenberger wrote:
> The interface indices are exported as uint32_t, although being
> signed integer inside the kernel, which goes in line with
> what nfnetlink_queue does.
> 
> Both interface indices are wrapped inside CTA_ACCT.
> 
> Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>
> 
> Index: net-next-ipfix/include/net/netfilter/nf_conntrack_acct.h
> ===================================================================
> --- net-next-ipfix.orig/include/net/netfilter/nf_conntrack_acct.h
> +++ net-next-ipfix/include/net/netfilter/nf_conntrack_acct.h
> @@ -21,6 +21,8 @@ struct nf_conn_counter {
>  
>  struct nf_conn_acct {
>  	struct nf_conn_counter counter[IP_CT_DIR_MAX];
> +	int indev;
> +	int outdev;
>  };
>  
>  static inline
> Index: net-next-ipfix/net/netfilter/nf_conntrack_core.c
> ===================================================================
> --- net-next-ipfix.orig/net/netfilter/nf_conntrack_core.c
> +++ net-next-ipfix/net/netfilter/nf_conntrack_core.c
> @@ -33,6 +33,7 @@
>  #include <linux/mm.h>
>  #include <linux/nsproxy.h>
>  #include <linux/rculist_nulls.h>
> +#include <net/dst.h>
>  
>  #include <net/netfilter/nf_conntrack.h>
>  #include <net/netfilter/nf_conntrack_l3proto.h>
> @@ -1110,6 +1111,7 @@ void __nf_ct_refresh_acct(struct nf_conn
>  acct:
>  	if (do_acct) {
>  		struct nf_conn_acct *acct;
> +		struct dst_entry *dst;
>  
>  		acct = nf_conn_acct_find(ct);
>  		if (acct) {
> @@ -1117,6 +1119,13 @@ acct:
>  
>  			atomic64_inc(&counter[CTINFO2DIR(ctinfo)].packets);
>  			atomic64_add(skb->len, &counter[CTINFO2DIR(ctinfo)].bytes);
> +
> +			if (acct->indev == 0 && skb->dev)
> +				acct->indev = skb->dev->ifindex;
> +
> +			dst = skb_dst(skb);
> +			if (acct->outdev == 0 && dst && dst->dev)
> +				acct->outdev = dst->dev->ifindex;

If you only set indev/outdev once we can skip the conntrack extension
by passing the skb to nf_ct_deliver_cached_events and include this
information in the conntrack events. That would not allow to dump the
device from conntrack dumps though. I still have concerns with this
approach as this doesn't seem to cover the scenario in which the
in/outdev changes.

Regards.

Re: [PATCH RFC 3/3] acct: add input and output interface index

[Holger Eitzenberger]

Hi Pablo,

> I like patches 1/3 and 2/3, they are nice cleanups.

thanks for looking into this.

> If you only set indev/outdev once we can skip the conntrack extension
> by passing the skb to nf_ct_deliver_cached_events and include this
> information in the conntrack events. That would not allow to dump the
> device from conntrack dumps though. I still have concerns with this
> approach as this doesn't seem to cover the scenario in which the
> in/outdev changes.

I know that doing it this simiple way is only "best effort", as e. g.
with IP multipathing or 802.3ad this information is not % correct
in all cases.

And the question we have to answer is whether this interface
information *has* to be correct in every case, even the less commonly
used cases.

For IPFIX I would answer this question with a 'no'.

And we can later extend this to update the interface information
correctly in every case.  It's only a few patches away.

 /Holger

Re: [PATCH RFC 3/3] acct: add input and output interface index

[Pablo Neira Ayuso]

On Thu, Oct 17, 2013 at 01:33:45PM +0200, Holger Eitzenberger wrote:
> Hi Pablo,
> 
> > I like patches 1/3 and 2/3, they are nice cleanups.
> 
> thanks for looking into this.

I'm going to apply 1/3 and 2/3 with some small glitches, I would like
not to lose these cleanups.

> > If you only set indev/outdev once we can skip the conntrack extension
> > by passing the skb to nf_ct_deliver_cached_events and include this
> > information in the conntrack events. That would not allow to dump the
> > device from conntrack dumps though. I still have concerns with this
> > approach as this doesn't seem to cover the scenario in which the
> > in/outdev changes.
> 
> I know that doing it this simiple way is only "best effort", as e. g.
> with IP multipathing or 802.3ad this information is not % correct
> in all cases.
> 
> And the question we have to answer is whether this interface
> information *has* to be correct in every case, even the less commonly
> used cases.
> 
> For IPFIX I would answer this question with a 'no'.
> 
> And we can later extend this to update the interface information
> correctly in every case.  It's only a few patches away.

My suggestion is to rework patch 3/3 to pass the interface information
to nf_ct_deliver_cached_events via nf_ct_event struct, then include it
in the event message. Thus, we don't need to increase the size the
conntrack. The downside of this approach is that we cannot retrieve
the interface via dump operation, but I think it should be enough for
IPFIX. This feature should be disabled by default, so please add a
/proc switch to enable/disable it in runtime.

Thanks.