[PATCH 0/3] nf-next-2.6 pull request, Complete deprecation of CONFIG_NF_CT_ACCT (V2)

[PATCH 0/3] nf-next-2.6 pull request, Complete deprecation of CONFIG_NF_CT_ACCT (V2)

[tim.gardner]

The following changes since commit fe6fb552858f686f39e33d7b0a33fe56dacea0bf:
  Arnd Hannemann (1):
        netfilter: fix simple typo in KConfig for netfiltert xt_TEE

are available in the git repository at:

  git://kernel.ubuntu.com/rtg/nf-next-2.6 CONFIG_NF_CT_ACCT

Tim Gardner (3):
      netfilter: Expose connection tracking accounting toggles
      netfilter: xt_connbytes: Force CT accounting to be enabled
      netfilter: Complete the deprecation of CONFIG_NF_CT_ACCT

 Documentation/feature-removal-schedule.txt |    9 ---------
 Documentation/kernel-parameters.txt        |    3 +--
 include/net/netfilter/nf_conntrack.h       |   12 ++++++++++++
 net/netfilter/Kconfig                      |   22 ----------------------
 net/netfilter/nf_conntrack_acct.c          |   10 ----------
 net/netfilter/xt_connbytes.c               |   13 ++++++++++++-
 6 files changed, 25 insertions(+), 44 deletions(-)

[PATCH 1/3] netfilter: Expose connection tracking accounting toggles

[tim.gardner]

From: Tim Gardner <tim.gardner@canonical.com>

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 include/net/netfilter/nf_conntrack.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index e624dae..2326754 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -232,6 +232,18 @@ static inline void nf_ct_refresh(struct nf_conn *ct,
 	__nf_ct_refresh_acct(ct, 0, skb, extra_jiffies, 0);
 }
 
+/* Check if connection tracking accounting is enabled */
+static inline bool nf_ct_acct_enabled(struct nf_conn *ct)
+{
+		return ct->ct_net->ct.sysctl_acct == 0 ? false : true;
+}
+
+/* Enable/disable connection tracking accounting */
+static inline void nf_ct_set_acct(struct nf_conn *ct, bool enable)
+{
+		ct->ct_net->ct.sysctl_acct = enable == true ? 1 : 0;
+}
+
 extern bool __nf_ct_kill_acct(struct nf_conn *ct,
 			      enum ip_conntrack_info ctinfo,
 			      const struct sk_buff *skb,
-- 
1.7.0.4

[PATCH 2/3] netfilter: xt_connbytes: Force CT tracking to be enabled

[tim.gardner]

From: Tim Gardner <tim.gardner@canonical.com>

Check at runtime that CT tracking is enabled, and force it
to be enabled if not.

This is in preparation for deprecating CONFIG_NF_CT_ACCT.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 net/netfilter/xt_connbytes.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/xt_connbytes.c b/net/netfilter/xt_connbytes.c
index 7351783..d703355 100644
--- a/net/netfilter/xt_connbytes.c
+++ b/net/netfilter/xt_connbytes.c
@@ -21,7 +21,7 @@ static bool
 connbytes_mt(const struct sk_buff *skb, struct xt_action_param *par)
 {
 	const struct xt_connbytes_info *sinfo = par->matchinfo;
-	const struct nf_conn *ct;
+	struct nf_conn *ct;
 	enum ip_conntrack_info ctinfo;
 	u_int64_t what = 0;	/* initialize to make gcc happy */
 	u_int64_t bytes = 0;
@@ -32,6 +32,17 @@ connbytes_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	if (!ct)
 		return false;
 
+	/*
+	 * This filter cannot function correctly unless connection tracking
+	 * accounting is enabled, so complain about it until someone notices.
+	 * It _should_ only print one warning message.
+	 */
+	if (unlikely(nf_ct_acct_enabled(ct) == false)) {
+		if (net_ratelimit())
+			pr_warning("ipt_connbytes: Force enabling CT accounting\n");
+		nf_ct_set_acct(ct, true);
+	}
+
 	counters = nf_conn_acct_find(ct);
 	if (!counters)
 		return false;
-- 
1.7.0.4

[PATCH 3/3] netfilter: Complete the deprecation of CONFIG_NF_CT_ACCT

[tim.gardner]

From: Tim Gardner <tim.gardner@canonical.com>

CONFIG_NF_CT_ACCT has been deprecated for awhile and
was originally scheduled for removal by 2.6.29.

Removing support for this config option also stops
this deprecation warning message in the kernel log.

[   61.669627] nf_conntrack version 0.5.0 (16384 buckets, 65536 max)
[   61.669850] CONFIG_NF_CT_ACCT is deprecated and will be removed soon. Please use
[   61.669852] nf_conntrack.acct=1 kernel parameter, acct=1 nf_conntrack module option or
[   61.669853] sysctl net.netfilter.nf_conntrack_acct=1 to enable it.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 Documentation/feature-removal-schedule.txt |    9 ---------
 Documentation/kernel-parameters.txt        |    3 +--
 net/netfilter/Kconfig                      |   22 ----------------------
 net/netfilter/nf_conntrack_acct.c          |   10 ----------
 4 files changed, 1 insertions(+), 43 deletions(-)

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index 672be01..92f021a 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -303,15 +303,6 @@ Who:	Johannes Berg <johannes@sipsolutions.net>
 
 ---------------------------
 
-What: CONFIG_NF_CT_ACCT
-When: 2.6.29
-Why:  Accounting can now be enabled/disabled without kernel recompilation.
-      Currently used only to set a default value for a feature that is also
-      controlled by a kernel/module/sysfs/sysctl parameter.
-Who:  Krzysztof Piotr Oledzki <ole@ans.pl>
-
----------------------------
-
 What:	sysfs ui for changing p4-clockmod parameters
 When:	September 2009
 Why:	See commits 129f8ae9b1b5be94517da76009ea956e89104ce8 and
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 1808f11..a7279d0 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1597,8 +1597,7 @@ and is between 256 and 4096 characters. It is defined in the file
 			[NETFILTER] Enable connection tracking flow accounting
 			0 to disable accounting
 			1 to enable accounting
-			Default value depends on CONFIG_NF_CT_ACCT that is
-			going to be removed in 2.6.29.
+			Default value is 1
 
 	nfsaddrs=	[NFS]
 			See Documentation/filesystems/nfs/nfsroot.txt.
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 21be535..aa2f106 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -40,27 +40,6 @@ config NF_CONNTRACK
 
 if NF_CONNTRACK
 
-config NF_CT_ACCT
-	bool "Connection tracking flow accounting"
-	depends on NETFILTER_ADVANCED
-	help
-	  If this option is enabled, the connection tracking code will
-	  keep per-flow packet and byte counters.
-
-	  Those counters can be used for flow-based accounting or the
-	  `connbytes' match.
-
-	  Please note that currently this option only sets a default state.
-	  You may change it at boot time with nf_conntrack.acct=0/1 kernel
-	  parameter or by loading the nf_conntrack module with acct=0/1.
-
-	  You may also disable/enable it on a running system with:
-	   sysctl net.netfilter.nf_conntrack_acct=0/1
-
-	  This option will be removed in 2.6.29.
-
-	  If unsure, say `N'.
-
 config NF_CONNTRACK_MARK
 	bool  'Connection mark tracking support'
 	depends on NETFILTER_ADVANCED
@@ -630,7 +609,6 @@ config NETFILTER_XT_MATCH_CONNBYTES
 	tristate  '"connbytes" per-connection counter match support'
 	depends on NF_CONNTRACK
 	depends on NETFILTER_ADVANCED
-	select NF_CT_ACCT
 	help
 	  This option adds a `connbytes' match, which allows you to match the
 	  number of bytes and/or packets for each direction within a connection.
diff --git a/net/netfilter/nf_conntrack_acct.c b/net/netfilter/nf_conntrack_acct.c
index ab81b38..57059aa 100644
--- a/net/netfilter/nf_conntrack_acct.c
+++ b/net/netfilter/nf_conntrack_acct.c
@@ -17,11 +17,7 @@
 #include <net/netfilter/nf_conntrack_extend.h>
 #include <net/netfilter/nf_conntrack_acct.h>
 
-#ifdef CONFIG_NF_CT_ACCT
 #define NF_CT_ACCT_DEFAULT 1
-#else
-#define NF_CT_ACCT_DEFAULT 0
-#endif
 
 static int nf_ct_acct __read_mostly = NF_CT_ACCT_DEFAULT;
 
@@ -114,12 +110,6 @@ int nf_conntrack_acct_init(struct net *net)
 	net->ct.sysctl_acct = nf_ct_acct;
 
 	if (net_eq(net, &init_net)) {
-#ifdef CONFIG_NF_CT_ACCT
-	printk(KERN_WARNING "CONFIG_NF_CT_ACCT is deprecated and will be removed soon. Please use\n");
-		printk(KERN_WARNING "nf_conntrack.acct=1 kernel parameter, acct=1 nf_conntrack module option or\n");
-		printk(KERN_WARNING "sysctl net.netfilter.nf_conntrack_acct=1 to enable it.\n");
-#endif
-
 		ret = nf_ct_extend_register(&acct_extend);
 		if (ret < 0) {
 			printk(KERN_ERR "nf_conntrack_acct: Unable to register extension\n");
-- 
1.7.0.4

Re: [PATCH 2/3] netfilter: xt_connbytes: Force CT tracking to be enabled

[Jan Engelhardt]

On Tuesday 2010-06-22 18:44, tim.gardner@canonical.com wrote:
> net/netfilter/xt_connbytes.c |   13 ++++++++++++-
> 1 files changed, 12 insertions(+), 1 deletions(-)
>
>+	 * accounting is enabled, so complain about it until someone notices.
>+	 * It _should_ only print one warning message.
>+	 */
>+	if (unlikely(nf_ct_acct_enabled(ct) == false)) {
>+		if (net_ratelimit())
>+			pr_warning("ipt_connbytes: Force enabling CT accounting\n");
>+		nf_ct_set_acct(ct, true);
>+	}

Am I in a timewarp vortex? It's xt_connbytes, not ipt_connbytes.
Better yet, use KBUILD_MODNAME together with pr_fmt.

Re: [PATCH 2/3] netfilter: xt_connbytes: Force CT tracking to be enabled

[Tim Gardner]

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

Bah! This should be 'CT accounting', not 'CT tracking'. The commit in 
the git repo is correct, I'd just forgotten to regen the patches that I 
emailed.

rtg

-- 
Tim Gardner tim.gardner@canonical.com

[-- Attachment #2: 0002-netfilter-xt_connbytes-Force-CT-accounting-to-be-ena.patch --]
[-- Type: text/x-patch, Size: 1617 bytes --]

>From 5836a019e4d267d78ba2b33db2d77cd03cd83fb2 Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Tue, 22 Jun 2010 09:27:30 -0600
Subject: [PATCH 2/3] netfilter: xt_connbytes: Force CT accounting to be enabled

Check at runtime that CT accounting is enabled, and force it
to be enabled if not.

This is in preparation for deprecating CONFIG_NF_CT_ACCT.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 net/netfilter/xt_connbytes.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/xt_connbytes.c b/net/netfilter/xt_connbytes.c
index 7351783..d703355 100644
--- a/net/netfilter/xt_connbytes.c
+++ b/net/netfilter/xt_connbytes.c
@@ -21,7 +21,7 @@ static bool
 connbytes_mt(const struct sk_buff *skb, struct xt_action_param *par)
 {
 	const struct xt_connbytes_info *sinfo = par->matchinfo;
-	const struct nf_conn *ct;
+	struct nf_conn *ct;
 	enum ip_conntrack_info ctinfo;
 	u_int64_t what = 0;	/* initialize to make gcc happy */
 	u_int64_t bytes = 0;
@@ -32,6 +32,17 @@ connbytes_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	if (!ct)
 		return false;
 
+	/*
+	 * This filter cannot function correctly unless connection tracking
+	 * accounting is enabled, so complain about it until someone notices.
+	 * It _should_ only print one warning message.
+	 */
+	if (unlikely(nf_ct_acct_enabled(ct) == false)) {
+		if (net_ratelimit())
+			pr_warning("ipt_connbytes: Force enabling CT accounting\n");
+		nf_ct_set_acct(ct, true);
+	}
+
 	counters = nf_conn_acct_find(ct);
 	if (!counters)
 		return false;
-- 
1.7.0.4

Re: [PATCH 2/3] netfilter: xt_connbytes: Force CT tracking to be enabled

[Tim Gardner]

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

On 06/22/2010 10:49 AM, Jan Engelhardt wrote:
>
>
> On Tuesday 2010-06-22 18:44, tim.gardner@canonical.com wrote:
>> net/netfilter/xt_connbytes.c |   13 ++++++++++++-
>> 1 files changed, 12 insertions(+), 1 deletions(-)
>>
>> +	 * accounting is enabled, so complain about it until someone notices.
>> +	 * It _should_ only print one warning message.
>> +	 */
>> +	if (unlikely(nf_ct_acct_enabled(ct) == false)) {
>> +		if (net_ratelimit())
>> +			pr_warning("ipt_connbytes: Force enabling CT accounting\n");
>> +		nf_ct_set_acct(ct, true);
>> +	}
>
> Am I in a timewarp vortex? It's xt_connbytes, not ipt_connbytes.
> Better yet, use KBUILD_MODNAME together with pr_fmt.
> --

It turns out that pr_warning() already uses pr_fmt(). Change pushed.

rtg
-- 
Tim Gardner tim.gardner@canonical.com

[-- Attachment #2: 0002-netfilter-xt_connbytes-Force-CT-accounting-to-be-ena.patch --]
[-- Type: text/x-patch, Size: 1602 bytes --]

>From 5b47470d916e85bfc5df835580c5898997fdeb81 Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Tue, 22 Jun 2010 09:27:30 -0600
Subject: [PATCH 2/3] netfilter: xt_connbytes: Force CT accounting to be enabled

Check at runtime that CT accounting is enabled, and force it
to be enabled if not.

This is in preparation for deprecating CONFIG_NF_CT_ACCT.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 net/netfilter/xt_connbytes.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/xt_connbytes.c b/net/netfilter/xt_connbytes.c
index 7351783..b25bf54 100644
--- a/net/netfilter/xt_connbytes.c
+++ b/net/netfilter/xt_connbytes.c
@@ -21,7 +21,7 @@ static bool
 connbytes_mt(const struct sk_buff *skb, struct xt_action_param *par)
 {
 	const struct xt_connbytes_info *sinfo = par->matchinfo;
-	const struct nf_conn *ct;
+	struct nf_conn *ct;
 	enum ip_conntrack_info ctinfo;
 	u_int64_t what = 0;	/* initialize to make gcc happy */
 	u_int64_t bytes = 0;
@@ -32,6 +32,17 @@ connbytes_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	if (!ct)
 		return false;
 
+	/*
+	 * This filter cannot function correctly unless connection tracking
+	 * accounting is enabled, so complain about it until someone notices.
+	 * It _should_ only print one warning message.
+	 */
+	if (unlikely(nf_ct_acct_enabled(ct) == false)) {
+		if (net_ratelimit())
+			pr_warning("Force enabling CT accounting\n");
+		nf_ct_set_acct(ct, true);
+	}
+
 	counters = nf_conn_acct_find(ct);
 	if (!counters)
 		return false;
-- 
1.7.0.4

Re: [PATCH 1/3] netfilter: Expose connection tracking accounting toggles

[Patrick McHardy]

tim.gardner@canonical.com wrote:
> From: Tim Gardner <tim.gardner@canonical.com>
>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>  include/net/netfilter/nf_conntrack.h |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
> index e624dae..2326754 100644
> --- a/include/net/netfilter/nf_conntrack.h
> +++ b/include/net/netfilter/nf_conntrack.h
> @@ -232,6 +232,18 @@ static inline void nf_ct_refresh(struct nf_conn *ct,
>  	__nf_ct_refresh_acct(ct, 0, skb, extra_jiffies, 0);
>  }
>  
> +/* Check if connection tracking accounting is enabled */
> +static inline bool nf_ct_acct_enabled(struct nf_conn *ct)
> +{
> +		return ct->ct_net->ct.sysctl_acct == 0 ? false : true;
> +}
> +
> +/* Enable/disable connection tracking accounting */
> +static inline void nf_ct_set_acct(struct nf_conn *ct, bool enable)
> +{
> +		ct->ct_net->ct.sysctl_acct = enable == true ? 1 : 0;
> +}
> +

This looks strangely indented, please use a single tab. You also need
to take care of the CONFIG_NET_NS=n case where the ct_net pointer is
#ifdef'ed out. I'd suggest to simply pass the net pointer from
xt_mtchk_param, which is available unconditionally.

Re: [PATCH 2/3] netfilter: xt_connbytes: Force CT tracking to be enabled

[Patrick McHardy]

tim.gardner@canonical.com wrote:
> From: Tim Gardner <tim.gardner@canonical.com>
>
> Check at runtime that CT tracking is enabled, and force it
> to be enabled if not.
>
> This is in preparation for deprecating CONFIG_NF_CT_ACCT.
>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>  net/netfilter/xt_connbytes.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/net/netfilter/xt_connbytes.c b/net/netfilter/xt_connbytes.c
> index 7351783..d703355 100644
> --- a/net/netfilter/xt_connbytes.c
> +++ b/net/netfilter/xt_connbytes.c
> @@ -21,7 +21,7 @@ static bool
>  connbytes_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  {
>  	const struct xt_connbytes_info *sinfo = par->matchinfo;
> -	const struct nf_conn *ct;
> +	struct nf_conn *ct;
>  	enum ip_conntrack_info ctinfo;
>  	u_int64_t what = 0;	/* initialize to make gcc happy */
>  	u_int64_t bytes = 0;
> @@ -32,6 +32,17 @@ connbytes_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  	if (!ct)
>  		return false;
>  
> +	/*
> +	 * This filter cannot function correctly unless connection tracking
> +	 * accounting is enabled, so complain about it until someone notices.
> +	 * It _should_ only print one warning message.
> +	 */
> +	if (unlikely(nf_ct_acct_enabled(ct) == false)) {
> +		if (net_ratelimit())
> +			pr_warning("ipt_connbytes: Force enabling CT accounting\n");
> +		nf_ct_set_acct(ct, true);
> +	}

This should be checked once the rule is added in ->checkentry(),
not once for every packet.