Add real default to NFLOG qthreshold

Add real default to NFLOG qthreshold

[Eric Leblond]

Hi,

The goal of this patchset is to fix some issues with NFLOG. This target has a
qthreshold value that can be set from iptables or from libnetfilter_log. If the
qthreshold is set from libnetfilter_log and if the rule does not set the value,
NFLOG should use the instance default. This is not the case, because iptables
set the qthreshold to 1 by default and this is used as a user-defined value by
kernel. This patchset fixes the issue by using the qthreshold as default for
iptables:
 - netfilter 1/2: if set per-rule qthreshold overrides per-instance
 - iptables: xt_NFLOG: Set default NFLOG qthreshold to 0

This patchset also fixes an issue with definition of timeout in the logging
queue. It fixes a computation error and change the unit to millisecond which
is a more convenient as the targeted 1/100 sec:
 - netfilter 2/2: fix nflog timeout handling

BR,
--
Eric Leblond <eric@inl.fr>
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/

[PATCH 1/2] netfilter: if set per-rule qthreshold overrides per-instance

[Eric Leblond]

In NFLOG the per-rule qthreshold should overrides per-instance only
it is set. With current code, the per-rule qthreshold is 1 if not set
and it overrides the per-instance qthreshold.

This patch modifies the default xt_NFLOG threshold from 1 to
0. Thus a value of 0 means there is no per-rule setting and the instance
parameter has to apply.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/linux/netfilter/xt_NFLOG.h |    2 +-
 net/netfilter/nfnetlink_log.c      |    6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/netfilter/xt_NFLOG.h b/include/linux/netfilter/xt_NFLOG.h
index cdcd0ed..4b36aeb 100644
--- a/include/linux/netfilter/xt_NFLOG.h
+++ b/include/linux/netfilter/xt_NFLOG.h
@@ -2,7 +2,7 @@
 #define _XT_NFLOG_TARGET
 
 #define XT_NFLOG_DEFAULT_GROUP		0x1
-#define XT_NFLOG_DEFAULT_THRESHOLD	1
+#define XT_NFLOG_DEFAULT_THRESHOLD	0
 
 #define XT_NFLOG_MASK			0x0
 
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index fe52265..88b455e 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -590,8 +590,10 @@ nfulnl_log_packet(u_int8_t pf,
 
 	qthreshold = inst->qthreshold;
 	/* per-rule qthreshold overrides per-instance */
-	if (qthreshold > li->u.ulog.qthreshold)
-		qthreshold = li->u.ulog.qthreshold;
+	if (li->u.ulog.qthreshold)
+		if (qthreshold > li->u.ulog.qthreshold)
+			qthreshold = li->u.ulog.qthreshold;
+
 
 	switch (inst->copy_mode) {
 	case NFULNL_COPY_META:
-- 
1.6.1

Re: [PATCH 1/2] netfilter: if set per-rule qthreshold overrides per-instance

[Patrick McHardy]

Eric Leblond wrote:
> In NFLOG the per-rule qthreshold should overrides per-instance only
> it is set. With current code, the per-rule qthreshold is 1 if not set
> and it overrides the per-instance qthreshold.
> 
> This patch modifies the default xt_NFLOG threshold from 1 to
> 0. Thus a value of 0 means there is no per-rule setting and the instance
> parameter has to apply.

This makes much more sense :) Applied, thanks.

[iptables] xt_NFLOG: Set default NFLOG qthreshold to 0

[Eric Leblond]

By setting default NFLOG qthreshold to 0, userspace does not overwrite
the per-instance value.
---
 include/linux/netfilter/xt_NFLOG.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/netfilter/xt_NFLOG.h b/include/linux/netfilter/xt_NFLOG.h
index cdcd0ed..4b36aeb 100644
--- a/include/linux/netfilter/xt_NFLOG.h
+++ b/include/linux/netfilter/xt_NFLOG.h
@@ -2,7 +2,7 @@
 #define _XT_NFLOG_TARGET
 
 #define XT_NFLOG_DEFAULT_GROUP		0x1
-#define XT_NFLOG_DEFAULT_THRESHOLD	1
+#define XT_NFLOG_DEFAULT_THRESHOLD	0
 
 #define XT_NFLOG_MASK			0x0
 
-- 
1.6.1

Re: [iptables] xt_NFLOG: Set default NFLOG qthreshold to 0

[Patrick McHardy]

Eric Leblond wrote:
> By setting default NFLOG qthreshold to 0, userspace does not overwrite
> the per-instance value.

Also applied, thanks.

[PATCH 2/2] netfilter: fix nflog timeout handling

[Eric Leblond]

NFLOG timeout was computed in timer by doing:
 flushtimeout*HZ/100
Default value of flushtimeout was HZ (for 1 second delay). This was
wrong for non 100HZ computer.

This patch modifies the delay computation by using:
 flushtimeout*HZ/1000
delay and set default value of flushtimeout to 1000 (as 1/100 sec is not
a common unit).

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 net/netfilter/nfnetlink_log.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 88b455e..b5086fa 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -39,7 +39,7 @@
 #endif
 
 #define NFULNL_NLBUFSIZ_DEFAULT	NLMSG_GOODSIZE
-#define NFULNL_TIMEOUT_DEFAULT 	HZ	/* every second */
+#define NFULNL_TIMEOUT_DEFAULT 	1000	/* every second */
 #define NFULNL_QTHRESH_DEFAULT 	100	/* 100 packets */
 #define NFULNL_COPY_RANGE_MAX	0xFFFF	/* max packet size is limited by 16-bit struct nfattr nfa_len field */
 
@@ -639,7 +639,7 @@ nfulnl_log_packet(u_int8_t pf,
 	 * is no chance of a race here */
 	else if (!timer_pending(&inst->timer)) {
 		instance_get(inst);
-		inst->timer.expires = jiffies + (inst->flushtimeout*HZ/100);
+		inst->timer.expires = jiffies + (inst->flushtimeout*HZ/1000);
 		add_timer(&inst->timer);
 	}
 
-- 
1.6.1

Re: [PATCH 2/2] netfilter: fix nflog timeout handling

[Patrick McHardy]

Eric Leblond wrote:
> NFLOG timeout was computed in timer by doing:
>  flushtimeout*HZ/100
> Default value of flushtimeout was HZ (for 1 second delay). This was
> wrong for non 100HZ computer.
> 
> This patch modifies the delay computation by using:
>  flushtimeout*HZ/1000
> delay and set default value of flushtimeout to 1000 (as 1/100 sec is not
> a common unit).

I think we should adjust the default instead as that doesn't
affect the units exposed to userspace (which is 10ms).