[PATCH] Don't call nf_log_packet in NFLOG module.

[PATCH] Don't call nf_log_packet in NFLOG module.

[Eric Leblond]

This patch modifies xt_NFLOG to suppress the call to nf_log_packet()
function. The call of this wrapper in xt_NFLOG was causing NFLOG to
use the first initialized module. Thus, if ipt_ULOG is loaded before
nfnetlink_log all NFLOG rules are treated as plain LOG rules.

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

diff --git a/include/net/netfilter/nfnetlink_log.h b/include/net/netfilter/nfnetlink_log.h
new file mode 100644
index 0000000..c920259
--- /dev/null
+++ b/include/net/netfilter/nfnetlink_log.h
@@ -0,0 +1,13 @@
+#ifndef _KER_NFNETLINK_LOG_H
+#define _KER_NFNETLINK_LOG_H
+
+void
+nfulnl_log_packet(unsigned int pf,
+		  unsigned int hooknum,
+		  const struct sk_buff *skb,
+		  const struct net_device *in,
+		  const struct net_device *out,
+		  const struct nf_loginfo *li_user,
+		  const char *prefix);
+
+#endif /* _KER_NFNETLINK_LOG_H */
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 9a35b57..494329a 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -533,7 +533,7 @@ static struct nf_loginfo default_loginfo = {
 };
 
 /* log handler for internal netfilter logging api */
-static void
+void
 nfulnl_log_packet(unsigned int pf,
 		  unsigned int hooknum,
 		  const struct sk_buff *skb,
@@ -648,6 +648,7 @@ alloc_failure:
 	/* FIXME: statistics */
 	goto unlock_and_release;
 }
+EXPORT_SYMBOL(nfulnl_log_packet);
 
 static int
 nfulnl_rcv_nl_event(struct notifier_block *this,
diff --git a/net/netfilter/xt_NFLOG.c b/net/netfilter/xt_NFLOG.c
index 19ae8ef..bb94aaa 100644
--- a/net/netfilter/xt_NFLOG.c
+++ b/net/netfilter/xt_NFLOG.c
@@ -13,6 +13,7 @@
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_NFLOG.h>
 #include <net/netfilter/nf_log.h>
+#include <net/netfilter/nfnetlink_log.h>
 
 MODULE_AUTHOR("Patrick McHardy <kaber@trash.net>");
 MODULE_DESCRIPTION("Xtables: packet logging to netlink using NFLOG");
@@ -33,8 +34,8 @@ nflog_tg(struct sk_buff *skb, const struct net_device *in,
 	li.u.ulog.group	     = info->group;
 	li.u.ulog.qthreshold = info->threshold;
 
-	nf_log_packet(target->family, hooknum, skb, in, out, &li,
-		      "%s", info->prefix);
+	nfulnl_log_packet(target->family, hooknum, skb, in, out, &li,
+			  info->prefix);
 	return XT_CONTINUE;
 }
 
-- 
1.5.6.5

Re: [PATCH] Don't call nf_log_packet in NFLOG module.

[Patrick McHardy]

Eric Leblond wrote:
> This patch modifies xt_NFLOG to suppress the call to nf_log_packet()
> function. The call of this wrapper in xt_NFLOG was causing NFLOG to
> use the first initialized module. Thus, if ipt_ULOG is loaded before
> nfnetlink_log all NFLOG rules are treated as plain LOG rules.

Oops, this slipped through somehow. It has been an intentional
decision to use the registered logging backends though, just changing
it to unconditionally use nfnetlink_log only solves the problem
partially.

The main problem is that the policy which backend to use is defined
by module load order, which is obviously a pretty bad idea. This does
not only affect xt_NFLOG, but also internal conntrack logging and
anything else we might want to use this for in the future.

So I think what we should do instead is introduce a proper way to
select among the logging backends. We could introduce a global
policy, or split by subsystem, which would currently be just
"conntrack" and "NFLOG".

And perhaps we should start getting rid of ULOG completely to
simplify this stuff.

But I'm open to being convinced of better ways :)

Re: [PATCH] Don't call nf_log_packet in NFLOG module.

[Eric Leblond]

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

Hi,

On Wednesday, 2008 October  8 at 15:02:54 +0200, Patrick McHardy wrote:
> Eric Leblond wrote:
>> This patch modifies xt_NFLOG to suppress the call to nf_log_packet()
>> function. The call of this wrapper in xt_NFLOG was causing NFLOG to
>> use the first initialized module. Thus, if ipt_ULOG is loaded before
>> nfnetlink_log all NFLOG rules are treated as plain LOG rules.
>
> Oops, this slipped through somehow. It has been an intentional
> decision to use the registered logging backends though, just changing
> it to unconditionally use nfnetlink_log only solves the problem
> partially.

Hmm, looks like my explanation is not correct. This patch fixes the
following bug :

modprobe ipt_LOG
modprobe nfnetlink_log
iptables -A OUTPUT -j NFLOG
Then : logged packet are treated as packet reaching the LOG target.

> The main problem is that the policy which backend to use is defined
> by module load order, which is obviously a pretty bad idea. This does
> not only affect xt_NFLOG, but also internal conntrack logging and
> anything else we might want to use this for in the future.
>
> So I think what we should do instead is introduce a proper way to
> select among the logging backends. We could introduce a global
> policy, or split by subsystem, which would currently be just
> "conntrack" and "NFLOG".

Yes, I currently working on doing that. I plan to send it in an other
patch. I've send the following patch alone to fix this weird NFLOG
target working as LOG target problem.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Don't call nf_log_packet in NFLOG module.

[Patrick McHardy]

Eric Leblond wrote:
> Hi,
> 
> On Wednesday, 2008 October  8 at 15:02:54 +0200, Patrick McHardy wrote:
>> Eric Leblond wrote:
>>> This patch modifies xt_NFLOG to suppress the call to nf_log_packet()
>>> function. The call of this wrapper in xt_NFLOG was causing NFLOG to
>>> use the first initialized module. Thus, if ipt_ULOG is loaded before
>>> nfnetlink_log all NFLOG rules are treated as plain LOG rules.
>> Oops, this slipped through somehow. It has been an intentional
>> decision to use the registered logging backends though, just changing
>> it to unconditionally use nfnetlink_log only solves the problem
>> partially.
> 
> Hmm, looks like my explanation is not correct. This patch fixes the
> following bug :
> 
> modprobe ipt_LOG
> modprobe nfnetlink_log
> iptables -A OUTPUT -j NFLOG
> Then : logged packet are treated as packet reaching the LOG target.

Yes, I know. That behaviour was intentional in the original design.
But I agree, it sucks, so I'll apply your patch.

>> The main problem is that the policy which backend to use is defined
>> by module load order, which is obviously a pretty bad idea. This does
>> not only affect xt_NFLOG, but also internal conntrack logging and
>> anything else we might want to use this for in the future.
>>
>> So I think what we should do instead is introduce a proper way to
>> select among the logging backends. We could introduce a global
>> policy, or split by subsystem, which would currently be just
>> "conntrack" and "NFLOG".
> 
> Yes, I currently working on doing that. I plan to send it in an other
> patch. I've send the following patch alone to fix this weird NFLOG
> target working as LOG target problem.

Great, thanks.

Re: [PATCH] Don't call nf_log_packet in NFLOG module.

[Patrick McHardy]

Eric Leblond wrote:
> This patch modifies xt_NFLOG to suppress the call to nf_log_packet()
> function. The call of this wrapper in xt_NFLOG was causing NFLOG to
> use the first initialized module. Thus, if ipt_ULOG is loaded before
> nfnetlink_log all NFLOG rules are treated as plain LOG rules.

The patch doesn't apply because of clashes with Jan's argument
encapsulation patches. Could you rediff again Dave's net-next tree
please? Thanks!

[PATCH] Don't call nf_log_packet in NFLOG module.

[Eric Leblond]

This patch modifies xt_NFLOG to suppress the call to nf_log_packet()
function. The call of this wrapper in xt_NFLOG was causing NFLOG to
use the first initialized module. Thus, if ipt_ULOG is loaded before
nfnetlink_log all NFLOG rules are treated as plain LOG rules.

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

diff --git a/include/net/netfilter/nfnetlink_log.h b/include/net/netfilter/nfnetlink_log.h
new file mode 100644
index 0000000..9b67f94
--- /dev/null
+++ b/include/net/netfilter/nfnetlink_log.h
@@ -0,0 +1,14 @@
+#ifndef _KER_NFNETLINK_LOG_H
+#define _KER_NFNETLINK_LOG_H
+
+void
+nfulnl_log_packet(unsigned int pf,
+		  unsigned int hooknum,
+		  const struct sk_buff *skb,
+		  const struct net_device *in,
+		  const struct net_device *out,
+		  const struct nf_loginfo *li_user,
+		  const char *prefix);
+
+#endif /* _KER_NFNETLINK_LOG_H */
+
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 41e0105..a7ff7b9 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -533,7 +533,7 @@ static struct nf_loginfo default_loginfo = {
 };
 
 /* log handler for internal netfilter logging api */
-static void
+void
 nfulnl_log_packet(u_int8_t pf,
 		  unsigned int hooknum,
 		  const struct sk_buff *skb,
@@ -648,6 +648,7 @@ alloc_failure:
 	/* FIXME: statistics */
 	goto unlock_and_release;
 }
+EXPORT_SYMBOL(nfulnl_log_packet);
 
 static int
 nfulnl_rcv_nl_event(struct notifier_block *this,
diff --git a/net/netfilter/xt_NFLOG.c b/net/netfilter/xt_NFLOG.c
index 50e3a52..a57c5cf 100644
--- a/net/netfilter/xt_NFLOG.c
+++ b/net/netfilter/xt_NFLOG.c
@@ -13,6 +13,7 @@
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_NFLOG.h>
 #include <net/netfilter/nf_log.h>
+#include <net/netfilter/nfnetlink_log.h>
 
 MODULE_AUTHOR("Patrick McHardy <kaber@trash.net>");
 MODULE_DESCRIPTION("Xtables: packet logging to netlink using NFLOG");
@@ -31,8 +32,8 @@ nflog_tg(struct sk_buff *skb, const struct xt_target_param *par)
 	li.u.ulog.group	     = info->group;
 	li.u.ulog.qthreshold = info->threshold;
 
-	nf_log_packet(par->family, par->hooknum, skb, par->in,
-	              par->out, &li, "%s", info->prefix);
+	nfulnl_log_packet(par->family, par->hooknum, skb, par->in,
+			  par->out, &li, info->prefix);
 	return XT_CONTINUE;
 }
 
-- 
1.5.4.3

Re: [PATCH] Don't call nf_log_packet in NFLOG module.

[Patrick McHardy]

Eric Leblond wrote:
> This patch modifies xt_NFLOG to suppress the call to nf_log_packet()
> function. The call of this wrapper in xt_NFLOG was causing NFLOG to
> use the first initialized module. Thus, if ipt_ULOG is loaded before
> nfnetlink_log all NFLOG rules are treated as plain LOG rules.

I've changed the EXPORT_SYMBOL to EXPORT_SYMBOL_GPL and applied it,
thanks.