[netfilter 0/5] nf_log refactoring

[netfilter 0/5] nf_log refactoring

[Eric Leblond]

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

Hi,

As discussed during netfilter workshop, I've worked on changing the way
netfilter system logger can be used. This patchset has been tested and
seems to work fine but I'm not yet familiar things like rcu and mistakes
can have been done. 

Background:
Some Netfilter components are using nf_log_packet() to send information
packet to userspace. This is mainly the case of connection tracking
modules which are using this to log invalid packets. Currently the first
loaded module wins the logger function race (loggers are stored in a
per-protocol array of function pointer).
nfnetlink_log has introduced a minor difference because it has a
unbinding and binding capability. But, this is the only logging module
with this capability and there is a issue here as this is currently not
possible to switch back to another module when nfnetlink_log has been
choosen (unbind operation leads to NONE as existence of other module has
been forgotten).

Patchset description:
The goal of this patchset is to replace the first registered win
strategy with something more flexible and intuitive. It thus modify the
binding strategy by providing a register and a bind operation. A module
has first to register and then it can bind to a given pf family. The
registration phase adds the logger structure to a per-protocol chained
list. The binding is pure nf_log operation and thus it will be possible
to change at will the used logger without direct interaction with
logging modules. The first three patches implements this:
 - netfilter: use a linked list of loggers.
 - netfilter: suppress nf_log_unregister_pf function.
 - netfilter: convert logger modules to new API.
The fourth patch modifies the output of /proc/net/netfilter/nf_log to
also give the list of registered logger for a protocol. And the fith
patch fixes the registration problem by adding support of modification
via sysctl of the logger fonction:
 - netfilter: print the list of register loggers.
 - netfilter: sysctl support of logger choice.

List of patches:
 - netfilter: use a linked list of loggers.
 - netfilter: suppress nf_log_unregister_pf function.
 - netfilter: convert logger modules to new API.
 - netfilter: print the list of register loggers.
 - netfilter: sysctl support of logger choice.

Patchset statistics: 
 include/linux/sysctl.h         |    1 +
 include/net/netfilter/nf_log.h |   11 ++-
 net/ipv4/netfilter/ipt_LOG.c   |    4 +-
 net/ipv4/netfilter/ipt_ULOG.c  |    4 +-
 net/ipv6/netfilter/ip6t_LOG.c  |    1 +
 net/netfilter/nf_log.c         |  201 ++++++++++++++++++++++++++++++++-------
 net/netfilter/nfnetlink_log.c  |   11 ++-
 7 files changed, 189 insertions(+), 44 deletions(-)

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


[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 1/5] netfilter: use a linked list of loggers.

[Eric Leblond]

This patch modifies nf_log to use a linked list of loggers for each
protocol. It also separate registration and binding. To be used as
logging module, a module has to register calling nf_log_register()
and to bind to a protocol it has to call nf_log_bind_pf().

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |   10 +++-
 net/netfilter/nf_log.c         |  107 +++++++++++++++++++++++++++++++---------
 2 files changed, 91 insertions(+), 26 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 7182c06..08b4f18 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -1,6 +1,8 @@
 #ifndef _NF_LOG_H
 #define _NF_LOG_H
 
+#include <linux/netfilter.h>
+
 /* those NF_LOG_* defines and struct nf_loginfo are legacy definitios that will
  * disappear once iptables is replaced with pkttables.  Please DO NOT use them
  * for any new code! */
@@ -40,13 +42,17 @@ struct nf_logger {
 	struct module	*me;
 	nf_logfn 	*logfn;
 	char		*name;
+	struct list_head	list[NFPROTO_NUMPROTO];
 };
 
 /* Function to register/unregister log function. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger);
-void nf_log_unregister(const struct nf_logger *logger);
+int nf_log_register(u_int8_t pf, struct nf_logger *logger);
+void nf_log_unregister(struct nf_logger *logger);
 void nf_log_unregister_pf(u_int8_t pf);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
+int nf_log_unbind_pf(u_int8_t pf);
+
 /* Calls the registered backend logging function */
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index fa8ae5d..43871da 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,13 +14,25 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
+#define NFLOGGER_NAME_LEN		12
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
-static DEFINE_MUTEX(nf_log_mutex);
+static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
+static DEFINE_SPINLOCK(nf_log_lock);
 
-/* return EBUSY if somebody else is registered, EEXIST if the same logger
- * is registred, 0 on success. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
+static struct nf_logger *__find_logger(int pf, const char *str_logger)
+{
+	struct nf_logger *t;
+
+	list_for_each_entry(t, &nf_loggers_l[pf], list[pf])
+		if (!strnicmp(str_logger, t->name, NFLOGGER_NAME_LEN))
+			return t;
+
+	return NULL;
+}
+
+/* return EEXIST if the same logger is registred, 0 on success. */
+int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	int ret;
 
@@ -29,50 +41,91 @@ int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
 
 	/* Any setup of logging members must be done before
 	 * substituting pointer. */
-	ret = mutex_lock_interruptible(&nf_log_mutex);
-	if (ret < 0)
-		return ret;
-
-	if (!nf_loggers[pf])
-		rcu_assign_pointer(nf_loggers[pf], logger);
-	else if (nf_loggers[pf] == logger)
-		ret = -EEXIST;
-	else
-		ret = -EBUSY;
-
-	mutex_unlock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
+
+	ret = -EEXIST;
+	if (pf == NFPROTO_UNSPEC) {
+		int i;
+		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+			if (__find_logger(i, logger->name) == NULL) {
+				ret = 0;
+				INIT_LIST_HEAD(&(logger->list[i]));
+				list_add_tail(&(logger->list[i]),
+						  &(nf_loggers_l[i]));
+			}
+		}
+	} else {
+		if (__find_logger(pf, logger->name) == NULL) {
+			ret = 0;
+			INIT_LIST_HEAD(&(logger->list[pf]));
+			/* register at end of list to honor first register win */
+			list_add_tail(&(logger->list[pf]),
+					  &nf_loggers_l[pf]);
+		}
+	}
+
+	spin_unlock_bh(&nf_log_lock);
 	return ret;
 }
 EXPORT_SYMBOL(nf_log_register);
 
 void nf_log_unregister_pf(u_int8_t pf)
 {
+	struct nf_logger *e;
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return;
-	mutex_lock(&nf_log_mutex);
-	rcu_assign_pointer(nf_loggers[pf], NULL);
-	mutex_unlock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
+	list_for_each_entry(e, &(nf_loggers_l[pf]), list[pf])
+		list_del(&(e->list[pf]));
+	spin_unlock_bh(&nf_log_lock);
 
 	/* Give time to concurrent readers. */
 	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unregister_pf);
 
-void nf_log_unregister(const struct nf_logger *logger)
+void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
+	struct nf_logger *llog;
 
-	mutex_lock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		if (nf_loggers[i] == logger)
-			rcu_assign_pointer(nf_loggers[i], NULL);
+		llog = __find_logger(i, logger->name);
+		if (llog) {
+			struct nf_logger *c_logger = rcu_dereference(nf_loggers[i]);
+			if (c_logger == llog)
+				rcu_assign_pointer(nf_loggers[i], NULL);
+			list_del(&(logger->list[i]));
+		}
 	}
-	mutex_unlock(&nf_log_mutex);
+	spin_unlock_bh(&nf_log_lock);
 
 	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unregister);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger)
+{
+	if (__find_logger(pf, logger->name) == NULL)
+		return -1;
+
+	spin_lock_bh(&nf_log_lock);
+	rcu_assign_pointer(nf_loggers[pf], logger);
+	spin_unlock_bh(&nf_log_lock);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_bind_pf);
+
+int nf_log_unbind_pf(u_int8_t pf)
+{
+	spin_lock_bh(&nf_log_lock);
+	rcu_assign_pointer(nf_loggers[pf], NULL);
+	spin_unlock_bh(&nf_log_lock);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_unbind_pf);
+
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
 		   const struct sk_buff *skb,
@@ -163,10 +216,16 @@ static const struct file_operations nflog_file_ops = {
 
 int __init netfilter_log_init(void)
 {
+	int i;
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		INIT_LIST_HEAD(&(nf_loggers_l[i]));
+	}
+
 	return 0;
 }
-- 
1.5.6.3

[PATCH 2/5] netfilter: suppress nf_log_unregister_pf function.

[Eric Leblond]

The nf_log_unregister_pf() function is not used anymore as it was
corresponding to the unbind operation which is now done via
nflog_unbind_pf().

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |    1 -
 net/netfilter/nf_log.c         |   15 ---------------
 2 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 08b4f18..ccaa43a 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -48,7 +48,6 @@ struct nf_logger {
 /* Function to register/unregister log function. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger);
 void nf_log_unregister(struct nf_logger *logger);
-void nf_log_unregister_pf(u_int8_t pf);
 
 int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
 int nf_log_unbind_pf(u_int8_t pf);
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 43871da..5d6748c 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -69,21 +69,6 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 }
 EXPORT_SYMBOL(nf_log_register);
 
-void nf_log_unregister_pf(u_int8_t pf)
-{
-	struct nf_logger *e;
-	if (pf >= ARRAY_SIZE(nf_loggers))
-		return;
-	spin_lock_bh(&nf_log_lock);
-	list_for_each_entry(e, &(nf_loggers_l[pf]), list[pf])
-		list_del(&(e->list[pf]));
-	spin_unlock_bh(&nf_log_lock);
-
-	/* Give time to concurrent readers. */
-	synchronize_rcu();
-}
-EXPORT_SYMBOL(nf_log_unregister_pf);
-
 void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
-- 
1.5.6.3

[PATCH 3/5] netfilter: convert logger modules to new API.

[Eric Leblond]

This patch converts the logging modules to the new API. For all modules
but nfnetlink_log the only change is a call to nf_log_register() and
nf_log_bind_pf instead of a simple call of nf_log_register(). For nfnetlink_log,
the patch simply switchs call to register functions to call to bind function
and adds a call to nflog_register during init. Backward compatibility
should be preserved by this patch.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 net/ipv4/netfilter/ipt_LOG.c  |    4 +++-
 net/ipv4/netfilter/ipt_ULOG.c |    4 +++-
 net/ipv6/netfilter/ip6t_LOG.c |    1 +
 net/netfilter/nfnetlink_log.c |   11 ++++++++---
 4 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c
index 4614a69..f1f0338 100644
--- a/net/ipv4/netfilter/ipt_LOG.c
+++ b/net/ipv4/netfilter/ipt_LOG.c
@@ -464,7 +464,7 @@ static struct xt_target log_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static const struct nf_logger ipt_log_logger ={
+static struct nf_logger ipt_log_logger ={
 	.name		= "ipt_LOG",
 	.logfn		= &ipt_log_packet,
 	.me		= THIS_MODULE,
@@ -477,7 +477,9 @@ static int __init log_tg_init(void)
 	ret = xt_register_target(&log_tg_reg);
 	if (ret < 0)
 		return ret;
+
 	nf_log_register(NFPROTO_IPV4, &ipt_log_logger);
+	nf_log_bind_pf(NFPROTO_IPV4, &ipt_log_logger);
 	return 0;
 }
 
diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index 18a2826..b895b65 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -411,8 +411,10 @@ static int __init ulog_tg_init(void)
 		netlink_kernel_release(nflognl);
 		return ret;
 	}
-	if (nflog)
+	if (nflog) {
 		nf_log_register(NFPROTO_IPV4, &ipt_ulog_logger);
+		nf_log_bind_pf(NFPROTO_IPV4, &ipt_ulog_logger);
+	}
 
 	return 0;
 }
diff --git a/net/ipv6/netfilter/ip6t_LOG.c b/net/ipv6/netfilter/ip6t_LOG.c
index 7c668c6..64042b3 100644
--- a/net/ipv6/netfilter/ip6t_LOG.c
+++ b/net/ipv6/netfilter/ip6t_LOG.c
@@ -491,6 +491,7 @@ static int __init log_tg6_init(void)
 	if (ret < 0)
 		return ret;
 	nf_log_register(NFPROTO_IPV6, &ip6t_logger);
+	nf_log_bind_pf(NFPROTO_IPV6, &ip6t_logger);
 	return 0;
 }
 
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 2770b4e..c18b809 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -690,7 +690,7 @@ nfulnl_recv_unsupp(struct sock *ctnl, struct sk_buff *skb,
 	return -ENOTSUPP;
 }
 
-static const struct nf_logger nfulnl_logger = {
+static struct nf_logger nfulnl_logger = {
 	.name	= "nfnetlink_log",
 	.logfn	= &nfulnl_log_packet,
 	.me	= THIS_MODULE,
@@ -722,9 +722,9 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		/* Commands without queue context */
 		switch (cmd->command) {
 		case NFULNL_CFG_CMD_PF_BIND:
-			return nf_log_register(pf, &nfulnl_logger);
+			return nf_log_bind_pf(pf, &nfulnl_logger);
 		case NFULNL_CFG_CMD_PF_UNBIND:
-			nf_log_unregister_pf(pf);
+			nf_log_unbind_pf(pf);
 			return 0;
 		}
 	}
@@ -949,6 +949,11 @@ static int __init nfnetlink_log_init(void)
 		goto cleanup_netlink_notifier;
 	}
 
+	if ((status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger)) < 0) {
+		printk(KERN_ERR "log: failed to register logger\n");
+		goto cleanup_netlink_notifier;
+	}
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nfnetlink_log", 0440,
 			 proc_net_netfilter, &nful_file_ops))
-- 
1.5.6.3

[PATCH 4/5] netfilter: print the list of register loggers.

[Eric Leblond]

This patch modifies the proc output to add display of registered
loggers. The content of /proc/net/netfilter/nf_log is modified. Instead
of displaying a protocol per line with format:
	proto:logger
it now displays:
	proto:logger (comma_separated_list_of_loggers)

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 5d6748c..ed03c20 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -167,13 +167,26 @@ static int seq_show(struct seq_file *s, void *v)
 {
 	loff_t *pos = v;
 	const struct nf_logger *logger;
+	struct nf_logger *t;
+	char loggers_string[256];
+	int len = 256;
 
 	logger = rcu_dereference(nf_loggers[*pos]);
 
+	memset(loggers_string, 0, len);
+	list_for_each_entry(t, &nf_loggers_l[*pos], list[*pos]) {
+		strncat(loggers_string, t->name, len);
+		strncat(loggers_string, ",", 1);
+		len -= strlen(t->name) + 1;
+	}
+	if (strlen(loggers_string))
+		loggers_string[strlen(loggers_string) -1] = 0;
+
 	if (!logger)
-		return seq_printf(s, "%2lld NONE\n", *pos);
+		return seq_printf(s, "%2lld NONE (%s)\n", *pos, loggers_string);
 
-	return seq_printf(s, "%2lld %s\n", *pos, logger->name);
+	return seq_printf(s, "%2lld %s (%s)\n", *pos, logger->name,
+			  loggers_string);
 }
 
 static const struct seq_operations nflog_seq_ops = {
-- 
1.5.6.3

[PATCH 5/5] netfilter: sysctl support of logger choice.

[Eric Leblond]

This patchs adds support of modification of the used logger via sysctl.
It can be used to change the logger to module that can not use the bind
operation (ipt_LOG and ipt_ULOG). For this purpose, it creates a
directory /proc/sys/net/netfilter/nf_log which contains a file
per-protocol. The content of the file is the name current logger (NONE if
not set) and a logger can be setup by simply echoing its name to the file.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/linux/sysctl.h |    1 +
 net/netfilter/nf_log.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 39d471d..aa11b1c 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -335,6 +335,7 @@ enum
 	NET_NF_CONNTRACK_FRAG6_LOW_THRESH=30,
 	NET_NF_CONNTRACK_FRAG6_HIGH_THRESH=31,
 	NET_NF_CONNTRACK_CHECKSUM=32,
+	NET_NF_LOG=33,
 };
 
 /* /proc/sys/net/ipv4 */
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index ed03c20..17698e3 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,7 +14,7 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
-#define NFLOGGER_NAME_LEN		12
+#define NFLOGGER_NAME_LEN		24
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
 static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
@@ -211,16 +211,88 @@ static const struct file_operations nflog_file_ops = {
 
 #endif /* PROC_FS */
 
+#ifdef CONFIG_SYSCTL
+struct ctl_path nf_log_sysctl_path[] = {
+	{ .procname = "net", .ctl_name = CTL_NET, },
+	{ .procname = "netfilter", .ctl_name = NET_NETFILTER, },
+	{ .procname = "nf_log", .ctl_name = NET_NF_LOG, },
+	{ }
+};
+
+static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
+static struct ctl_table_header *nf_log_dir_header;
+
+static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
+			 void *buffer, size_t *lenp, loff_t *ppos)
+{
+	const struct nf_logger *logger;
+	int r;
+	char ln_name[NFLOGGER_NAME_LEN];
+
+	rcu_read_lock();
+	if (write) {
+		logger = __find_logger(table->ctl_name, buffer);
+		if (logger == NULL) {
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+		r = nf_log_bind_pf(table->ctl_name, logger);
+	} else {
+		logger = rcu_dereference(nf_loggers[table->ctl_name]);
+		if (! logger)
+			table->data = "NONE";
+		else {
+			memcpy(ln_name, logger->name, NFLOGGER_NAME_LEN);
+			table->data = ln_name;
+		}
+		r = proc_dostring(table, write, filp, buffer, lenp, ppos);
+	}
+	rcu_read_unlock();
+
+	return r;
+}
+
+static int netfilter_log_sysctl_init(void)
+{
+	int i;
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		char *pr_name = kmalloc(3, GFP_KERNEL);
+		/* FIXME free at deinit but there is currently no deinit */
+		snprintf(pr_name, 3, "%d", i);
+		nf_log_sysctl_table[i].ctl_name	= i;
+		nf_log_sysctl_table[i].procname	= pr_name;
+		nf_log_sysctl_table[i].data = NULL;
+		nf_log_sysctl_table[i].maxlen = NFLOGGER_NAME_LEN * sizeof(char);
+		nf_log_sysctl_table[i].mode = 0644;
+		nf_log_sysctl_table[i].proc_handler = nf_log_proc_dostring;
+	}
+
+	nf_log_dir_header = register_sysctl_paths(nf_log_sysctl_path,
+				       nf_log_sysctl_table);
+
+	return 0;
+}
+#else
+static int netfilter_log_sysctl_init(void)
+{
+	return 0;
+}
+#endif /* CONFIG_SYSCTL */
 
 int __init netfilter_log_init(void)
 {
 	int i;
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
 
+	if (netfilter_log_sysctl_init() < 0)
+		return -1;
+
 	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
 		INIT_LIST_HEAD(&(nf_loggers_l[i]));
 	}
-- 
1.5.6.3

Re: [PATCH 3/5] netfilter: convert logger modules to new API.

[Eric Leblond]

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

Hi,
Le vendredi 02 janvier 2009 à 18:07 +0100, Eric Leblond a écrit :
> This patch converts the logging modules to the new API. For all modules
...
> and adds a call to nflog_register during init. Backward compatibility
> should be preserved by this patch.

I've discovered this is not the case, I will resend a reworked patchset
soon to provide a true backward compatibility.

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

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 1/6] netfilter: use a linked list of loggers.

[Eric Leblond]

This patch modifies nf_log to use a linked list of loggers for each
protocol. It also separate registration and binding. To be used as
logging module, a module has to register calling nf_log_register()
and to bind to a protocol it has to call nf_log_bind_pf().

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |   10 +++-
 net/netfilter/nf_log.c         |  107 +++++++++++++++++++++++++++++++---------
 2 files changed, 91 insertions(+), 26 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 7182c06..08b4f18 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -1,6 +1,8 @@
 #ifndef _NF_LOG_H
 #define _NF_LOG_H
 
+#include <linux/netfilter.h>
+
 /* those NF_LOG_* defines and struct nf_loginfo are legacy definitios that will
  * disappear once iptables is replaced with pkttables.  Please DO NOT use them
  * for any new code! */
@@ -40,13 +42,17 @@ struct nf_logger {
 	struct module	*me;
 	nf_logfn 	*logfn;
 	char		*name;
+	struct list_head	list[NFPROTO_NUMPROTO];
 };
 
 /* Function to register/unregister log function. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger);
-void nf_log_unregister(const struct nf_logger *logger);
+int nf_log_register(u_int8_t pf, struct nf_logger *logger);
+void nf_log_unregister(struct nf_logger *logger);
 void nf_log_unregister_pf(u_int8_t pf);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
+int nf_log_unbind_pf(u_int8_t pf);
+
 /* Calls the registered backend logging function */
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index fa8ae5d..8a5e4aa 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,13 +14,25 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
+#define NFLOGGER_NAME_LEN		12
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
-static DEFINE_MUTEX(nf_log_mutex);
+static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
+static DEFINE_SPINLOCK(nf_log_lock);
 
-/* return EBUSY if somebody else is registered, EEXIST if the same logger
- * is registred, 0 on success. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
+static struct nf_logger *__find_logger(int pf, const char *str_logger)
+{
+	struct nf_logger *t;
+
+	list_for_each_entry(t, &nf_loggers_l[pf], list[pf])
+		if (!strnicmp(str_logger, t->name, NFLOGGER_NAME_LEN))
+			return t;
+
+	return NULL;
+}
+
+/* return EEXIST if the same logger is registred, 0 on success. */
+int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	int ret;
 
@@ -29,50 +41,91 @@ int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
 
 	/* Any setup of logging members must be done before
 	 * substituting pointer. */
-	ret = mutex_lock_interruptible(&nf_log_mutex);
-	if (ret < 0)
-		return ret;
-
-	if (!nf_loggers[pf])
-		rcu_assign_pointer(nf_loggers[pf], logger);
-	else if (nf_loggers[pf] == logger)
-		ret = -EEXIST;
-	else
-		ret = -EBUSY;
-
-	mutex_unlock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
+
+	ret = -EEXIST;
+	if (pf == NFPROTO_UNSPEC) {
+		int i;
+		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+			if (__find_logger(i, logger->name) == NULL) {
+				ret = 0;
+				INIT_LIST_HEAD(&(logger->list[i]));
+				list_add_tail(&(logger->list[i]),
+						  &(nf_loggers_l[i]));
+			}
+		}
+	} else {
+		if (__find_logger(pf, logger->name) == NULL) {
+			ret = 0;
+			INIT_LIST_HEAD(&(logger->list[pf]));
+			/* register at end of list to honor first register win */
+			list_add_tail(&(logger->list[pf]),
+					  &nf_loggers_l[pf]);
+		}
+	}
+
+	spin_unlock_bh(&nf_log_lock);
 	return ret;
 }
 EXPORT_SYMBOL(nf_log_register);
 
 void nf_log_unregister_pf(u_int8_t pf)
 {
+	struct nf_logger *e;
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return;
-	mutex_lock(&nf_log_mutex);
-	rcu_assign_pointer(nf_loggers[pf], NULL);
-	mutex_unlock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
+	list_for_each_entry(e, &(nf_loggers_l[pf]), list[pf])
+		list_del(&(e->list[pf]));
+	spin_unlock_bh(&nf_log_lock);
 
 	/* Give time to concurrent readers. */
 	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unregister_pf);
 
-void nf_log_unregister(const struct nf_logger *logger)
+void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
+	struct nf_logger *llog;
 
-	mutex_lock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		if (nf_loggers[i] == logger)
-			rcu_assign_pointer(nf_loggers[i], NULL);
+		llog = __find_logger(i, logger->name);
+		if (llog) {
+			const struct nf_logger *c_logger = rcu_dereference(nf_loggers[i]);
+			if (c_logger == llog)
+				rcu_assign_pointer(nf_loggers[i], NULL);
+			list_del(&(logger->list[i]));
+		}
 	}
-	mutex_unlock(&nf_log_mutex);
+	spin_unlock_bh(&nf_log_lock);
 
 	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unregister);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger)
+{
+	if (__find_logger(pf, logger->name) == NULL)
+		return -1;
+
+	spin_lock_bh(&nf_log_lock);
+	rcu_assign_pointer(nf_loggers[pf], logger);
+	spin_unlock_bh(&nf_log_lock);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_bind_pf);
+
+int nf_log_unbind_pf(u_int8_t pf)
+{
+	spin_lock_bh(&nf_log_lock);
+	rcu_assign_pointer(nf_loggers[pf], NULL);
+	spin_unlock_bh(&nf_log_lock);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_unbind_pf);
+
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
 		   const struct sk_buff *skb,
@@ -163,10 +216,16 @@ static const struct file_operations nflog_file_ops = {
 
 int __init netfilter_log_init(void)
 {
+	int i;
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		INIT_LIST_HEAD(&(nf_loggers_l[i]));
+	}
+
 	return 0;
 }
-- 
1.6.1

[PATCH 2/6] netfilter: suppress nf_log_unregister_pf function.

[Eric Leblond]

The nf_log_unregister_pf() function is not used anymore as it was
corresponding to the unbind operation which is now done via
nflog_unbind_pf().

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |    1 -
 net/netfilter/nf_log.c         |   15 ---------------
 2 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 08b4f18..ccaa43a 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -48,7 +48,6 @@ struct nf_logger {
 /* Function to register/unregister log function. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger);
 void nf_log_unregister(struct nf_logger *logger);
-void nf_log_unregister_pf(u_int8_t pf);
 
 int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
 int nf_log_unbind_pf(u_int8_t pf);
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 8a5e4aa..a1dcd46 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -69,21 +69,6 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 }
 EXPORT_SYMBOL(nf_log_register);
 
-void nf_log_unregister_pf(u_int8_t pf)
-{
-	struct nf_logger *e;
-	if (pf >= ARRAY_SIZE(nf_loggers))
-		return;
-	spin_lock_bh(&nf_log_lock);
-	list_for_each_entry(e, &(nf_loggers_l[pf]), list[pf])
-		list_del(&(e->list[pf]));
-	spin_unlock_bh(&nf_log_lock);
-
-	/* Give time to concurrent readers. */
-	synchronize_rcu();
-}
-EXPORT_SYMBOL(nf_log_unregister_pf);
-
 void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
-- 
1.6.1

[PATCH 3/6] netfilter: bind at registration if no logger is already set.

[Eric Leblond]

This patch adds a bind operation when registration is done and when
there is currently no logger defined for the protocol. This emulates the
first registered wins policy and thus achieves backward compatibility.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index a1dcd46..1655a1f 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -35,6 +35,7 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	int ret;
+	const struct nf_logger *llog;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return -EINVAL;
@@ -58,13 +59,17 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 		if (__find_logger(pf, logger->name) == NULL) {
 			ret = 0;
 			INIT_LIST_HEAD(&(logger->list[pf]));
-			/* register at end of list to honor first register win */
 			list_add_tail(&(logger->list[pf]),
 					  &nf_loggers_l[pf]);
+			llog = rcu_dereference(nf_loggers[pf]);
+			if (llog == NULL)
+				rcu_assign_pointer(nf_loggers[pf], logger);
 		}
 	}
 
 	spin_unlock_bh(&nf_log_lock);
+
+	synchronize_rcu();
 	return ret;
 }
 EXPORT_SYMBOL(nf_log_register);
-- 
1.6.1

[PATCH 4/6] netfilter: convert logger modules to new API.

[Eric Leblond]

This patch converts the logging modules to the new API. For nfnetlink_log,
it simply switchs call to register functions to call to bind function and
adds a call to nf_log_register() during init. For other modules, it just
remove a const flag from the logger structure.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 net/ipv4/netfilter/ipt_LOG.c  |    2 +-
 net/ipv6/netfilter/ip6t_LOG.c |    2 +-
 net/netfilter/nfnetlink_log.c |   11 ++++++++---
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c
index 4614a69..6c94e4e 100644
--- a/net/ipv4/netfilter/ipt_LOG.c
+++ b/net/ipv4/netfilter/ipt_LOG.c
@@ -464,7 +464,7 @@ static struct xt_target log_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static const struct nf_logger ipt_log_logger ={
+static struct nf_logger ipt_log_logger ={
 	.name		= "ipt_LOG",
 	.logfn		= &ipt_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/ipv6/netfilter/ip6t_LOG.c b/net/ipv6/netfilter/ip6t_LOG.c
index 7c668c6..c80978e 100644
--- a/net/ipv6/netfilter/ip6t_LOG.c
+++ b/net/ipv6/netfilter/ip6t_LOG.c
@@ -477,7 +477,7 @@ static struct xt_target log_tg6_reg __read_mostly = {
 	.me 		= THIS_MODULE,
 };
 
-static const struct nf_logger ip6t_logger = {
+static struct nf_logger ip6t_logger = {
 	.name		= "ip6t_LOG",
 	.logfn		= &ip6t_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 2770b4e..c18b809 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -690,7 +690,7 @@ nfulnl_recv_unsupp(struct sock *ctnl, struct sk_buff *skb,
 	return -ENOTSUPP;
 }
 
-static const struct nf_logger nfulnl_logger = {
+static struct nf_logger nfulnl_logger = {
 	.name	= "nfnetlink_log",
 	.logfn	= &nfulnl_log_packet,
 	.me	= THIS_MODULE,
@@ -722,9 +722,9 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		/* Commands without queue context */
 		switch (cmd->command) {
 		case NFULNL_CFG_CMD_PF_BIND:
-			return nf_log_register(pf, &nfulnl_logger);
+			return nf_log_bind_pf(pf, &nfulnl_logger);
 		case NFULNL_CFG_CMD_PF_UNBIND:
-			nf_log_unregister_pf(pf);
+			nf_log_unbind_pf(pf);
 			return 0;
 		}
 	}
@@ -949,6 +949,11 @@ static int __init nfnetlink_log_init(void)
 		goto cleanup_netlink_notifier;
 	}
 
+	if ((status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger)) < 0) {
+		printk(KERN_ERR "log: failed to register logger\n");
+		goto cleanup_netlink_notifier;
+	}
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nfnetlink_log", 0440,
 			 proc_net_netfilter, &nful_file_ops))
-- 
1.6.1

[PATCH 5/6] netfilter: print the list of register loggers.

[Eric Leblond]

This patch modifies the proc output to add display of registered
loggers. The content of /proc/net/netfilter/nf_log is modified. Instead
of displaying a protocol per line with format:
	proto:logger
it now displays:
	proto:logger (comma_separated_list_of_loggers)

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 1655a1f..ee08b62 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -172,13 +172,26 @@ static int seq_show(struct seq_file *s, void *v)
 {
 	loff_t *pos = v;
 	const struct nf_logger *logger;
+	struct nf_logger *t;
+	char loggers_string[256];
+	int len = 256;
 
 	logger = rcu_dereference(nf_loggers[*pos]);
 
+	memset(loggers_string, 0, len);
+	list_for_each_entry(t, &nf_loggers_l[*pos], list[*pos]) {
+		strncat(loggers_string, t->name, len);
+		strncat(loggers_string, ",", 1);
+		len -= strlen(t->name) + 1;
+	}
+	if (strlen(loggers_string))
+		loggers_string[strlen(loggers_string) -1] = 0;
+
 	if (!logger)
-		return seq_printf(s, "%2lld NONE\n", *pos);
+		return seq_printf(s, "%2lld NONE (%s)\n", *pos, loggers_string);
 
-	return seq_printf(s, "%2lld %s\n", *pos, logger->name);
+	return seq_printf(s, "%2lld %s (%s)\n", *pos, logger->name,
+			  loggers_string);
 }
 
 static const struct seq_operations nflog_seq_ops = {
-- 
1.6.1

[PATCH 6/6] netfilter: sysctl support of logger choice.

[Eric Leblond]

This patchs adds support of modification of the used logger via sysctl.
It can be used to change the logger to module that can not use the bind
operation (ipt_LOG and ipt_ULOG). For this purpose, it creates a
directory /proc/sys/net/netfilter/nf_log which contains a file
per-protocol. The content of the file is the name current logger (NONE if
not set) and a logger can be setup by simply echoing its name to the file.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/linux/sysctl.h |    1 +
 net/netfilter/nf_log.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 39d471d..aa11b1c 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -335,6 +335,7 @@ enum
 	NET_NF_CONNTRACK_FRAG6_LOW_THRESH=30,
 	NET_NF_CONNTRACK_FRAG6_HIGH_THRESH=31,
 	NET_NF_CONNTRACK_CHECKSUM=32,
+	NET_NF_LOG=33,
 };
 
 /* /proc/sys/net/ipv4 */
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index ee08b62..93cf003 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,7 +14,7 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
-#define NFLOGGER_NAME_LEN		12
+#define NFLOGGER_NAME_LEN		24
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
 static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
@@ -216,16 +216,88 @@ static const struct file_operations nflog_file_ops = {
 
 #endif /* PROC_FS */
 
+#ifdef CONFIG_SYSCTL
+struct ctl_path nf_log_sysctl_path[] = {
+	{ .procname = "net", .ctl_name = CTL_NET, },
+	{ .procname = "netfilter", .ctl_name = NET_NETFILTER, },
+	{ .procname = "nf_log", .ctl_name = NET_NF_LOG, },
+	{ }
+};
+
+static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
+static struct ctl_table_header *nf_log_dir_header;
+
+static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
+			 void *buffer, size_t *lenp, loff_t *ppos)
+{
+	const struct nf_logger *logger;
+	int r;
+	char ln_name[NFLOGGER_NAME_LEN];
+
+	rcu_read_lock();
+	if (write) {
+		logger = __find_logger(table->ctl_name, buffer);
+		if (logger == NULL) {
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+		r = nf_log_bind_pf(table->ctl_name, logger);
+	} else {
+		logger = rcu_dereference(nf_loggers[table->ctl_name]);
+		if (! logger)
+			table->data = "NONE";
+		else {
+			memcpy(ln_name, logger->name, NFLOGGER_NAME_LEN);
+			table->data = ln_name;
+		}
+		r = proc_dostring(table, write, filp, buffer, lenp, ppos);
+	}
+	rcu_read_unlock();
+
+	return r;
+}
+
+static int netfilter_log_sysctl_init(void)
+{
+	int i;
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		char *pr_name = kmalloc(3, GFP_KERNEL);
+		/* FIXME free at deinit but there is currently no deinit */
+		snprintf(pr_name, 3, "%d", i);
+		nf_log_sysctl_table[i].ctl_name	= i;
+		nf_log_sysctl_table[i].procname	= pr_name;
+		nf_log_sysctl_table[i].data = NULL;
+		nf_log_sysctl_table[i].maxlen = NFLOGGER_NAME_LEN * sizeof(char);
+		nf_log_sysctl_table[i].mode = 0644;
+		nf_log_sysctl_table[i].proc_handler = nf_log_proc_dostring;
+	}
+
+	nf_log_dir_header = register_sysctl_paths(nf_log_sysctl_path,
+				       nf_log_sysctl_table);
+
+	return 0;
+}
+#else
+static int netfilter_log_sysctl_init(void)
+{
+	return 0;
+}
+#endif /* CONFIG_SYSCTL */
 
 int __init netfilter_log_init(void)
 {
 	int i;
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
 
+	if (netfilter_log_sysctl_init() < 0)
+		return -1;
+
 	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
 		INIT_LIST_HEAD(&(nf_loggers_l[i]));
 	}
-- 
1.6.1

Re: [PATCH 4/6] netfilter: convert logger modules to new API.

[Jan Engelhardt]

On Wednesday 2009-01-07 21:05, Eric Leblond wrote:

>This patch converts the logging modules to the new API. For nfnetlink_log,
>it simply switchs call to register functions to call to bind function and
>adds a call to nf_log_register() during init. For other modules, it just
>remove a const flag from the logger structure.
> 
>-static const struct nf_logger ipt_log_logger ={
>+static struct nf_logger ipt_log_logger ={

But maybe add __read_mostly, then?

Re: [PATCH 5/6] netfilter: print the list of register loggers.

[Jan Engelhardt]

On Wednesday 2009-01-07 21:05, Eric Leblond wrote:
>--- a/net/netfilter/nf_log.c
>+++ b/net/netfilter/nf_log.c
>@@ -172,13 +172,26 @@ static int seq_show(struct seq_file *s, void *v)
> {
> 	loff_t *pos = v;
> 	const struct nf_logger *logger;
>+	struct nf_logger *t;
>+	char loggers_string[256];
>+	int len = 256;

An array this big is likely to upset the guys who try to reduce
stack usage.
<mumble> there ought to be some way to figure out how much
space there is left in a seq... </mumble> and if there is
enough, just always use seq_printf.

[PATCH 0/6] rework of nf_log refactoring

[Eric Leblond]

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

Hi,

Le jeudi 08 janvier 2009 à 00:56 +0100, Jan Engelhardt a écrit :
> On Wednesday 2009-01-07 21:05, Eric Leblond wrote:
> >--- a/net/netfilter/nf_log.c
> >+++ b/net/netfilter/nf_log.c
> >@@ -172,13 +172,26 @@ static int seq_show(struct seq_file *s, void *v)
> > {
> > 	loff_t *pos = v;
> > 	const struct nf_logger *logger;
> >+	struct nf_logger *t;
> >+	char loggers_string[256];
> >+	int len = 256;
> 

Thanks Jan for your help. I've updated the patchset to follow
your remarks. It uses seq_printf instead of a strncat for the
proc modification and I've added the __read_mostly attribute.

Last modification of the patchset is a fix a of a bug in sysctl
handling.

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

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 1/6] netfilter: use a linked list of loggers.

[Eric Leblond]

This patch modifies nf_log to use a linked list of loggers for each
protocol. It also separate registration and binding. To be used as
logging module, a module has to register calling nf_log_register()
and to bind to a protocol it has to call nf_log_bind_pf().

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |   10 +++-
 net/netfilter/nf_log.c         |  107 +++++++++++++++++++++++++++++++---------
 2 files changed, 91 insertions(+), 26 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 7182c06..08b4f18 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -1,6 +1,8 @@
 #ifndef _NF_LOG_H
 #define _NF_LOG_H
 
+#include <linux/netfilter.h>
+
 /* those NF_LOG_* defines and struct nf_loginfo are legacy definitios that will
  * disappear once iptables is replaced with pkttables.  Please DO NOT use them
  * for any new code! */
@@ -40,13 +42,17 @@ struct nf_logger {
 	struct module	*me;
 	nf_logfn 	*logfn;
 	char		*name;
+	struct list_head	list[NFPROTO_NUMPROTO];
 };
 
 /* Function to register/unregister log function. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger);
-void nf_log_unregister(const struct nf_logger *logger);
+int nf_log_register(u_int8_t pf, struct nf_logger *logger);
+void nf_log_unregister(struct nf_logger *logger);
 void nf_log_unregister_pf(u_int8_t pf);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
+int nf_log_unbind_pf(u_int8_t pf);
+
 /* Calls the registered backend logging function */
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index fa8ae5d..8a5e4aa 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,13 +14,25 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
+#define NFLOGGER_NAME_LEN		12
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
-static DEFINE_MUTEX(nf_log_mutex);
+static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
+static DEFINE_SPINLOCK(nf_log_lock);
 
-/* return EBUSY if somebody else is registered, EEXIST if the same logger
- * is registred, 0 on success. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
+static struct nf_logger *__find_logger(int pf, const char *str_logger)
+{
+	struct nf_logger *t;
+
+	list_for_each_entry(t, &nf_loggers_l[pf], list[pf])
+		if (!strnicmp(str_logger, t->name, NFLOGGER_NAME_LEN))
+			return t;
+
+	return NULL;
+}
+
+/* return EEXIST if the same logger is registred, 0 on success. */
+int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	int ret;
 
@@ -29,50 +41,91 @@ int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
 
 	/* Any setup of logging members must be done before
 	 * substituting pointer. */
-	ret = mutex_lock_interruptible(&nf_log_mutex);
-	if (ret < 0)
-		return ret;
-
-	if (!nf_loggers[pf])
-		rcu_assign_pointer(nf_loggers[pf], logger);
-	else if (nf_loggers[pf] == logger)
-		ret = -EEXIST;
-	else
-		ret = -EBUSY;
-
-	mutex_unlock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
+
+	ret = -EEXIST;
+	if (pf == NFPROTO_UNSPEC) {
+		int i;
+		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+			if (__find_logger(i, logger->name) == NULL) {
+				ret = 0;
+				INIT_LIST_HEAD(&(logger->list[i]));
+				list_add_tail(&(logger->list[i]),
+						  &(nf_loggers_l[i]));
+			}
+		}
+	} else {
+		if (__find_logger(pf, logger->name) == NULL) {
+			ret = 0;
+			INIT_LIST_HEAD(&(logger->list[pf]));
+			/* register at end of list to honor first register win */
+			list_add_tail(&(logger->list[pf]),
+					  &nf_loggers_l[pf]);
+		}
+	}
+
+	spin_unlock_bh(&nf_log_lock);
 	return ret;
 }
 EXPORT_SYMBOL(nf_log_register);
 
 void nf_log_unregister_pf(u_int8_t pf)
 {
+	struct nf_logger *e;
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return;
-	mutex_lock(&nf_log_mutex);
-	rcu_assign_pointer(nf_loggers[pf], NULL);
-	mutex_unlock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
+	list_for_each_entry(e, &(nf_loggers_l[pf]), list[pf])
+		list_del(&(e->list[pf]));
+	spin_unlock_bh(&nf_log_lock);
 
 	/* Give time to concurrent readers. */
 	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unregister_pf);
 
-void nf_log_unregister(const struct nf_logger *logger)
+void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
+	struct nf_logger *llog;
 
-	mutex_lock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		if (nf_loggers[i] == logger)
-			rcu_assign_pointer(nf_loggers[i], NULL);
+		llog = __find_logger(i, logger->name);
+		if (llog) {
+			const struct nf_logger *c_logger = rcu_dereference(nf_loggers[i]);
+			if (c_logger == llog)
+				rcu_assign_pointer(nf_loggers[i], NULL);
+			list_del(&(logger->list[i]));
+		}
 	}
-	mutex_unlock(&nf_log_mutex);
+	spin_unlock_bh(&nf_log_lock);
 
 	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unregister);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger)
+{
+	if (__find_logger(pf, logger->name) == NULL)
+		return -1;
+
+	spin_lock_bh(&nf_log_lock);
+	rcu_assign_pointer(nf_loggers[pf], logger);
+	spin_unlock_bh(&nf_log_lock);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_bind_pf);
+
+int nf_log_unbind_pf(u_int8_t pf)
+{
+	spin_lock_bh(&nf_log_lock);
+	rcu_assign_pointer(nf_loggers[pf], NULL);
+	spin_unlock_bh(&nf_log_lock);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_unbind_pf);
+
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
 		   const struct sk_buff *skb,
@@ -163,10 +216,16 @@ static const struct file_operations nflog_file_ops = {
 
 int __init netfilter_log_init(void)
 {
+	int i;
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		INIT_LIST_HEAD(&(nf_loggers_l[i]));
+	}
+
 	return 0;
 }
-- 
1.5.6.3

[PATCH 2/6] netfilter: suppress nf_log_unregister_pf function.

[Eric Leblond]

The nf_log_unregister_pf() function is not used anymore as it was
corresponding to the unbind operation which is now done via
nflog_unbind_pf().

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |    1 -
 net/netfilter/nf_log.c         |   15 ---------------
 2 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 08b4f18..ccaa43a 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -48,7 +48,6 @@ struct nf_logger {
 /* Function to register/unregister log function. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger);
 void nf_log_unregister(struct nf_logger *logger);
-void nf_log_unregister_pf(u_int8_t pf);
 
 int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
 int nf_log_unbind_pf(u_int8_t pf);
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 8a5e4aa..a1dcd46 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -69,21 +69,6 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 }
 EXPORT_SYMBOL(nf_log_register);
 
-void nf_log_unregister_pf(u_int8_t pf)
-{
-	struct nf_logger *e;
-	if (pf >= ARRAY_SIZE(nf_loggers))
-		return;
-	spin_lock_bh(&nf_log_lock);
-	list_for_each_entry(e, &(nf_loggers_l[pf]), list[pf])
-		list_del(&(e->list[pf]));
-	spin_unlock_bh(&nf_log_lock);
-
-	/* Give time to concurrent readers. */
-	synchronize_rcu();
-}
-EXPORT_SYMBOL(nf_log_unregister_pf);
-
 void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
-- 
1.5.6.3

[PATCH 3/6] netfilter: bind at registration if no logger is already set.

[Eric Leblond]

This patch adds a bind operation when registration is done and when
there is currently no logger defined for the protocol. This emulates the
first registered wins policy and thus achieves backward compatibility.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index a1dcd46..1655a1f 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -35,6 +35,7 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	int ret;
+	const struct nf_logger *llog;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return -EINVAL;
@@ -58,13 +59,17 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 		if (__find_logger(pf, logger->name) == NULL) {
 			ret = 0;
 			INIT_LIST_HEAD(&(logger->list[pf]));
-			/* register at end of list to honor first register win */
 			list_add_tail(&(logger->list[pf]),
 					  &nf_loggers_l[pf]);
+			llog = rcu_dereference(nf_loggers[pf]);
+			if (llog == NULL)
+				rcu_assign_pointer(nf_loggers[pf], logger);
 		}
 	}
 
 	spin_unlock_bh(&nf_log_lock);
+
+	synchronize_rcu();
 	return ret;
 }
 EXPORT_SYMBOL(nf_log_register);
-- 
1.5.6.3

[PATCH 4/6] netfilter: convert logger modules to new API.

[Eric Leblond]

This patch converts the logging modules to the new API. For nfnetlink_log,
it simply switchs call to register functions to call to bind function and
adds a call to nf_log_register() during init. For other modules, it just
remove a const flag from the logger structure and replace it with a
__read_mostly.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 net/ipv4/netfilter/ipt_LOG.c  |    2 +-
 net/ipv4/netfilter/ipt_ULOG.c |    2 +-
 net/ipv6/netfilter/ip6t_LOG.c |    2 +-
 net/netfilter/nfnetlink_log.c |   11 ++++++++---
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c
index 4614a69..43ea638 100644
--- a/net/ipv4/netfilter/ipt_LOG.c
+++ b/net/ipv4/netfilter/ipt_LOG.c
@@ -464,7 +464,7 @@ static struct xt_target log_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static const struct nf_logger ipt_log_logger ={
+static struct nf_logger ipt_log_logger __read_mostly = {
 	.name		= "ipt_LOG",
 	.logfn		= &ipt_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index 18a2826..d32cc4b 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -379,7 +379,7 @@ static struct xt_target ulog_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static struct nf_logger ipt_ulog_logger = {
+static struct nf_logger ipt_ulog_logger __read_mostly = {
 	.name		= "ipt_ULOG",
 	.logfn		= ipt_logfn,
 	.me		= THIS_MODULE,
diff --git a/net/ipv6/netfilter/ip6t_LOG.c b/net/ipv6/netfilter/ip6t_LOG.c
index 7c668c6..66ae36b 100644
--- a/net/ipv6/netfilter/ip6t_LOG.c
+++ b/net/ipv6/netfilter/ip6t_LOG.c
@@ -477,7 +477,7 @@ static struct xt_target log_tg6_reg __read_mostly = {
 	.me 		= THIS_MODULE,
 };
 
-static const struct nf_logger ip6t_logger = {
+static struct nf_logger ip6t_logger __read_mostly = {
 	.name		= "ip6t_LOG",
 	.logfn		= &ip6t_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 2770b4e..3bed7d2 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -690,7 +690,7 @@ nfulnl_recv_unsupp(struct sock *ctnl, struct sk_buff *skb,
 	return -ENOTSUPP;
 }
 
-static const struct nf_logger nfulnl_logger = {
+static struct nf_logger nfulnl_logger __read_mostly = {
 	.name	= "nfnetlink_log",
 	.logfn	= &nfulnl_log_packet,
 	.me	= THIS_MODULE,
@@ -722,9 +722,9 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		/* Commands without queue context */
 		switch (cmd->command) {
 		case NFULNL_CFG_CMD_PF_BIND:
-			return nf_log_register(pf, &nfulnl_logger);
+			return nf_log_bind_pf(pf, &nfulnl_logger);
 		case NFULNL_CFG_CMD_PF_UNBIND:
-			nf_log_unregister_pf(pf);
+			nf_log_unbind_pf(pf);
 			return 0;
 		}
 	}
@@ -949,6 +949,11 @@ static int __init nfnetlink_log_init(void)
 		goto cleanup_netlink_notifier;
 	}
 
+	if ((status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger)) < 0) {
+		printk(KERN_ERR "log: failed to register logger\n");
+		goto cleanup_netlink_notifier;
+	}
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nfnetlink_log", 0440,
 			 proc_net_netfilter, &nful_file_ops))
-- 
1.5.6.3

[PATCH 5/6] netfilter: print the list of register loggers.

[Eric Leblond]

This patch modifies the proc output to add display of registered
loggers. The content of /proc/net/netfilter/nf_log is modified. Instead
of displaying a protocol per line with format:
	proto:logger
it now displays:
	proto:logger (comma_separated_list_of_loggers)

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 1655a1f..fc1deca 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -172,13 +172,31 @@ static int seq_show(struct seq_file *s, void *v)
 {
 	loff_t *pos = v;
 	const struct nf_logger *logger;
+	struct nf_logger *t;
+	int ret;
 
 	logger = rcu_dereference(nf_loggers[*pos]);
 
 	if (!logger)
-		return seq_printf(s, "%2lld NONE\n", *pos);
+		ret = seq_printf(s, "%2lld NONE (", *pos);
+	else
+		ret = seq_printf(s, "%2lld %s (", *pos, logger->name);
+
+	if (ret < 0)
+		return ret;
+
+	list_for_each_entry(t, &nf_loggers_l[*pos], list[*pos]) {
+		ret = seq_printf(s, "%s", t->name);
+		if (ret < 0)
+			return ret;
+		if (&t->list[*pos] != nf_loggers_l[*pos].prev) {
+			ret = seq_printf(s, ",");
+			if (ret < 0)
+				return ret;
+		}
+	}
 
-	return seq_printf(s, "%2lld %s\n", *pos, logger->name);
+	return seq_printf(s, ")\n");
 }
 
 static const struct seq_operations nflog_seq_ops = {
-- 
1.5.6.3

[PATCH 6/6] netfilter: sysctl support of logger choice.

[Eric Leblond]

This patchs adds support of modification of the used logger via sysctl.
It can be used to change the logger to module that can not use the bind
operation (ipt_LOG and ipt_ULOG). For this purpose, it creates a
directory /proc/sys/net/netfilter/nf_log which contains a file
per-protocol. The content of the file is the name current logger (NONE if
not set) and a logger can be setup by simply echoing its name to the file.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/linux/sysctl.h |    1 +
 net/netfilter/nf_log.c |   91 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 88 insertions(+), 4 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 39d471d..aa11b1c 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -335,6 +335,7 @@ enum
 	NET_NF_CONNTRACK_FRAG6_LOW_THRESH=30,
 	NET_NF_CONNTRACK_FRAG6_HIGH_THRESH=31,
 	NET_NF_CONNTRACK_CHECKSUM=32,
+	NET_NF_LOG=33,
 };
 
 /* /proc/sys/net/ipv4 */
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index fc1deca..d4b5259 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,23 +14,34 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
-#define NFLOGGER_NAME_LEN		12
+#define NFLOGGER_NAME_LEN		24
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
 static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
 static DEFINE_SPINLOCK(nf_log_lock);
 
-static struct nf_logger *__find_logger(int pf, const char *str_logger)
+static struct nf_logger *__find_logger_n(int pf, const char *str_logger, size_t len)
 {
 	struct nf_logger *t;
 
-	list_for_each_entry(t, &nf_loggers_l[pf], list[pf])
-		if (!strnicmp(str_logger, t->name, NFLOGGER_NAME_LEN))
+	if (len > NFLOGGER_NAME_LEN)
+		return NULL;
+
+	list_for_each_entry(t, &nf_loggers_l[pf], list[pf]) {
+		if ((len != NFLOGGER_NAME_LEN) && (strlen(t->name) != len))
+			continue;
+		if (!strnicmp(str_logger, t->name, len))
 			return t;
+	}
 
 	return NULL;
 }
 
+static struct nf_logger *__find_logger(int pf, const char *str_logger)
+{
+	return __find_logger_n(pf, str_logger, NFLOGGER_NAME_LEN);
+}
+
 /* return EEXIST if the same logger is registred, 0 on success. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
@@ -221,16 +232,88 @@ static const struct file_operations nflog_file_ops = {
 
 #endif /* PROC_FS */
 
+#ifdef CONFIG_SYSCTL
+struct ctl_path nf_log_sysctl_path[] = {
+	{ .procname = "net", .ctl_name = CTL_NET, },
+	{ .procname = "netfilter", .ctl_name = NET_NETFILTER, },
+	{ .procname = "nf_log", .ctl_name = NET_NF_LOG, },
+	{ }
+};
+
+static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
+static struct ctl_table_header *nf_log_dir_header;
+
+static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
+			 void *buffer, size_t *lenp, loff_t *ppos)
+{
+	const struct nf_logger *logger;
+	int r;
+	char ln_name[NFLOGGER_NAME_LEN];
+
+	rcu_read_lock();
+	if (write) {
+		logger = __find_logger_n(table->ctl_name, buffer, *lenp -1);
+		if (logger == NULL) {
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+		r = nf_log_bind_pf(table->ctl_name, logger);
+	} else {
+		logger = rcu_dereference(nf_loggers[table->ctl_name]);
+		if (! logger)
+			table->data = "NONE";
+		else {
+			memcpy(ln_name, logger->name, NFLOGGER_NAME_LEN);
+			table->data = ln_name;
+		}
+		r = proc_dostring(table, write, filp, buffer, lenp, ppos);
+	}
+	rcu_read_unlock();
+
+	return r;
+}
+
+static int netfilter_log_sysctl_init(void)
+{
+	int i;
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		char *pr_name = kmalloc(3, GFP_KERNEL);
+		/* FIXME free at deinit but there is currently no deinit */
+		snprintf(pr_name, 3, "%d", i);
+		nf_log_sysctl_table[i].ctl_name	= i;
+		nf_log_sysctl_table[i].procname	= pr_name;
+		nf_log_sysctl_table[i].data = NULL;
+		nf_log_sysctl_table[i].maxlen = NFLOGGER_NAME_LEN * sizeof(char);
+		nf_log_sysctl_table[i].mode = 0644;
+		nf_log_sysctl_table[i].proc_handler = nf_log_proc_dostring;
+	}
+
+	nf_log_dir_header = register_sysctl_paths(nf_log_sysctl_path,
+				       nf_log_sysctl_table);
+
+	return 0;
+}
+#else
+static int netfilter_log_sysctl_init(void)
+{
+	return 0;
+}
+#endif /* CONFIG_SYSCTL */
 
 int __init netfilter_log_init(void)
 {
 	int i;
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
 
+	if (netfilter_log_sysctl_init() < 0)
+		return -1;
+
 	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
 		INIT_LIST_HEAD(&(nf_loggers_l[i]));
 	}
-- 
1.5.6.3

[PATCH] netfilter: desactivate nf_log logger via sysctl.

[Eric Leblond]

This patch adds support for unbind of logger via sysctl. By echoing
"NONE" to a /proc/sys/net/netfilter/nf_log/PROTO file, the logger
corresponding to this PROTO is set to NULL.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index d4b5259..4775700 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -252,6 +252,10 @@ static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
 
 	rcu_read_lock();
 	if (write) {
+		if (!strnicmp(buffer, "NONE", *lenp - 1)) {
+			rcu_read_unlock();
+			return nf_log_unbind_pf(table->ctl_name);
+		}
 		logger = __find_logger_n(table->ctl_name, buffer, *lenp -1);
 		if (logger == NULL) {
 			rcu_read_unlock();
-- 
1.5.6.3

Re: [PATCH 0/6] rework of nf_log refactoring

[Patrick McHardy]

Eric Leblond wrote:
> Hi,
> 
> Le jeudi 08 janvier 2009 à 00:56 +0100, Jan Engelhardt a écrit :
>> On Wednesday 2009-01-07 21:05, Eric Leblond wrote:
>>> --- a/net/netfilter/nf_log.c
>>> +++ b/net/netfilter/nf_log.c
>>> @@ -172,13 +172,26 @@ static int seq_show(struct seq_file *s, void *v)
>>> {
>>> 	loff_t *pos = v;
>>> 	const struct nf_logger *logger;
>>> +	struct nf_logger *t;
>>> +	char loggers_string[256];
>>> +	int len = 256;
> 
> Thanks Jan for your help. I've updated the patchset to follow
> your remarks. It uses seq_printf instead of a strncat for the
> proc modification and I've added the __read_mostly attribute.
> 
> Last modification of the patchset is a fix a of a bug in sysctl
> handling.

I'm not sure whether I'm looking at the latest version. Could you
resend all logging patches I haven't applied yet please?

My tree for 2.6.30 (not merged up with the patches for 2.6.29 yet,
but I'll take care of any conflicts myself) is available at:

git://git.kernel.org/pub/scm/linux/kernel/git/kaber/nf-next-2.6.git
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] rework of nf_log refactoring

[Eric Leblond]

Hi,

Patrick McHardy a écrit :
> Eric Leblond wrote:
>> Hi,
>>
>> Le jeudi 08 janvier 2009 à 00:56 +0100, Jan Engelhardt a écrit :
>>> On Wednesday 2009-01-07 21:05, Eric Leblond wrote:
>>>> --- a/net/netfilter/nf_log.c
>>>> +++ b/net/netfilter/nf_log.c
> 
> I'm not sure whether I'm looking at the latest version. Could you
> resend all logging patches I haven't applied yet please?
> 
> My tree for 2.6.30 (not merged up with the patches for 2.6.29 yet,
> but I'll take care of any conflicts myself) is available at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/kaber/nf-next-2.6.git

Requested patches are following this mail. I've also added the reworked
"netfilter: fix nflog timeout handling" patch.

BR,
-- 
Eric Leblond <eric@inl.fr>
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] netfilter: use a linked list of loggers.

[Eric Leblond]

This patch modifies nf_log to use a linked list of loggers for each
protocol. It also separate registration and binding. To be used as
logging module, a module has to register calling nf_log_register()
and to bind to a protocol it has to call nf_log_bind_pf().

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |   10 +++-
 net/netfilter/nf_log.c         |  107 +++++++++++++++++++++++++++++++---------
 2 files changed, 91 insertions(+), 26 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 7182c06..08b4f18 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -1,6 +1,8 @@
 #ifndef _NF_LOG_H
 #define _NF_LOG_H
 
+#include <linux/netfilter.h>
+
 /* those NF_LOG_* defines and struct nf_loginfo are legacy definitios that will
  * disappear once iptables is replaced with pkttables.  Please DO NOT use them
  * for any new code! */
@@ -40,13 +42,17 @@ struct nf_logger {
 	struct module	*me;
 	nf_logfn 	*logfn;
 	char		*name;
+	struct list_head	list[NFPROTO_NUMPROTO];
 };
 
 /* Function to register/unregister log function. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger);
-void nf_log_unregister(const struct nf_logger *logger);
+int nf_log_register(u_int8_t pf, struct nf_logger *logger);
+void nf_log_unregister(struct nf_logger *logger);
 void nf_log_unregister_pf(u_int8_t pf);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
+int nf_log_unbind_pf(u_int8_t pf);
+
 /* Calls the registered backend logging function */
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index fa8ae5d..8a5e4aa 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,13 +14,25 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
+#define NFLOGGER_NAME_LEN		12
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
-static DEFINE_MUTEX(nf_log_mutex);
+static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
+static DEFINE_SPINLOCK(nf_log_lock);
 
-/* return EBUSY if somebody else is registered, EEXIST if the same logger
- * is registred, 0 on success. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
+static struct nf_logger *__find_logger(int pf, const char *str_logger)
+{
+	struct nf_logger *t;
+
+	list_for_each_entry(t, &nf_loggers_l[pf], list[pf])
+		if (!strnicmp(str_logger, t->name, NFLOGGER_NAME_LEN))
+			return t;
+
+	return NULL;
+}
+
+/* return EEXIST if the same logger is registred, 0 on success. */
+int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	int ret;
 
@@ -29,50 +41,91 @@ int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
 
 	/* Any setup of logging members must be done before
 	 * substituting pointer. */
-	ret = mutex_lock_interruptible(&nf_log_mutex);
-	if (ret < 0)
-		return ret;
-
-	if (!nf_loggers[pf])
-		rcu_assign_pointer(nf_loggers[pf], logger);
-	else if (nf_loggers[pf] == logger)
-		ret = -EEXIST;
-	else
-		ret = -EBUSY;
-
-	mutex_unlock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
+
+	ret = -EEXIST;
+	if (pf == NFPROTO_UNSPEC) {
+		int i;
+		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+			if (__find_logger(i, logger->name) == NULL) {
+				ret = 0;
+				INIT_LIST_HEAD(&(logger->list[i]));
+				list_add_tail(&(logger->list[i]),
+						  &(nf_loggers_l[i]));
+			}
+		}
+	} else {
+		if (__find_logger(pf, logger->name) == NULL) {
+			ret = 0;
+			INIT_LIST_HEAD(&(logger->list[pf]));
+			/* register at end of list to honor first register win */
+			list_add_tail(&(logger->list[pf]),
+					  &nf_loggers_l[pf]);
+		}
+	}
+
+	spin_unlock_bh(&nf_log_lock);
 	return ret;
 }
 EXPORT_SYMBOL(nf_log_register);
 
 void nf_log_unregister_pf(u_int8_t pf)
 {
+	struct nf_logger *e;
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return;
-	mutex_lock(&nf_log_mutex);
-	rcu_assign_pointer(nf_loggers[pf], NULL);
-	mutex_unlock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
+	list_for_each_entry(e, &(nf_loggers_l[pf]), list[pf])
+		list_del(&(e->list[pf]));
+	spin_unlock_bh(&nf_log_lock);
 
 	/* Give time to concurrent readers. */
 	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unregister_pf);
 
-void nf_log_unregister(const struct nf_logger *logger)
+void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
+	struct nf_logger *llog;
 
-	mutex_lock(&nf_log_mutex);
+	spin_lock_bh(&nf_log_lock);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		if (nf_loggers[i] == logger)
-			rcu_assign_pointer(nf_loggers[i], NULL);
+		llog = __find_logger(i, logger->name);
+		if (llog) {
+			const struct nf_logger *c_logger = rcu_dereference(nf_loggers[i]);
+			if (c_logger == llog)
+				rcu_assign_pointer(nf_loggers[i], NULL);
+			list_del(&(logger->list[i]));
+		}
 	}
-	mutex_unlock(&nf_log_mutex);
+	spin_unlock_bh(&nf_log_lock);
 
 	synchronize_rcu();
 }
 EXPORT_SYMBOL(nf_log_unregister);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger)
+{
+	if (__find_logger(pf, logger->name) == NULL)
+		return -1;
+
+	spin_lock_bh(&nf_log_lock);
+	rcu_assign_pointer(nf_loggers[pf], logger);
+	spin_unlock_bh(&nf_log_lock);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_bind_pf);
+
+int nf_log_unbind_pf(u_int8_t pf)
+{
+	spin_lock_bh(&nf_log_lock);
+	rcu_assign_pointer(nf_loggers[pf], NULL);
+	spin_unlock_bh(&nf_log_lock);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_unbind_pf);
+
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
 		   const struct sk_buff *skb,
@@ -163,10 +216,16 @@ static const struct file_operations nflog_file_ops = {
 
 int __init netfilter_log_init(void)
 {
+	int i;
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		INIT_LIST_HEAD(&(nf_loggers_l[i]));
+	}
+
 	return 0;
 }
-- 
1.5.6.3

[PATCH 2/7] netfilter: suppress nf_log_unregister_pf function.

[Eric Leblond]

The nf_log_unregister_pf() function is not used anymore as it was
corresponding to the unbind operation which is now done via
nflog_unbind_pf().

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |    1 -
 net/netfilter/nf_log.c         |   15 ---------------
 2 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 08b4f18..ccaa43a 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -48,7 +48,6 @@ struct nf_logger {
 /* Function to register/unregister log function. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger);
 void nf_log_unregister(struct nf_logger *logger);
-void nf_log_unregister_pf(u_int8_t pf);
 
 int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
 int nf_log_unbind_pf(u_int8_t pf);
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 8a5e4aa..a1dcd46 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -69,21 +69,6 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 }
 EXPORT_SYMBOL(nf_log_register);
 
-void nf_log_unregister_pf(u_int8_t pf)
-{
-	struct nf_logger *e;
-	if (pf >= ARRAY_SIZE(nf_loggers))
-		return;
-	spin_lock_bh(&nf_log_lock);
-	list_for_each_entry(e, &(nf_loggers_l[pf]), list[pf])
-		list_del(&(e->list[pf]));
-	spin_unlock_bh(&nf_log_lock);
-
-	/* Give time to concurrent readers. */
-	synchronize_rcu();
-}
-EXPORT_SYMBOL(nf_log_unregister_pf);
-
 void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
-- 
1.5.6.3

[PATCH 3/7] netfilter: bind at registration if no logger is already set.

[Eric Leblond]

This patch adds a bind operation when registration is done and when
there is currently no logger defined for the protocol. This emulates the
first registered wins policy and thus achieves backward compatibility.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index a1dcd46..1655a1f 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -35,6 +35,7 @@ static struct nf_logger *__find_logger(int pf, const char *str_logger)
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	int ret;
+	const struct nf_logger *llog;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return -EINVAL;
@@ -58,13 +59,17 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 		if (__find_logger(pf, logger->name) == NULL) {
 			ret = 0;
 			INIT_LIST_HEAD(&(logger->list[pf]));
-			/* register at end of list to honor first register win */
 			list_add_tail(&(logger->list[pf]),
 					  &nf_loggers_l[pf]);
+			llog = rcu_dereference(nf_loggers[pf]);
+			if (llog == NULL)
+				rcu_assign_pointer(nf_loggers[pf], logger);
 		}
 	}
 
 	spin_unlock_bh(&nf_log_lock);
+
+	synchronize_rcu();
 	return ret;
 }
 EXPORT_SYMBOL(nf_log_register);
-- 
1.5.6.3

[PATCH 4/7] netfilter: convert logger modules to new API.

[Eric Leblond]

This patch converts the logging modules to the new API. For nfnetlink_log,
it simply switchs call to register functions to call to bind function and
adds a call to nf_log_register() during init. For other modules, it just
remove a const flag from the logger structure and replace it with a
__read_mostly.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 net/ipv4/netfilter/ipt_LOG.c  |    2 +-
 net/ipv4/netfilter/ipt_ULOG.c |    2 +-
 net/ipv6/netfilter/ip6t_LOG.c |    2 +-
 net/netfilter/nfnetlink_log.c |   11 ++++++++---
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c
index 27a78fb..acc44c6 100644
--- a/net/ipv4/netfilter/ipt_LOG.c
+++ b/net/ipv4/netfilter/ipt_LOG.c
@@ -464,7 +464,7 @@ static struct xt_target log_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static const struct nf_logger ipt_log_logger ={
+static struct nf_logger ipt_log_logger __read_mostly = {
 	.name		= "ipt_LOG",
 	.logfn		= &ipt_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index 18a2826..d32cc4b 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -379,7 +379,7 @@ static struct xt_target ulog_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static struct nf_logger ipt_ulog_logger = {
+static struct nf_logger ipt_ulog_logger __read_mostly = {
 	.name		= "ipt_ULOG",
 	.logfn		= ipt_logfn,
 	.me		= THIS_MODULE,
diff --git a/net/ipv6/netfilter/ip6t_LOG.c b/net/ipv6/netfilter/ip6t_LOG.c
index 37adf5a..7018cac 100644
--- a/net/ipv6/netfilter/ip6t_LOG.c
+++ b/net/ipv6/netfilter/ip6t_LOG.c
@@ -477,7 +477,7 @@ static struct xt_target log_tg6_reg __read_mostly = {
 	.me 		= THIS_MODULE,
 };
 
-static const struct nf_logger ip6t_logger = {
+static struct nf_logger ip6t_logger __read_mostly = {
 	.name		= "ip6t_LOG",
 	.logfn		= &ip6t_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 580b837..88b455e 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -693,7 +693,7 @@ nfulnl_recv_unsupp(struct sock *ctnl, struct sk_buff *skb,
 	return -ENOTSUPP;
 }
 
-static const struct nf_logger nfulnl_logger = {
+static struct nf_logger nfulnl_logger __read_mostly = {
 	.name	= "nfnetlink_log",
 	.logfn	= &nfulnl_log_packet,
 	.me	= THIS_MODULE,
@@ -725,9 +725,9 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		/* Commands without queue context */
 		switch (cmd->command) {
 		case NFULNL_CFG_CMD_PF_BIND:
-			return nf_log_register(pf, &nfulnl_logger);
+			return nf_log_bind_pf(pf, &nfulnl_logger);
 		case NFULNL_CFG_CMD_PF_UNBIND:
-			nf_log_unregister_pf(pf);
+			nf_log_unbind_pf(pf);
 			return 0;
 		}
 	}
@@ -952,6 +952,11 @@ static int __init nfnetlink_log_init(void)
 		goto cleanup_netlink_notifier;
 	}
 
+	if ((status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger)) < 0) {
+		printk(KERN_ERR "log: failed to register logger\n");
+		goto cleanup_netlink_notifier;
+	}
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nfnetlink_log", 0440,
 			 proc_net_netfilter, &nful_file_ops))
-- 
1.5.6.3

[PATCH 5/7] netfilter: print the list of register loggers.

[Eric Leblond]

This patch modifies the proc output to add display of registered
loggers. The content of /proc/net/netfilter/nf_log is modified. Instead
of displaying a protocol per line with format:
	proto:logger
it now displays:
	proto:logger (comma_separated_list_of_loggers)

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 1655a1f..fc1deca 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -172,13 +172,31 @@ static int seq_show(struct seq_file *s, void *v)
 {
 	loff_t *pos = v;
 	const struct nf_logger *logger;
+	struct nf_logger *t;
+	int ret;
 
 	logger = rcu_dereference(nf_loggers[*pos]);
 
 	if (!logger)
-		return seq_printf(s, "%2lld NONE\n", *pos);
+		ret = seq_printf(s, "%2lld NONE (", *pos);
+	else
+		ret = seq_printf(s, "%2lld %s (", *pos, logger->name);
+
+	if (ret < 0)
+		return ret;
+
+	list_for_each_entry(t, &nf_loggers_l[*pos], list[*pos]) {
+		ret = seq_printf(s, "%s", t->name);
+		if (ret < 0)
+			return ret;
+		if (&t->list[*pos] != nf_loggers_l[*pos].prev) {
+			ret = seq_printf(s, ",");
+			if (ret < 0)
+				return ret;
+		}
+	}
 
-	return seq_printf(s, "%2lld %s\n", *pos, logger->name);
+	return seq_printf(s, ")\n");
 }
 
 static const struct seq_operations nflog_seq_ops = {
-- 
1.5.6.3

[PATCH 6/7] netfilter: sysctl support of logger choice.

[Eric Leblond]

This patchs adds support of modification of the used logger via sysctl.
It can be used to change the logger to module that can not use the bind
operation (ipt_LOG and ipt_ULOG). For this purpose, it creates a
directory /proc/sys/net/netfilter/nf_log which contains a file
per-protocol. The content of the file is the name current logger (NONE if
not set) and a logger can be setup by simply echoing its name to the file.
By echoing "NONE" to a /proc/sys/net/netfilter/nf_log/PROTO file, the
logger corresponding to this PROTO is set to NULL.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/linux/sysctl.h |    1 +
 net/netfilter/nf_log.c |   95 ++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 92 insertions(+), 4 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index e76d3b2..78d8642 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -335,6 +335,7 @@ enum
 	NET_NF_CONNTRACK_FRAG6_LOW_THRESH=30,
 	NET_NF_CONNTRACK_FRAG6_HIGH_THRESH=31,
 	NET_NF_CONNTRACK_CHECKSUM=32,
+	NET_NF_LOG=33,
 };
 
 /* /proc/sys/net/ipv4 */
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index fc1deca..4775700 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,23 +14,34 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
-#define NFLOGGER_NAME_LEN		12
+#define NFLOGGER_NAME_LEN		24
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
 static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
 static DEFINE_SPINLOCK(nf_log_lock);
 
-static struct nf_logger *__find_logger(int pf, const char *str_logger)
+static struct nf_logger *__find_logger_n(int pf, const char *str_logger, size_t len)
 {
 	struct nf_logger *t;
 
-	list_for_each_entry(t, &nf_loggers_l[pf], list[pf])
-		if (!strnicmp(str_logger, t->name, NFLOGGER_NAME_LEN))
+	if (len > NFLOGGER_NAME_LEN)
+		return NULL;
+
+	list_for_each_entry(t, &nf_loggers_l[pf], list[pf]) {
+		if ((len != NFLOGGER_NAME_LEN) && (strlen(t->name) != len))
+			continue;
+		if (!strnicmp(str_logger, t->name, len))
 			return t;
+	}
 
 	return NULL;
 }
 
+static struct nf_logger *__find_logger(int pf, const char *str_logger)
+{
+	return __find_logger_n(pf, str_logger, NFLOGGER_NAME_LEN);
+}
+
 /* return EEXIST if the same logger is registred, 0 on success. */
 int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
@@ -221,16 +232,92 @@ static const struct file_operations nflog_file_ops = {
 
 #endif /* PROC_FS */
 
+#ifdef CONFIG_SYSCTL
+struct ctl_path nf_log_sysctl_path[] = {
+	{ .procname = "net", .ctl_name = CTL_NET, },
+	{ .procname = "netfilter", .ctl_name = NET_NETFILTER, },
+	{ .procname = "nf_log", .ctl_name = NET_NF_LOG, },
+	{ }
+};
+
+static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
+static struct ctl_table_header *nf_log_dir_header;
+
+static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
+			 void *buffer, size_t *lenp, loff_t *ppos)
+{
+	const struct nf_logger *logger;
+	int r;
+	char ln_name[NFLOGGER_NAME_LEN];
+
+	rcu_read_lock();
+	if (write) {
+		if (!strnicmp(buffer, "NONE", *lenp - 1)) {
+			rcu_read_unlock();
+			return nf_log_unbind_pf(table->ctl_name);
+		}
+		logger = __find_logger_n(table->ctl_name, buffer, *lenp -1);
+		if (logger == NULL) {
+			rcu_read_unlock();
+			return -EINVAL;
+		}
+		r = nf_log_bind_pf(table->ctl_name, logger);
+	} else {
+		logger = rcu_dereference(nf_loggers[table->ctl_name]);
+		if (! logger)
+			table->data = "NONE";
+		else {
+			memcpy(ln_name, logger->name, NFLOGGER_NAME_LEN);
+			table->data = ln_name;
+		}
+		r = proc_dostring(table, write, filp, buffer, lenp, ppos);
+	}
+	rcu_read_unlock();
+
+	return r;
+}
+
+static int netfilter_log_sysctl_init(void)
+{
+	int i;
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		char *pr_name = kmalloc(3, GFP_KERNEL);
+		/* FIXME free at deinit but there is currently no deinit */
+		snprintf(pr_name, 3, "%d", i);
+		nf_log_sysctl_table[i].ctl_name	= i;
+		nf_log_sysctl_table[i].procname	= pr_name;
+		nf_log_sysctl_table[i].data = NULL;
+		nf_log_sysctl_table[i].maxlen = NFLOGGER_NAME_LEN * sizeof(char);
+		nf_log_sysctl_table[i].mode = 0644;
+		nf_log_sysctl_table[i].proc_handler = nf_log_proc_dostring;
+	}
+
+	nf_log_dir_header = register_sysctl_paths(nf_log_sysctl_path,
+				       nf_log_sysctl_table);
+
+	return 0;
+}
+#else
+static int netfilter_log_sysctl_init(void)
+{
+	return 0;
+}
+#endif /* CONFIG_SYSCTL */
 
 int __init netfilter_log_init(void)
 {
 	int i;
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
 
+	if (netfilter_log_sysctl_init() < 0)
+		return -1;
+
 	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
 		INIT_LIST_HEAD(&(nf_loggers_l[i]));
 	}
-- 
1.5.6.3

[PATCH 7/7] 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 modify the default delay by
using 100 instead of HZ.

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

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 88b455e..42e7de9 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 	100	/* 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 */
 
-- 
1.5.6.3

Re: [PATCH 1/7] netfilter: use a linked list of loggers.

[Patrick McHardy]

Eric Leblond wrote:
> This patch modifies nf_log to use a linked list of loggers for each
> protocol. It also separate registration and binding. To be used as
> logging module, a module has to register calling nf_log_register()
> and to bind to a protocol it has to call nf_log_bind_pf().
> 
> Signed-off-by: Eric Leblond <eric@inl.fr>
> ---
>  include/net/netfilter/nf_log.h |   10 +++-
>  net/netfilter/nf_log.c         |  107 +++++++++++++++++++++++++++++++---------
>  2 files changed, 91 insertions(+), 26 deletions(-)
> 
> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index fa8ae5d..8a5e4aa 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -14,13 +14,25 @@
>     LOG target modules */
>  
>  #define NF_LOG_PREFIXLEN		128
> +#define NFLOGGER_NAME_LEN		12

Just wondering what this limit is based on, I can't seem to
fine anything requiring a limit at all since the names are
const char *.

>  static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
> -static DEFINE_MUTEX(nf_log_mutex);
> +static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
> +static DEFINE_SPINLOCK(nf_log_lock);

I don't understand how locking is supposed to work now. You change
the mutex to a spinlock and use it with _bh everywhere, yet I don't
see a single spot thats called outside of process context and the
RCU assignments are still present.

The users of nf_log_register are also not converted simultaneously,
so this patch breaks bisection.

> @@ -29,50 +41,91 @@ int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
>  
>  	/* Any setup of logging members must be done before
>  	 * substituting pointer. */
> -	ret = mutex_lock_interruptible(&nf_log_mutex);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (!nf_loggers[pf])
> -		rcu_assign_pointer(nf_loggers[pf], logger);
> -	else if (nf_loggers[pf] == logger)
> -		ret = -EEXIST;
> -	else
> -		ret = -EBUSY;
> -
> -	mutex_unlock(&nf_log_mutex);
> +	spin_lock_bh(&nf_log_lock);
> +
> +	ret = -EEXIST;
> +	if (pf == NFPROTO_UNSPEC) {
> +		int i;
> +		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
> +			if (__find_logger(i, logger->name) == NULL) {
> +				ret = 0;
> +				INIT_LIST_HEAD(&(logger->list[i]));

This is not necessary, list_add_* doesn't require the new element
to be initialized. Also please remove the unneeded parens (same
remarks for below).

> +				list_add_tail(&(logger->list[i]),
> +						  &(nf_loggers_l[i]));
> +			}
> +		}
> +	} else {
> +		if (__find_logger(pf, logger->name) == NULL) {
> +			ret = 0;
> +			INIT_LIST_HEAD(&(logger->list[pf]));
> +			/* register at end of list to honor first register win */
> +			list_add_tail(&(logger->list[pf]),
> +					  &nf_loggers_l[pf]);
> +		}
> +	}
> +
> +	spin_unlock_bh(&nf_log_lock);
>  	return ret;
>  }
>  EXPORT_SYMBOL(nf_log_register);
>  
>  void nf_log_unregister_pf(u_int8_t pf)
>  {
> +	struct nf_logger *e;
>  	if (pf >= ARRAY_SIZE(nf_loggers))
>  		return;
> -	mutex_lock(&nf_log_mutex);
> -	rcu_assign_pointer(nf_loggers[pf], NULL);
> -	mutex_unlock(&nf_log_mutex);
> +	spin_lock_bh(&nf_log_lock);
> +	list_for_each_entry(e, &(nf_loggers_l[pf]), list[pf])
> +		list_del(&(e->list[pf]));
> +	spin_unlock_bh(&nf_log_lock);
>  
>  	/* Give time to concurrent readers. */
>  	synchronize_rcu();
>  }
>  EXPORT_SYMBOL(nf_log_unregister_pf);
>  
> -void nf_log_unregister(const struct nf_logger *logger)
> +void nf_log_unregister(struct nf_logger *logger)
>  {
>  	int i;
> +	struct nf_logger *llog;
>  
> -	mutex_lock(&nf_log_mutex);
> +	spin_lock_bh(&nf_log_lock);
>  	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
> -		if (nf_loggers[i] == logger)
> -			rcu_assign_pointer(nf_loggers[i], NULL);
> +		llog = __find_logger(i, logger->name);
> +		if (llog) {
> +			const struct nf_logger *c_logger = rcu_dereference(nf_loggers[i]);
> +			if (c_logger == llog)
> +				rcu_assign_pointer(nf_loggers[i], NULL);
> +			list_del(&(logger->list[i]));
> +		}
>  	}
> -	mutex_unlock(&nf_log_mutex);
> +	spin_unlock_bh(&nf_log_lock);
>  
>  	synchronize_rcu();
>  }
>  EXPORT_SYMBOL(nf_log_unregister);
>  
> +int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger)
> +{
> +	if (__find_logger(pf, logger->name) == NULL)
> +		return -1;
> +
> +	spin_lock_bh(&nf_log_lock);
> +	rcu_assign_pointer(nf_loggers[pf], logger);
> +	spin_unlock_bh(&nf_log_lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL(nf_log_bind_pf);
> +
> +int nf_log_unbind_pf(u_int8_t pf)
> +{
> +	spin_lock_bh(&nf_log_lock);
> +	rcu_assign_pointer(nf_loggers[pf], NULL);
> +	spin_unlock_bh(&nf_log_lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL(nf_log_unbind_pf);
> +
>  void nf_log_packet(u_int8_t pf,
>  		   unsigned int hooknum,
>  		   const struct sk_buff *skb,
> @@ -163,10 +216,16 @@ static const struct file_operations nflog_file_ops = {
>  
>  int __init netfilter_log_init(void)
>  {
> +	int i;
>  #ifdef CONFIG_PROC_FS
>  	if (!proc_create("nf_log", S_IRUGO,
>  			 proc_net_netfilter, &nflog_file_ops))
>  		return -1;
>  #endif
> +
> +	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
> +		INIT_LIST_HEAD(&(nf_loggers_l[i]));
> +	}

Please no parens for single statements.

Re: [PATCH 6/7] netfilter: sysctl support of logger choice.

[Patrick McHardy]

Eric Leblond wrote:
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index e76d3b2..78d8642 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -335,6 +335,7 @@ enum
>  	NET_NF_CONNTRACK_FRAG6_LOW_THRESH=30,
>  	NET_NF_CONNTRACK_FRAG6_HIGH_THRESH=31,
>  	NET_NF_CONNTRACK_CHECKSUM=32,
> +	NET_NF_LOG=33,

New sysctls should use CTL_UNNUMBERED.

>  };
>  
>  /* /proc/sys/net/ipv4 */
> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index fc1deca..4775700 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -14,23 +14,34 @@
>     LOG target modules */
>  
>  #define NF_LOG_PREFIXLEN		128
> -#define NFLOGGER_NAME_LEN		12
> +#define NFLOGGER_NAME_LEN		24

Please restructure this so the first patch adds the final value,
if necessary at all.

>  
>  static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
>  static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
>  static DEFINE_SPINLOCK(nf_log_lock);
>  
> -static struct nf_logger *__find_logger(int pf, const char *str_logger)
> +static struct nf_logger *__find_logger_n(int pf, const char *str_logger, size_t len)
>  {
>  	struct nf_logger *t;
>  
> -	list_for_each_entry(t, &nf_loggers_l[pf], list[pf])
> -		if (!strnicmp(str_logger, t->name, NFLOGGER_NAME_LEN))
> +	if (len > NFLOGGER_NAME_LEN)
> +		return NULL;
> +
> +	list_for_each_entry(t, &nf_loggers_l[pf], list[pf]) {
> +		if ((len != NFLOGGER_NAME_LEN) && (strlen(t->name) != len))

This seems like overoptimization, just the strcmp is fine. Then you can
also remove the wrapper around this function.

> +			continue;
> +		if (!strnicmp(str_logger, t->name, len))
>  			return t;
> +	}
>  
>  	return NULL;
>  }
>  
> +static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
> +static struct ctl_table_header *nf_log_dir_header;
> +
> +static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
> +			 void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	const struct nf_logger *logger;
> +	int r;
> +	char ln_name[NFLOGGER_NAME_LEN];
> +
> +	rcu_read_lock();
> +	if (write) {
> +		if (!strnicmp(buffer, "NONE", *lenp - 1)) {
> +			rcu_read_unlock();
> +			return nf_log_unbind_pf(table->ctl_name);
> +		}
> +		logger = __find_logger_n(table->ctl_name, buffer, *lenp -1);
> +		if (logger == NULL) {
> +			rcu_read_unlock();
> +			return -EINVAL;
> +		}
> +		r = nf_log_bind_pf(table->ctl_name, logger);

I doubt that RCU read lock is really what you want here since you're
actively changing things.

Re: [PATCH 7/7] 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 modify the default delay by
> using 100 instead of HZ.

Applied, thanks.

Re: [PATCH 1/7] netfilter: use a linked list of loggers.

[Eric Leblond]

Hi,

Patrick McHardy a écrit :
> Eric Leblond wrote:
>> This patch modifies nf_log to use a linked list of loggers for each
>
>> ---
>>  include/net/netfilter/nf_log.h |   10 +++-

>> +#define NFLOGGER_NAME_LEN        12
> 
> Just wondering what this limit is based on, I can't seem to
> fine anything requiring a limit at all since the names are
> const char *.

Ok, good remark. This is not needed here but I was forced to keep a name
maximum length due to the maxlen part of sysctl.

>>  static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO]
>> __read_mostly;
>> -static DEFINE_MUTEX(nf_log_mutex);
>> +static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
>> +static DEFINE_SPINLOCK(nf_log_lock);
> 
> I don't understand how locking is supposed to work now. You change
> the mutex to a spinlock and use it with _bh everywhere, yet I don't
> see a single spot thats called outside of process context and the
> RCU assignments are still present.

I've simplified the code to have the loggers list protected by a trivial
mutex and I only use RCU for the used function array.

> The users of nf_log_register are also not converted simultaneously,
> so this patch breaks bisection.

I've reordered and combined my patches to avoid this.

Thanks a lot for all your remarks, the updated patchset will follow as
reply to this mail.

BR,
-- 
Eric Leblond <eric@inl.fr>
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] netfilter: use a linked list of loggers.

[Eric Leblond]

This patch modifies nf_log to use a linked list of loggers for each
protocol. This list of loggers is read and write protected with a
mutex.
This patch separates registration and binding. To be used as
logging module, a module has to register calling nf_log_register()
and to bind to a protocol it has to call nf_log_bind_pf().
This patch also converts the logging modules to the new API. For nfnetlink_log,
it simply switchs call to register functions to call to bind function and
adds a call to nf_log_register() during init. For other modules, it just
remove a const flag from the logger structure and replace it with a
__read_mostly.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |   11 ++++-
 net/ipv4/netfilter/ipt_LOG.c   |    2 +-
 net/ipv4/netfilter/ipt_ULOG.c  |    2 +-
 net/ipv6/netfilter/ip6t_LOG.c  |    2 +-
 net/netfilter/nf_log.c         |   91 ++++++++++++++++++++++++++++++++-------
 net/netfilter/nfnetlink_log.c  |   11 ++++-
 6 files changed, 93 insertions(+), 26 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 7182c06..ccaa43a 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -1,6 +1,8 @@
 #ifndef _NF_LOG_H
 #define _NF_LOG_H
 
+#include <linux/netfilter.h>
+
 /* those NF_LOG_* defines and struct nf_loginfo are legacy definitios that will
  * disappear once iptables is replaced with pkttables.  Please DO NOT use them
  * for any new code! */
@@ -40,12 +42,15 @@ struct nf_logger {
 	struct module	*me;
 	nf_logfn 	*logfn;
 	char		*name;
+	struct list_head	list[NFPROTO_NUMPROTO];
 };
 
 /* Function to register/unregister log function. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger);
-void nf_log_unregister(const struct nf_logger *logger);
-void nf_log_unregister_pf(u_int8_t pf);
+int nf_log_register(u_int8_t pf, struct nf_logger *logger);
+void nf_log_unregister(struct nf_logger *logger);
+
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
+int nf_log_unbind_pf(u_int8_t pf);
 
 /* Calls the registered backend logging function */
 void nf_log_packet(u_int8_t pf,
diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c
index 27a78fb..acc44c6 100644
--- a/net/ipv4/netfilter/ipt_LOG.c
+++ b/net/ipv4/netfilter/ipt_LOG.c
@@ -464,7 +464,7 @@ static struct xt_target log_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static const struct nf_logger ipt_log_logger ={
+static struct nf_logger ipt_log_logger __read_mostly = {
 	.name		= "ipt_LOG",
 	.logfn		= &ipt_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index 18a2826..d32cc4b 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -379,7 +379,7 @@ static struct xt_target ulog_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static struct nf_logger ipt_ulog_logger = {
+static struct nf_logger ipt_ulog_logger __read_mostly = {
 	.name		= "ipt_ULOG",
 	.logfn		= ipt_logfn,
 	.me		= THIS_MODULE,
diff --git a/net/ipv6/netfilter/ip6t_LOG.c b/net/ipv6/netfilter/ip6t_LOG.c
index 37adf5a..7018cac 100644
--- a/net/ipv6/netfilter/ip6t_LOG.c
+++ b/net/ipv6/netfilter/ip6t_LOG.c
@@ -477,7 +477,7 @@ static struct xt_target log_tg6_reg __read_mostly = {
 	.me 		= THIS_MODULE,
 };
 
-static const struct nf_logger ip6t_logger = {
+static struct nf_logger ip6t_logger __read_mostly = {
 	.name		= "ip6t_LOG",
 	.logfn		= &ip6t_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index fa8ae5d..50d34db 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -16,31 +16,55 @@
 #define NF_LOG_PREFIXLEN		128
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
+static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
 static DEFINE_MUTEX(nf_log_mutex);
 
-/* return EBUSY if somebody else is registered, EEXIST if the same logger
- * is registred, 0 on success. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
+static struct nf_logger *__find_logger(int pf, const char *str_logger)
+{
+	struct nf_logger *t;
+
+	list_for_each_entry(t, &nf_loggers_l[pf], list[pf])
+		if (!strnicmp(str_logger, t->name, strlen(t->name))) {
+			return t;
+		}
+
+	return NULL;
+}
+
+/* return EEXIST if the same logger is registred, 0 on success. */
+int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
 	int ret;
+	const struct nf_logger *llog;
 
 	if (pf >= ARRAY_SIZE(nf_loggers))
 		return -EINVAL;
 
-	/* Any setup of logging members must be done before
-	 * substituting pointer. */
-	ret = mutex_lock_interruptible(&nf_log_mutex);
-	if (ret < 0)
-		return ret;
+	mutex_lock(&nf_log_mutex);
 
-	if (!nf_loggers[pf])
-		rcu_assign_pointer(nf_loggers[pf], logger);
-	else if (nf_loggers[pf] == logger)
-		ret = -EEXIST;
-	else
-		ret = -EBUSY;
+	ret = -EEXIST;
+	if (pf == NFPROTO_UNSPEC) {
+		int i;
+		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
+			if (__find_logger(i, logger->name) == NULL) {
+				ret = 0;
+				list_add_tail(&(logger->list[i]),
+						&(nf_loggers_l[i]));
+			}
+	} else {
+		if (__find_logger(pf, logger->name) == NULL) {
+			ret = 0;
+			/* register at end of list to honor first register win */
+			list_add_tail(&(logger->list[pf]),
+					  &nf_loggers_l[pf]);
+			llog = rcu_dereference(nf_loggers[pf]);
+			if (llog == NULL)
+				rcu_assign_pointer(nf_loggers[pf], logger);
+		}
+	}
 
 	mutex_unlock(&nf_log_mutex);
+
 	return ret;
 }
 EXPORT_SYMBOL(nf_log_register);
@@ -58,14 +82,20 @@ void nf_log_unregister_pf(u_int8_t pf)
 }
 EXPORT_SYMBOL(nf_log_unregister_pf);
 
-void nf_log_unregister(const struct nf_logger *logger)
+void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
+	struct nf_logger *llog;
 
 	mutex_lock(&nf_log_mutex);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		if (nf_loggers[i] == logger)
-			rcu_assign_pointer(nf_loggers[i], NULL);
+		llog = __find_logger(i, logger->name);
+		if (llog) {
+			const struct nf_logger *c_logger = rcu_dereference(nf_loggers[i]);
+			if (c_logger == llog)
+				rcu_assign_pointer(nf_loggers[i], NULL);
+			list_del(&(logger->list[i]));
+		}
 	}
 	mutex_unlock(&nf_log_mutex);
 
@@ -73,6 +103,28 @@ void nf_log_unregister(const struct nf_logger *logger)
 }
 EXPORT_SYMBOL(nf_log_unregister);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger)
+{
+	mutex_lock(&nf_log_mutex);
+	if (__find_logger(pf, logger->name) == NULL) {
+		mutex_unlock(&nf_log_mutex);
+		return -1;
+	}
+	rcu_assign_pointer(nf_loggers[pf], logger);
+	mutex_unlock(&nf_log_mutex);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_bind_pf);
+
+int nf_log_unbind_pf(u_int8_t pf)
+{
+	mutex_lock(&nf_log_mutex);
+	rcu_assign_pointer(nf_loggers[pf], NULL);
+	mutex_unlock(&nf_log_mutex);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_unbind_pf);
+
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
 		   const struct sk_buff *skb,
@@ -163,10 +215,15 @@ static const struct file_operations nflog_file_ops = {
 
 int __init netfilter_log_init(void)
 {
+	int i;
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
+		INIT_LIST_HEAD(&(nf_loggers_l[i]));
+
 	return 0;
 }
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index c712e9f..42e7de9 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -693,7 +693,7 @@ nfulnl_recv_unsupp(struct sock *ctnl, struct sk_buff *skb,
 	return -ENOTSUPP;
 }
 
-static const struct nf_logger nfulnl_logger = {
+static struct nf_logger nfulnl_logger __read_mostly = {
 	.name	= "nfnetlink_log",
 	.logfn	= &nfulnl_log_packet,
 	.me	= THIS_MODULE,
@@ -725,9 +725,9 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		/* Commands without queue context */
 		switch (cmd->command) {
 		case NFULNL_CFG_CMD_PF_BIND:
-			return nf_log_register(pf, &nfulnl_logger);
+			return nf_log_bind_pf(pf, &nfulnl_logger);
 		case NFULNL_CFG_CMD_PF_UNBIND:
-			nf_log_unregister_pf(pf);
+			nf_log_unbind_pf(pf);
 			return 0;
 		}
 	}
@@ -952,6 +952,11 @@ static int __init nfnetlink_log_init(void)
 		goto cleanup_netlink_notifier;
 	}
 
+	if ((status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger)) < 0) {
+		printk(KERN_ERR "log: failed to register logger\n");
+		goto cleanup_netlink_notifier;
+	}
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nfnetlink_log", 0440,
 			 proc_net_netfilter, &nful_file_ops))
-- 
1.5.6.3

[PATCH 2/4] netfilter: suppress now unused nf_log_unregister_pf() function.

[Eric Leblond]

This patch suppresses the now unused nf_log_unregister_pf() function
which is as been replaced by the bind and unbind functions.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 50d34db..1271b4f 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -69,19 +69,6 @@ int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 }
 EXPORT_SYMBOL(nf_log_register);
 
-void nf_log_unregister_pf(u_int8_t pf)
-{
-	if (pf >= ARRAY_SIZE(nf_loggers))
-		return;
-	mutex_lock(&nf_log_mutex);
-	rcu_assign_pointer(nf_loggers[pf], NULL);
-	mutex_unlock(&nf_log_mutex);
-
-	/* Give time to concurrent readers. */
-	synchronize_rcu();
-}
-EXPORT_SYMBOL(nf_log_unregister_pf);
-
 void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
-- 
1.5.6.3

[PATCH 3/4] netfilter: print the list of register loggers.

[Eric Leblond]

This patch modifies the proc output to add display of registered
loggers. The content of /proc/net/netfilter/nf_log is modified. Instead
of displaying a protocol per line with format:
	proto:logger
it now displays:
	proto:logger (comma_separated_list_of_loggers)
NONE is used as keyword if no logger is used.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 1271b4f..f9e6cad 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -168,13 +168,37 @@ static int seq_show(struct seq_file *s, void *v)
 {
 	loff_t *pos = v;
 	const struct nf_logger *logger;
+	struct nf_logger *t;
+	int ret;
 
 	logger = rcu_dereference(nf_loggers[*pos]);
 
 	if (!logger)
-		return seq_printf(s, "%2lld NONE\n", *pos);
+		ret = seq_printf(s, "%2lld NONE (", *pos);
+	else
+		ret = seq_printf(s, "%2lld %s (", *pos, logger->name);
+
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&nf_log_mutex);
+	list_for_each_entry(t, &nf_loggers_l[*pos], list[*pos]) {
+		ret = seq_printf(s, "%s", t->name);
+		if (ret < 0) {
+			mutex_unlock(&nf_log_mutex);
+			return ret;
+		}
+		if (&t->list[*pos] != nf_loggers_l[*pos].prev) {
+			ret = seq_printf(s, ",");
+			if (ret < 0) {
+				mutex_unlock(&nf_log_mutex);
+				return ret;
+			}
+		}
+	}
+	mutex_unlock(&nf_log_mutex);
 
-	return seq_printf(s, "%2lld %s\n", *pos, logger->name);
+	return seq_printf(s, ")\n");
 }
 
 static const struct seq_operations nflog_seq_ops = {
-- 
1.5.6.3

[PATCH 4/4] netfilter: sysctl support of logger choice.

[Eric Leblond]

This patchs adds support of modification of the used logger via sysctl.
It can be used to change the logger to module that can not use the bind
operation (ipt_LOG and ipt_ULOG). For this purpose, it creates a
directory /proc/sys/net/netfilter/nf_log which contains a file
per-protocol. The content of the file is the name current logger (NONE if
not set) and a logger can be setup by simply echoing its name to the file.
By echoing "NONE" to a /proc/sys/net/netfilter/nf_log/PROTO file, the
logger corresponding to this PROTO is set to NULL.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index f9e6cad..fa88c9b 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,6 +14,7 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
+#define NFLOGGER_NAME_LEN		64
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
 static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
@@ -223,6 +224,76 @@ static const struct file_operations nflog_file_ops = {
 
 #endif /* PROC_FS */
 
+#ifdef CONFIG_SYSCTL
+struct ctl_path nf_log_sysctl_path[] = {
+	{ .procname = "net", .ctl_name = CTL_NET, },
+	{ .procname = "netfilter", .ctl_name = NET_NETFILTER, },
+	{ .procname = "nf_log", .ctl_name = CTL_UNNUMBERED, },
+	{ }
+};
+
+static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
+static struct ctl_table_header *nf_log_dir_header;
+
+static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
+			 void *buffer, size_t *lenp, loff_t *ppos)
+{
+	const struct nf_logger *logger;
+	int r = 0;
+
+	if (write) {
+		if (!strnicmp(buffer, "NONE", *lenp - 1)) {
+			return nf_log_unbind_pf(table->ctl_name);
+		}
+		mutex_lock(&nf_log_mutex);
+		logger = __find_logger(table->ctl_name, buffer);
+		if (logger == NULL) {
+			mutex_unlock(&nf_log_mutex);
+			return -EINVAL;
+		}
+		rcu_assign_pointer(nf_loggers[table->ctl_name], logger);
+		mutex_unlock(&nf_log_mutex);
+	} else {
+		rcu_read_lock();
+		logger = rcu_dereference(nf_loggers[table->ctl_name]);
+		if (! logger)
+			table->data = "NONE";
+		else
+			table->data = logger->name;
+		r = proc_dostring(table, write, filp, buffer, lenp, ppos);
+		rcu_read_unlock();
+	}
+
+	return r;
+}
+
+static int netfilter_log_sysctl_init(void)
+{
+	int i;
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		char *pr_name = kmalloc(3, GFP_KERNEL);
+		/* FIXME free at deinit but there is currently no deinit */
+		snprintf(pr_name, 3, "%d", i);
+		nf_log_sysctl_table[i].ctl_name	= i;
+		nf_log_sysctl_table[i].procname	= pr_name;
+		nf_log_sysctl_table[i].data = NULL;
+		nf_log_sysctl_table[i].maxlen = NFLOGGER_NAME_LEN * sizeof(char);
+		nf_log_sysctl_table[i].mode = 0644;
+		nf_log_sysctl_table[i].proc_handler = nf_log_proc_dostring;
+	}
+
+	nf_log_dir_header = register_sysctl_paths(nf_log_sysctl_path,
+				       nf_log_sysctl_table);
+
+	return 0;
+}
+#else
+static int netfilter_log_sysctl_init(void)
+{
+	return 0;
+}
+#endif /* CONFIG_SYSCTL */
 
 int __init netfilter_log_init(void)
 {
@@ -233,6 +304,9 @@ int __init netfilter_log_init(void)
 		return -1;
 #endif
 
+	if (netfilter_log_sysctl_init() < 0)
+		return -1;
+
 	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
 		INIT_LIST_HEAD(&(nf_loggers_l[i]));
 
-- 
1.5.6.3

Re: [PATCH 3/4] netfilter: print the list of register loggers.

[Jan Engelhardt]

On Sunday 2009-02-15 13:37, Eric Leblond wrote:
>--- a/net/netfilter/nf_log.c
>+++ b/net/netfilter/nf_log.c
>@@ -168,13 +168,37 @@ static int seq_show(struct seq_file *s, void *v)
> {
> 	loff_t *pos = v;
> 	const struct nf_logger *logger;
>+	struct nf_logger *t;
>+	int ret;
> 
> 	logger = rcu_dereference(nf_loggers[*pos]);
> 
> 	if (!logger)
>-		return seq_printf(s, "%2lld NONE\n", *pos);
>+		ret = seq_printf(s, "%2lld NONE (", *pos);
>+	else
>+		ret = seq_printf(s, "%2lld %s (", *pos, logger->name);
>+
>+	if (ret < 0)
>+		return ret;
>+
>+	mutex_lock(&nf_log_mutex);
>+	list_for_each_entry(t, &nf_loggers_l[*pos], list[*pos]) {
>+		ret = seq_printf(s, "%s", t->name);

The seqprinting (knowingly that I suggested it) has a bit of a drawback
the way you use it -- if userspace passes a buffer of, say, size 11,
then the first seq_printf will succeed in stuffing " 1 NONE (" into
the buffer; it then has 2 left. Because t->name is probably more descriptive
than that, the second seq_printf will fail, breaking out of the loop,
calling seq_stop, etc. and userspace gets 9 as the return value for
the read() it attempted. However, when userspace now tries to read()
again, as it is absolutely entitled to, the " 1 NONE (" will be printed
again into the buffer -- which is rather undesirable.

I see zero ways around this:

 a. use snprintf to build the string, and then do a single
    final seq_printf
    Drawback: determine size of buffer to use for snprintf -
    we do not want it too large not too small

 b. rework the *pos logic in a way so that it is checkpointed after
    every seq_printf.
    Drawback: It could become ugly, much like the regression fix
    I've sent in.

Maybe the authors of seq_file can offer some better function that
takes care of this?

Re: [PATCH 3/4] netfilter: print the list of register loggers.

[Patrick McHardy]

Jan Engelhardt wrote:
> The seqprinting (knowingly that I suggested it) has a bit of a drawback
> the way you use it -- if userspace passes a buffer of, say, size 11,
> then the first seq_printf will succeed in stuffing " 1 NONE (" into
> the buffer; it then has 2 left. Because t->name is probably more descriptive
> than that, the second seq_printf will fail, breaking out of the loop,
> calling seq_stop, etc. and userspace gets 9 as the return value for
> the read() it attempted. However, when userspace now tries to read()
> again, as it is absolutely entitled to, the " 1 NONE (" will be printed
> again into the buffer -- which is rather undesirable.
> ...
> Maybe the authors of seq_file can offer some better function that
> takes care of this?

seqfile operates on records not characters and always provides
one page of memory.

Re: [PATCH 4/4] netfilter: sysctl support of logger choice.

[Patrick McHardy]

Eric Leblond wrote:
> +static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
> +			 void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	const struct nf_logger *logger;
> +	int r = 0;
> +
> +	if (write) {
> +		if (!strnicmp(buffer, "NONE", *lenp - 1)) {
> +			return nf_log_unbind_pf(table->ctl_name);
> +		}
> +		mutex_lock(&nf_log_mutex);
> +		logger = __find_logger(table->ctl_name, buffer);
> +	...

> +static int netfilter_log_sysctl_init(void)
> +{
> +	int i;
> +
> +	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
> +		char *pr_name = kmalloc(3, GFP_KERNEL);
> +		/* FIXME free at deinit but there is currently no deinit */
> +		snprintf(pr_name, 3, "%d", i);
> +		nf_log_sysctl_table[i].ctl_name	= i;

This ctl_name use seems rather abusive. The names are used
as numeric identifiers, we can't just stuff index values in
there :)

Re: [PATCH 1/4] netfilter: use a linked list of loggers.

[Patrick McHardy]

Eric Leblond wrote:
> +/* return EEXIST if the same logger is registred, 0 on success. */
> +int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  {
>  	int ret;
> +	const struct nf_logger *llog;
>  
>  	if (pf >= ARRAY_SIZE(nf_loggers))
>  		return -EINVAL;
>  
> -	/* Any setup of logging members must be done before
> -	 * substituting pointer. */
> -	ret = mutex_lock_interruptible(&nf_log_mutex);
> -	if (ret < 0)
> -		return ret;
> +	mutex_lock(&nf_log_mutex);
>  
> -	if (!nf_loggers[pf])
> -		rcu_assign_pointer(nf_loggers[pf], logger);
> -	else if (nf_loggers[pf] == logger)
> -		ret = -EEXIST;
> -	else
> -		ret = -EBUSY;
> +	ret = -EEXIST;
> +	if (pf == NFPROTO_UNSPEC) {
> +		int i;
> +		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
> +			if (__find_logger(i, logger->name) == NULL) {
> +				ret = 0;
> +				list_add_tail(&(logger->list[i]),
> +						&(nf_loggers_l[i]));
> +			}
> +	} else {
> +		if (__find_logger(pf, logger->name) == NULL) {
> +			ret = 0;
> +			/* register at end of list to honor first register win */
> +			list_add_tail(&(logger->list[pf]),
> +					  &nf_loggers_l[pf]);
> +			llog = rcu_dereference(nf_loggers[pf]);
> +			if (llog == NULL)
> +				rcu_assign_pointer(nf_loggers[pf], logger);
> +		}
> +	}

As the loggers are only registered statically by kernel modules,
I think this check is overkill. Just add them and let it crash
if someone does two registrations of the same logger, it won't
take long to notice. Similar for unregistration, its not worth
the trouble to check whether its actually registered, especially
since the list deletion doesn't even need to know. Just NULL
out the affected nf_loggers pointers and it should be fine.

Sorry for not saying this earlier.

Re: [PATCH 2/4] netfilter: suppress now unused nf_log_unregister_pf() function.

[Patrick McHardy]

Eric Leblond wrote:
> This patch suppresses the now unused nf_log_unregister_pf() function
> which is as been replaced by the bind and unbind functions.

Please fold this one into patch 1, which already replaces its
use and removes the declaration.

Re: [PATCH 4/4] netfilter: sysctl support of logger choice.

[Eric Leblond]

Hi,

Patrick McHardy a écrit :
> Eric Leblond wrote:
>> +static int nf_log_proc_dostring(ctl_table *table, int write, struct
>> file *filp,
>
>> +    for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
>> +        char *pr_name = kmalloc(3, GFP_KERNEL);
>> +        /* FIXME free at deinit but there is currently no deinit */
>> +        snprintf(pr_name, 3, "%d", i);
>> +        nf_log_sysctl_table[i].ctl_name    = i;
> 
> This ctl_name use seems rather abusive. The names are used
> as numeric identifiers, we can't just stuff index values in
> there :)

Your remark is not clear for me. I store here a integer which is the
protocol number. I see it as a real numeric identifier.

Which change are you suggesting ?

BR,
-- 
Eric Leblond <eric@inl.fr>
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] netfilter: sysctl support of logger choice.

[Patrick McHardy]

Eric Leblond wrote:
> Hi,
>
> Patrick McHardy a écrit :
>   
>> Eric Leblond wrote:
>>     
>>> +static int nf_log_proc_dostring(ctl_table *table, int write, struct
>>> file *filp,
>>>       
>>> +    for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
>>> +        char *pr_name = kmalloc(3, GFP_KERNEL);
>>> +        /* FIXME free at deinit but there is currently no deinit */
>>> +        snprintf(pr_name, 3, "%d", i);
>>> +        nf_log_sysctl_table[i].ctl_name    = i;
>>>       
>> This ctl_name use seems rather abusive. The names are used
>> as numeric identifiers, we can't just stuff index values in
>> there :)
>>     
>
> Your remark is not clear for me. I store here a integer which is the
> protocol number. I see it as a real numeric identifier.
>
> Which change are you suggesting ?

Its an identifier for the sysctl syscall, its not meant to be used with
private identifiers. And any new sysctl should use CTL_UNNUMBERED (which
you do, but then overwrite it).


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] netfilter: sysctl support of logger choice.

[Eric Leblond]

Hi,

Patrick McHardy a écrit :
> Eric Leblond wrote:
>> Hi,
>>
>> Patrick McHardy a écrit :
>>   
>>> Eric Leblond wrote:
>>>     
>>>> +static int nf_log_proc_dostring(ctl_table *table, int write, struct
>>>> file *filp,
>>>>       
>>>> +    for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
>>>> +        char *pr_name = kmalloc(3, GFP_KERNEL);
>>>> +        /* FIXME free at deinit but there is currently no deinit */
>>>> +        snprintf(pr_name, 3, "%d", i);
>>>> +        nf_log_sysctl_table[i].ctl_name    = i;
>>>>       
>>> This ctl_name use seems rather abusive. The names are used
>>> as numeric identifiers, we can't just stuff index values in
>>> there :)
>>>     
>> Your remark is not clear for me. I store here a integer which is the
>> protocol number. I see it as a real numeric identifier.
>>
>> Which change are you suggesting ?
> 
> Its an identifier for the sysctl syscall, its not meant to be used with
> private identifiers. And any new sysctl should use CTL_UNNUMBERED (which
> you do, but then overwrite it).

Ok, once you've said it, it quiet obvious :/

A reworked patchset will follow as reply to this mail.

Thanks once again for your remarks and your patience.

BR,
-- 
Eric Leblond <eric@inl.fr>
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] netfilter: use a linked list of loggers.

[Eric Leblond]

This patch modifies nf_log to use a linked list of loggers for each
protocol. This list of loggers is read and write protected with a
mutex.
This patch separates registration and binding. To be used as
logging module, a module has to register calling nf_log_register()
and to bind to a protocol it has to call nf_log_bind_pf().
This patch also converts the logging modules to the new API. For nfnetlink_log,
it simply switchs call to register functions to call to bind function and
adds a call to nf_log_register() during init. For other modules, it just
remove a const flag from the logger structure and replace it with a
__read_mostly.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 include/net/netfilter/nf_log.h |   11 ++++-
 net/ipv4/netfilter/ipt_LOG.c   |    2 +-
 net/ipv4/netfilter/ipt_ULOG.c  |    2 +-
 net/ipv6/netfilter/ip6t_LOG.c  |    2 +-
 net/netfilter/nf_log.c         |   90 ++++++++++++++++++++++++++-------------
 net/netfilter/nfnetlink_log.c  |   11 ++++-
 6 files changed, 79 insertions(+), 39 deletions(-)

diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index 7182c06..ccaa43a 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -1,6 +1,8 @@
 #ifndef _NF_LOG_H
 #define _NF_LOG_H
 
+#include <linux/netfilter.h>
+
 /* those NF_LOG_* defines and struct nf_loginfo are legacy definitios that will
  * disappear once iptables is replaced with pkttables.  Please DO NOT use them
  * for any new code! */
@@ -40,12 +42,15 @@ struct nf_logger {
 	struct module	*me;
 	nf_logfn 	*logfn;
 	char		*name;
+	struct list_head	list[NFPROTO_NUMPROTO];
 };
 
 /* Function to register/unregister log function. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger);
-void nf_log_unregister(const struct nf_logger *logger);
-void nf_log_unregister_pf(u_int8_t pf);
+int nf_log_register(u_int8_t pf, struct nf_logger *logger);
+void nf_log_unregister(struct nf_logger *logger);
+
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger);
+int nf_log_unbind_pf(u_int8_t pf);
 
 /* Calls the registered backend logging function */
 void nf_log_packet(u_int8_t pf,
diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c
index 27a78fb..acc44c6 100644
--- a/net/ipv4/netfilter/ipt_LOG.c
+++ b/net/ipv4/netfilter/ipt_LOG.c
@@ -464,7 +464,7 @@ static struct xt_target log_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static const struct nf_logger ipt_log_logger ={
+static struct nf_logger ipt_log_logger __read_mostly = {
 	.name		= "ipt_LOG",
 	.logfn		= &ipt_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index 18a2826..d32cc4b 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -379,7 +379,7 @@ static struct xt_target ulog_tg_reg __read_mostly = {
 	.me		= THIS_MODULE,
 };
 
-static struct nf_logger ipt_ulog_logger = {
+static struct nf_logger ipt_ulog_logger __read_mostly = {
 	.name		= "ipt_ULOG",
 	.logfn		= ipt_logfn,
 	.me		= THIS_MODULE,
diff --git a/net/ipv6/netfilter/ip6t_LOG.c b/net/ipv6/netfilter/ip6t_LOG.c
index 37adf5a..7018cac 100644
--- a/net/ipv6/netfilter/ip6t_LOG.c
+++ b/net/ipv6/netfilter/ip6t_LOG.c
@@ -477,7 +477,7 @@ static struct xt_target log_tg6_reg __read_mostly = {
 	.me 		= THIS_MODULE,
 };
 
-static const struct nf_logger ip6t_logger = {
+static struct nf_logger ip6t_logger __read_mostly = {
 	.name		= "ip6t_LOG",
 	.logfn		= &ip6t_log_packet,
 	.me		= THIS_MODULE,
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index fa8ae5d..96cc2f4 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -16,56 +16,59 @@
 #define NF_LOG_PREFIXLEN		128
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
+static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
 static DEFINE_MUTEX(nf_log_mutex);
 
-/* return EBUSY if somebody else is registered, EEXIST if the same logger
- * is registred, 0 on success. */
-int nf_log_register(u_int8_t pf, const struct nf_logger *logger)
+static struct nf_logger *__find_logger(int pf, const char *str_logger)
 {
-	int ret;
+	struct nf_logger *t;
 
-	if (pf >= ARRAY_SIZE(nf_loggers))
-		return -EINVAL;
-
-	/* Any setup of logging members must be done before
-	 * substituting pointer. */
-	ret = mutex_lock_interruptible(&nf_log_mutex);
-	if (ret < 0)
-		return ret;
+	list_for_each_entry(t, &nf_loggers_l[pf], list[pf])
+		if (!strnicmp(str_logger, t->name, strlen(t->name))) {
+			return t;
+		}
 
-	if (!nf_loggers[pf])
-		rcu_assign_pointer(nf_loggers[pf], logger);
-	else if (nf_loggers[pf] == logger)
-		ret = -EEXIST;
-	else
-		ret = -EBUSY;
-
-	mutex_unlock(&nf_log_mutex);
-	return ret;
+	return NULL;
 }
-EXPORT_SYMBOL(nf_log_register);
 
-void nf_log_unregister_pf(u_int8_t pf)
+/* return EEXIST if the same logger is registred, 0 on success. */
+int nf_log_register(u_int8_t pf, struct nf_logger *logger)
 {
+	const struct nf_logger *llog;
+
 	if (pf >= ARRAY_SIZE(nf_loggers))
-		return;
+		return -EINVAL;
+
 	mutex_lock(&nf_log_mutex);
-	rcu_assign_pointer(nf_loggers[pf], NULL);
+
+	if (pf == NFPROTO_UNSPEC) {
+		int i;
+		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
+			list_add_tail(&(logger->list[i]), &(nf_loggers_l[i]));
+	} else {
+		/* register at end of list to honor first register win */
+		list_add_tail(&(logger->list[pf]), &nf_loggers_l[pf]);
+		llog = rcu_dereference(nf_loggers[pf]);
+		if (llog == NULL)
+			rcu_assign_pointer(nf_loggers[pf], logger);
+	}
+
 	mutex_unlock(&nf_log_mutex);
 
-	/* Give time to concurrent readers. */
-	synchronize_rcu();
+	return 0;
 }
-EXPORT_SYMBOL(nf_log_unregister_pf);
+EXPORT_SYMBOL(nf_log_register);
 
-void nf_log_unregister(const struct nf_logger *logger)
+void nf_log_unregister(struct nf_logger *logger)
 {
 	int i;
 
 	mutex_lock(&nf_log_mutex);
 	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
-		if (nf_loggers[i] == logger)
+		const struct nf_logger *c_logger = rcu_dereference(nf_loggers[i]);
+		if (c_logger == logger)
 			rcu_assign_pointer(nf_loggers[i], NULL);
+		list_del(&(logger->list[i]));
 	}
 	mutex_unlock(&nf_log_mutex);
 
@@ -73,6 +76,28 @@ void nf_log_unregister(const struct nf_logger *logger)
 }
 EXPORT_SYMBOL(nf_log_unregister);
 
+int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger)
+{
+	mutex_lock(&nf_log_mutex);
+	if (__find_logger(pf, logger->name) == NULL) {
+		mutex_unlock(&nf_log_mutex);
+		return -1;
+	}
+	rcu_assign_pointer(nf_loggers[pf], logger);
+	mutex_unlock(&nf_log_mutex);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_bind_pf);
+
+int nf_log_unbind_pf(u_int8_t pf)
+{
+	mutex_lock(&nf_log_mutex);
+	rcu_assign_pointer(nf_loggers[pf], NULL);
+	mutex_unlock(&nf_log_mutex);
+	return 0;
+}
+EXPORT_SYMBOL(nf_log_unbind_pf);
+
 void nf_log_packet(u_int8_t pf,
 		   unsigned int hooknum,
 		   const struct sk_buff *skb,
@@ -163,10 +188,15 @@ static const struct file_operations nflog_file_ops = {
 
 int __init netfilter_log_init(void)
 {
+	int i;
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
+		INIT_LIST_HEAD(&(nf_loggers_l[i]));
+
 	return 0;
 }
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index c712e9f..42e7de9 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -693,7 +693,7 @@ nfulnl_recv_unsupp(struct sock *ctnl, struct sk_buff *skb,
 	return -ENOTSUPP;
 }
 
-static const struct nf_logger nfulnl_logger = {
+static struct nf_logger nfulnl_logger __read_mostly = {
 	.name	= "nfnetlink_log",
 	.logfn	= &nfulnl_log_packet,
 	.me	= THIS_MODULE,
@@ -725,9 +725,9 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		/* Commands without queue context */
 		switch (cmd->command) {
 		case NFULNL_CFG_CMD_PF_BIND:
-			return nf_log_register(pf, &nfulnl_logger);
+			return nf_log_bind_pf(pf, &nfulnl_logger);
 		case NFULNL_CFG_CMD_PF_UNBIND:
-			nf_log_unregister_pf(pf);
+			nf_log_unbind_pf(pf);
 			return 0;
 		}
 	}
@@ -952,6 +952,11 @@ static int __init nfnetlink_log_init(void)
 		goto cleanup_netlink_notifier;
 	}
 
+	if ((status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger)) < 0) {
+		printk(KERN_ERR "log: failed to register logger\n");
+		goto cleanup_netlink_notifier;
+	}
+
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nfnetlink_log", 0440,
 			 proc_net_netfilter, &nful_file_ops))
-- 
1.5.6.3

[PATCH 2/3] netfilter: print the list of register loggers.

[Eric Leblond]

This patch modifies the proc output to add display of registered
loggers. The content of /proc/net/netfilter/nf_log is modified. Instead
of displaying a protocol per line with format:
	proto:logger
it now displays:
	proto:logger (comma_separated_list_of_loggers)
NONE is used as keyword if no logger is used.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 96cc2f4..efda4ba 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -154,13 +154,37 @@ static int seq_show(struct seq_file *s, void *v)
 {
 	loff_t *pos = v;
 	const struct nf_logger *logger;
+	struct nf_logger *t;
+	int ret;
 
 	logger = rcu_dereference(nf_loggers[*pos]);
 
 	if (!logger)
-		return seq_printf(s, "%2lld NONE\n", *pos);
+		ret = seq_printf(s, "%2lld NONE (", *pos);
+	else
+		ret = seq_printf(s, "%2lld %s (", *pos, logger->name);
+
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&nf_log_mutex);
+	list_for_each_entry(t, &nf_loggers_l[*pos], list[*pos]) {
+		ret = seq_printf(s, "%s", t->name);
+		if (ret < 0) {
+			mutex_unlock(&nf_log_mutex);
+			return ret;
+		}
+		if (&t->list[*pos] != nf_loggers_l[*pos].prev) {
+			ret = seq_printf(s, ",");
+			if (ret < 0) {
+				mutex_unlock(&nf_log_mutex);
+				return ret;
+			}
+		}
+	}
+	mutex_unlock(&nf_log_mutex);
 
-	return seq_printf(s, "%2lld %s\n", *pos, logger->name);
+	return seq_printf(s, ")\n");
 }
 
 static const struct seq_operations nflog_seq_ops = {
-- 
1.5.6.3

[PATCH 3/3] netfilter: sysctl support of logger choice.

[Eric Leblond]

This patchs adds support of modification of the used logger via sysctl.
It can be used to change the logger to module that can not use the bind
operation (ipt_LOG and ipt_ULOG). For this purpose, it creates a
directory /proc/sys/net/netfilter/nf_log which contains a file
per-protocol. The content of the file is the name current logger (NONE if
not set) and a logger can be setup by simply echoing its name to the file.
By echoing "NONE" to a /proc/sys/net/netfilter/nf_log/PROTO file, the
logger corresponding to this PROTO is set to NULL.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index efda4ba..87f1d49 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,6 +14,7 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
+#define NFLOGGER_NAME_LEN		64
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
 static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
@@ -209,6 +210,78 @@ static const struct file_operations nflog_file_ops = {
 
 #endif /* PROC_FS */
 
+#ifdef CONFIG_SYSCTL
+struct ctl_path nf_log_sysctl_path[] = {
+	{ .procname = "net", .ctl_name = CTL_NET, },
+	{ .procname = "netfilter", .ctl_name = NET_NETFILTER, },
+	{ .procname = "nf_log", .ctl_name = CTL_UNNUMBERED, },
+	{ }
+};
+
+static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
+static struct ctl_table_header *nf_log_dir_header;
+
+static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
+			 void *buffer, size_t *lenp, loff_t *ppos)
+{
+	const struct nf_logger *logger;
+	int r = 0;
+	int tindex = (unsigned long) table->extra1;
+
+	if (write) {
+		if (!strnicmp(buffer, "NONE", *lenp - 1)) {
+			return nf_log_unbind_pf(tindex);
+		}
+		mutex_lock(&nf_log_mutex);
+		logger = __find_logger(tindex, buffer);
+		if (logger == NULL) {
+			mutex_unlock(&nf_log_mutex);
+			return -EINVAL;
+		}
+		rcu_assign_pointer(nf_loggers[tindex], logger);
+		mutex_unlock(&nf_log_mutex);
+	} else {
+		rcu_read_lock();
+		logger = rcu_dereference(nf_loggers[tindex]);
+		if (! logger)
+			table->data = "NONE";
+		else
+			table->data = logger->name;
+		r = proc_dostring(table, write, filp, buffer, lenp, ppos);
+		rcu_read_unlock();
+	}
+
+	return r;
+}
+
+static int netfilter_log_sysctl_init(void)
+{
+	int i;
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		char *pr_name = kmalloc(3, GFP_KERNEL);
+		/* FIXME free at deinit but there is currently no deinit */
+		snprintf(pr_name, 3, "%d", i);
+		nf_log_sysctl_table[i].ctl_name	= CTL_UNNUMBERED;
+		nf_log_sysctl_table[i].procname	= pr_name;
+		nf_log_sysctl_table[i].data = NULL;
+		nf_log_sysctl_table[i].maxlen = NFLOGGER_NAME_LEN * sizeof(char);
+		nf_log_sysctl_table[i].mode = 0644;
+		nf_log_sysctl_table[i].proc_handler = nf_log_proc_dostring;
+		nf_log_sysctl_table[i].extra1 = (void *)(unsigned long) i;
+	}
+
+	nf_log_dir_header = register_sysctl_paths(nf_log_sysctl_path,
+				       nf_log_sysctl_table);
+
+	return 0;
+}
+#else
+static int netfilter_log_sysctl_init(void)
+{
+	return 0;
+}
+#endif /* CONFIG_SYSCTL */
 
 int __init netfilter_log_init(void)
 {
@@ -219,6 +292,9 @@ int __init netfilter_log_init(void)
 		return -1;
 #endif
 
+	if (netfilter_log_sysctl_init() < 0)
+		return -1;
+
 	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
 		INIT_LIST_HEAD(&(nf_loggers_l[i]));
 
-- 
1.5.6.3

Re: [PATCH 1/3] netfilter: use a linked list of loggers.

[Patrick McHardy]

Eric Leblond wrote:
> This patch modifies nf_log to use a linked list of loggers for each
> protocol. This list of loggers is read and write protected with a
> mutex.
> This patch separates registration and binding. To be used as
> logging module, a module has to register calling nf_log_register()
> and to bind to a protocol it has to call nf_log_bind_pf().
> This patch also converts the logging modules to the new API. For nfnetlink_log,
> it simply switchs call to register functions to call to bind function and
> adds a call to nf_log_register() during init. For other modules, it just
> remove a const flag from the logger structure and replace it with a
> __read_mostly.

Applied, thanks. I've made a few changes, please see below.

> -void nf_log_unregister(const struct nf_logger *logger)
> +void nf_log_unregister(struct nf_logger *logger)
>  {
>  	int i;
>  
>  	mutex_lock(&nf_log_mutex);
>  	for (i = 0; i < ARRAY_SIZE(nf_loggers); i++) {
> -		if (nf_loggers[i] == logger)
> +		const struct nf_logger *c_logger = rcu_dereference(nf_loggers[i]);

I've moved the definitionn to the beginning of the function to
bring the line down to 80 characters.

> +		if (c_logger == logger)
>  			rcu_assign_pointer(nf_loggers[i], NULL);
> +		list_del(&(logger->list[i]));
>  	}
>  	mutex_unlock(&nf_log_mutex);
>  
> @@ -73,6 +76,28 @@ void nf_log_unregister(const struct nf_logger *logger)
>  }
>  EXPORT_SYMBOL(nf_log_unregister);
>  
> +int nf_log_bind_pf(u_int8_t pf, const struct nf_logger *logger)
> +{
> +	mutex_lock(&nf_log_mutex);
> +	if (__find_logger(pf, logger->name) == NULL) {
> +		mutex_unlock(&nf_log_mutex);
> +		return -1;

This is returned directly to userspace and thus needs to use
errno-codes. I've changed it to -ENOENT.

> +	}
> +	rcu_assign_pointer(nf_loggers[pf], logger);
> +	mutex_unlock(&nf_log_mutex);
> +	return 0;
> +}
> +EXPORT_SYMBOL(nf_log_bind_pf);
> +
> +int nf_log_unbind_pf(u_int8_t pf)
> +{
> +	mutex_lock(&nf_log_mutex);
> +	rcu_assign_pointer(nf_loggers[pf], NULL);
> +	mutex_unlock(&nf_log_mutex);
> +	return 0;
> +}
> +EXPORT_SYMBOL(nf_log_unbind_pf);

This should return void since there's nothing a caller can do
about it and the only existing one doesn't care.

> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> index c712e9f..42e7de9 100644
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c

> @@ -952,6 +952,11 @@ static int __init nfnetlink_log_init(void)
>  		goto cleanup_netlink_notifier;
>  	}
>  
> +	if ((status = nf_log_register(NFPROTO_UNSPEC, &nfulnl_logger)) < 0) {
> +		printk(KERN_ERR "log: failed to register logger\n");
> +		goto cleanup_netlink_notifier;

This needs to undo the nfnetlink subsys registration. I've fixed
it before applying.

Re: [PATCH 2/3] netfilter: print the list of register loggers.

[Patrick McHardy]

Eric Leblond wrote:
> This patch modifies the proc output to add display of registered
> loggers. The content of /proc/net/netfilter/nf_log is modified. Instead
> of displaying a protocol per line with format:
> 	proto:logger
> it now displays:
> 	proto:logger (comma_separated_list_of_loggers)
> NONE is used as keyword if no logger is used.

Applied, thanks.

Re: [PATCH 3/3] netfilter: sysctl support of logger choice.

[Patrick McHardy]

Eric Leblond wrote:
> +static int netfilter_log_sysctl_init(void)
> +{
> +	int i;
> +
> +	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
> +		char *pr_name = kmalloc(3, GFP_KERNEL);

This needs to check for allocation errors.

> +		/* FIXME free at deinit but there is currently no deinit */

And this needs to be fixed as well. If you want to simplify this, I'd
suggest to just use static storage.

> +		snprintf(pr_name, 3, "%d", i);
> +		nf_log_sysctl_table[i].ctl_name	= CTL_UNNUMBERED;
> +		nf_log_sysctl_table[i].procname	= pr_name;
> +		nf_log_sysctl_table[i].data = NULL;
> +		nf_log_sysctl_table[i].maxlen = NFLOGGER_NAME_LEN * sizeof(char);
> +		nf_log_sysctl_table[i].mode = 0644;
> +		nf_log_sysctl_table[i].proc_handler = nf_log_proc_dostring;
> +		nf_log_sysctl_table[i].extra1 = (void *)(unsigned long) i;
> +	}
> +
> +	nf_log_dir_header = register_sysctl_paths(nf_log_sysctl_path,
> +				       nf_log_sysctl_table);
> +
> +	return 0;
> +}
> +#else
> +static int netfilter_log_sysctl_init(void)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_SYSCTL */
>  
>  int __init netfilter_log_init(void)
>  {
> @@ -219,6 +292,9 @@ int __init netfilter_log_init(void)
>  		return -1;
>  #endif
>  
> +	if (netfilter_log_sysctl_init() < 0)
> +		return -1;

And this needs to use proper errno codes.

Well, nfnetlink_log_sysctl_init() should return proper errno codes
and this function should propagate them.

> +
>  	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
>  		INIT_LIST_HEAD(&(nf_loggers_l[i]));
>  

Re: [PATCH 3/3] netfilter: sysctl support of logger choice.

[Eric Leblond]

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

Hi,

Le lundi 16 mars 2009 à 14:58 +0100, Patrick McHardy a écrit :
> Eric Leblond wrote:
> > +static int netfilter_log_sysctl_init(void)
> > +{
> > +	int i;
> > +
> > +	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
> > +		char *pr_name = kmalloc(3, GFP_KERNEL);
> 
> This needs to check for allocation errors.
> 
> > +		/* FIXME free at deinit but there is currently no deinit */
> 
> And this needs to be fixed as well. If you want to simplify this, I'd
> suggest to just use static storage.

Thanks for your remarks and your patience, updated patch will follow
this mail.

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

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] netfilter: sysctl support of logger choice.

[Eric Leblond]

This patchs adds support of modification of the used logger via sysctl.
It can be used to change the logger to module that can not use the bind
operation (ipt_LOG and ipt_ULOG). For this purpose, it creates a
directory /proc/sys/net/netfilter/nf_log which contains a file
per-protocol. The content of the file is the name current logger (NONE if
not set) and a logger can be setup by simply echoing its name to the file.
By echoing "NONE" to a /proc/sys/net/netfilter/nf_log/PROTO file, the
logger corresponding to this PROTO is set to NULL.

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

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 4fcbcc7..4b0cab3 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -14,6 +14,7 @@
    LOG target modules */
 
 #define NF_LOG_PREFIXLEN		128
+#define NFLOGGER_NAME_LEN		64
 
 static const struct nf_logger *nf_loggers[NFPROTO_NUMPROTO] __read_mostly;
 static struct list_head nf_loggers_l[NFPROTO_NUMPROTO] __read_mostly;
@@ -207,18 +208,99 @@ static const struct file_operations nflog_file_ops = {
 	.release = seq_release,
 };
 
+
 #endif /* PROC_FS */
 
+#ifdef CONFIG_SYSCTL
+struct ctl_path nf_log_sysctl_path[] = {
+	{ .procname = "net", .ctl_name = CTL_NET, },
+	{ .procname = "netfilter", .ctl_name = NET_NETFILTER, },
+	{ .procname = "nf_log", .ctl_name = CTL_UNNUMBERED, },
+	{ }
+};
+
+static char nf_log_sysctl_fnames[NFPROTO_NUMPROTO-NFPROTO_UNSPEC][3];
+static struct ctl_table nf_log_sysctl_table[NFPROTO_NUMPROTO+1];
+static struct ctl_table_header *nf_log_dir_header;
 
-int __init netfilter_log_init(void)
+static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
+			 void *buffer, size_t *lenp, loff_t *ppos)
+{
+	const struct nf_logger *logger;
+	int r = 0;
+	int tindex = (unsigned long) table->extra1;
+
+	if (write) {
+		if (!strnicmp(buffer, "NONE", *lenp - 1)) {
+			nf_log_unbind_pf(tindex);
+			return 0;
+		}
+		mutex_lock(&nf_log_mutex);
+		logger = __find_logger(tindex, buffer);
+		if (logger == NULL) {
+			mutex_unlock(&nf_log_mutex);
+			return -EINVAL;
+		}
+		rcu_assign_pointer(nf_loggers[tindex], logger);
+		mutex_unlock(&nf_log_mutex);
+	} else {
+		rcu_read_lock();
+		logger = rcu_dereference(nf_loggers[tindex]);
+		if (! logger)
+			table->data = "NONE";
+		else
+			table->data = logger->name;
+		r = proc_dostring(table, write, filp, buffer, lenp, ppos);
+		rcu_read_unlock();
+	}
+
+	return r;
+}
+
+static int netfilter_log_sysctl_init(void)
 {
 	int i;
+
+	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++) {
+		snprintf(nf_log_sysctl_fnames[i-NFPROTO_UNSPEC], 3, "%d", i);
+		nf_log_sysctl_table[i].ctl_name	= CTL_UNNUMBERED;
+		nf_log_sysctl_table[i].procname	=
+			nf_log_sysctl_fnames[i-NFPROTO_UNSPEC];
+		nf_log_sysctl_table[i].data = NULL;
+		nf_log_sysctl_table[i].maxlen =
+			NFLOGGER_NAME_LEN * sizeof(char);
+		nf_log_sysctl_table[i].mode = 0644;
+		nf_log_sysctl_table[i].proc_handler = nf_log_proc_dostring;
+		nf_log_sysctl_table[i].extra1 = (void *)(unsigned long) i;
+	}
+
+	nf_log_dir_header = register_sysctl_paths(nf_log_sysctl_path,
+				       nf_log_sysctl_table);
+	if (!nf_log_dir_header)
+		return -EINVAL;
+
+	return 0;
+}
+#else
+static int netfilter_log_sysctl_init(void)
+{
+	return 0;
+}
+#endif /* CONFIG_SYSCTL */
+
+int __init netfilter_log_init(void)
+{
+	int i, r;
 #ifdef CONFIG_PROC_FS
 	if (!proc_create("nf_log", S_IRUGO,
 			 proc_net_netfilter, &nflog_file_ops))
 		return -1;
 #endif
 
+	r = netfilter_log_sysctl_init();
+	if (r < 0)
+		return r;
+
 	for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
 		INIT_LIST_HEAD(&(nf_loggers_l[i]));
 
-- 
1.5.6.3

Re: [PATCH] netfilter: sysctl support of logger choice.

[Patrick McHardy]

Eric Leblond wrote:
> This patchs adds support of modification of the used logger via sysctl.
> It can be used to change the logger to module that can not use the bind
> operation (ipt_LOG and ipt_ULOG). For this purpose, it creates a
> directory /proc/sys/net/netfilter/nf_log which contains a file
> per-protocol. The content of the file is the name current logger (NONE if
> not set) and a logger can be setup by simply echoing its name to the file.
> By echoing "NONE" to a /proc/sys/net/netfilter/nf_log/PROTO file, the
> logger corresponding to this PROTO is set to NULL.
> 
> Signed-off-by: Eric Leblond <eric@inl.fr>

This seems fine apart from a few minor issues. No need to resend, just
let me know whether you agree to my changes. Thanks.

> -int __init netfilter_log_init(void)
> +static int nf_log_proc_dostring(ctl_table *table, int write, struct file *filp,
> +			 void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	const struct nf_logger *logger;
> +	int r = 0;
> +	int tindex = (unsigned long) table->extra1;
> +
> +	if (write) {
> +		if (!strnicmp(buffer, "NONE", *lenp - 1)) {

What is the motivation here for not doing a full comparison?
Suggested change: use strcmp

> +			nf_log_unbind_pf(tindex);
> +			return 0;
> +		}
> +		mutex_lock(&nf_log_mutex);
> +		logger = __find_logger(tindex, buffer);
> +		if (logger == NULL) {
> +			mutex_unlock(&nf_log_mutex);
> +			return -EINVAL;

ENOENT seems more approriate.

> +static int netfilter_log_sysctl_init(void)

__init?

> +int __init netfilter_log_init(void)
> +{
> +	int i, r;
>  #ifdef CONFIG_PROC_FS
>  	if (!proc_create("nf_log", S_IRUGO,
>  			 proc_net_netfilter, &nflog_file_ops))
>  		return -1;
>  #endif
>  
> +	r = netfilter_log_sysctl_init();
> +	if (r < 0)
> +		return r;

I previously didn't realize an error will cause a panic. I'll add a
comment explaning why unrolling is unnecessary.

Re: [PATCH] netfilter: sysctl support of logger choice.

[Eric Leblond]

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

Hi,

Le jeudi 19 mars 2009 à 10:45 +0100, Patrick McHardy a écrit :
> Eric Leblond wrote:
> > This patchs adds support of modification of the used logger via sysctl.
...
> > By echoing "NONE" to a /proc/sys/net/netfilter/nf_log/PROTO file, the
> > logger corresponding to this PROTO is set to NULL.
> > 
...
> > +	if (write) {
> > +		if (!strnicmp(buffer, "NONE", *lenp - 1)) {
> 
> What is the motivation here for not doing a full comparison?
> Suggested change: use strcmp

Hmmm that's correct. There is no motivation at all for insensitive
check. I should have done this.

The rest of the modifications are ok for me.

...
> > +		return r;
> 
> I previously didn't realize an error will cause a panic. I'll add a
> comment explaning why unrolling is unnecessary.

OK, interesting point.

BR,
-- 
Eric Leblond <eric@inl.fr>
INL SARL

[-- Attachment #2: Ceci est une partie de message numériquement signée --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] netfilter: sysctl support of logger choice.

[Patrick McHardy]

Eric Leblond wrote:
> Hi,
> 
> Le jeudi 19 mars 2009 à 10:45 +0100, Patrick McHardy a écrit :
>> Eric Leblond wrote:
>>> This patchs adds support of modification of the used logger via sysctl.
> ...
>>> By echoing "NONE" to a /proc/sys/net/netfilter/nf_log/PROTO file, the
>>> logger corresponding to this PROTO is set to NULL.
>>>
> ...
>>> +	if (write) {
>>> +		if (!strnicmp(buffer, "NONE", *lenp - 1)) {
>> What is the motivation here for not doing a full comparison?
>> Suggested change: use strcmp
> 
> Hmmm that's correct. There is no motivation at all for insensitive
> check. I should have done this.
> 
> The rest of the modifications are ok for me.

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html