[RFC 0/1] netfilter: xtables: xt_condition inclusion with namespace fix

[RFC 0/1] netfilter: xtables: xt_condition inclusion with namespace fix

[Luciano Coelho]

Hi,

This is a respin of the patch Jan sent to the list some time ago.  I've made
the changes proposed by Patrick in order to support multiple namespaces
correctly.

I still need to reapply my condition target and the u32 changes to the
condition on top of this, but I'd like to get some comments before I continue.

Please let me know how this looks.

Cheers,
Luca.

Luciano Coelho (1):
  netfilter: xtables: inclusion of xt_condition

 include/linux/netfilter/Kbuild         |    1 +
 include/linux/netfilter/xt_condition.h |   14 ++
 net/netfilter/Kconfig                  |    8 +
 net/netfilter/Makefile                 |    1 +
 net/netfilter/xt_condition.c           |  294 ++++++++++++++++++++++++++++++++
 5 files changed, 318 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/netfilter/xt_condition.h
 create mode 100644 net/netfilter/xt_condition.c

[RFC 1/1] netfilter: xtables: inclusion of xt_condition

[Luciano Coelho]

xt_condition can be used by userspace to influence decisions in rules
by means of togglable variables without having to reload the entire
ruleset.

This is a respin of the module in Xtables-addons, with support for
multiple namespaces and other small improvements.

Cc: Jan Engelhardt <jengelh@medozas.de>
Signed-off-by: Luciano Coelho <luciano.coelho@nokia.com>
---
 include/linux/netfilter/Kbuild         |    1 +
 include/linux/netfilter/xt_condition.h |   14 ++
 net/netfilter/Kconfig                  |    8 +
 net/netfilter/Makefile                 |    1 +
 net/netfilter/xt_condition.c           |  294 ++++++++++++++++++++++++++++++++
 5 files changed, 318 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/netfilter/xt_condition.h
 create mode 100644 net/netfilter/xt_condition.c

diff --git a/include/linux/netfilter/Kbuild b/include/linux/netfilter/Kbuild
index bb103f4..d873f67 100644
--- a/include/linux/netfilter/Kbuild
+++ b/include/linux/netfilter/Kbuild
@@ -20,6 +20,7 @@ header-y += xt_TCPOPTSTRIP.h
 header-y += xt_TEE.h
 header-y += xt_TPROXY.h
 header-y += xt_comment.h
+header-y += xt_condition.h
 header-y += xt_connbytes.h
 header-y += xt_connlimit.h
 header-y += xt_connmark.h
diff --git a/include/linux/netfilter/xt_condition.h b/include/linux/netfilter/xt_condition.h
new file mode 100644
index 0000000..4faf3ca
--- /dev/null
+++ b/include/linux/netfilter/xt_condition.h
@@ -0,0 +1,14 @@
+#ifndef _XT_CONDITION_H
+#define _XT_CONDITION_H
+
+#include <linux/types.h>
+
+struct xt_condition_mtinfo {
+	char name[31];
+	__u8 invert;
+
+	/* Used internally by the kernel */
+	void *condvar __attribute__((aligned(8)));
+};
+
+#endif /* _XT_CONDITION_H */
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index aa2f106..8c114b8 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -605,6 +605,14 @@ config NETFILTER_XT_MATCH_COMMENT
 	  If you want to compile it as a module, say M here and read
 	  <file:Documentation/kbuild/modules.txt>.  If unsure, say `N'.
 
+config NETFILTER_XT_MATCH_CONDITION
+	tristate '"condition" match support'
+	depends on NETFILTER_ADVANCED
+	depends on PROC_FS
+	---help---
+	This option allows you to match firewall rules against condition
+	variables stored in the /proc/net/nf_condition directory.
+
 config NETFILTER_XT_MATCH_CONNBYTES
 	tristate  '"connbytes" per-connection counter match support'
 	depends on NF_CONNTRACK
diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
index e28420a..474dd06 100644
--- a/net/netfilter/Makefile
+++ b/net/netfilter/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_NETFILTER_XT_TARGET_IDLETIMER) += xt_IDLETIMER.o
 # matches
 obj-$(CONFIG_NETFILTER_XT_MATCH_CLUSTER) += xt_cluster.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_COMMENT) += xt_comment.o
+obj-$(CONFIG_NETFILTER_XT_MATCH_CONDITION) += xt_condition.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CONNBYTES) += xt_connbytes.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CONNLIMIT) += xt_connlimit.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CONNTRACK) += xt_conntrack.o
diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c
new file mode 100644
index 0000000..162aa60
--- /dev/null
+++ b/net/netfilter/xt_condition.c
@@ -0,0 +1,294 @@
+/*
+ *	"condition" match extension for Xtables
+ *
+ *	Description: This module allows firewall rules to match using
+ *	condition variables available through procfs.
+ *
+ *	Authors:
+ *	Stephane Ouellette <ouellettes@videotron.ca>, 2002-10-22
+ *	Massimiliano Hofer <max@nucleus.it>, 2006-05-15
+ *
+ *	This program is free software; you can redistribute it and/or modify it
+ *	under the terms of the GNU General Public License; either version 2
+ *	or 3 of the License, as published by the Free Software Foundation.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/proc_fs.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/version.h>
+#include <linux/nsproxy.h>
+#include <linux/netfilter/x_tables.h>
+#include <linux/netfilter/xt_condition.h>
+#include <net/netns/generic.h>
+#include <asm/uaccess.h>
+
+MODULE_AUTHOR("Stephane Ouellette <ouellettes@videotron.ca>");
+MODULE_AUTHOR("Massimiliano Hofer <max@nucleus.it>");
+MODULE_AUTHOR("Jan Engelhardt <jengelh@medozas.de>");
+MODULE_DESCRIPTION("Allows rules to match against condition variables");
+MODULE_LICENSE("GPL");
+MODULE_PARM_DESC(condition_list_perms, "default permissions on /proc/net/nf_condition/* files");
+MODULE_PARM_DESC(condition_uid_perms, "default user owner of /proc/net/nf_condition/* files");
+MODULE_PARM_DESC(condition_gid_perms, "default group owner of /proc/net/nf_condition/* files");
+MODULE_ALIAS("ipt_condition");
+MODULE_ALIAS("ip6t_condition");
+
+struct condition_variable {
+	struct list_head list;
+	struct proc_dir_entry *status_proc;
+	unsigned int refcount;
+	bool enabled;
+};
+
+struct condition_net {
+	struct list_head list;
+	struct proc_dir_entry *proc_dir;
+	unsigned int list_perms;
+	unsigned int uid_perms;
+	unsigned int gid_perms;
+};
+
+static int condition_net_id;
+static inline struct condition_net *condition_pernet(struct net *net)
+{
+	return net_generic(net, condition_net_id);
+}
+
+/* proc_lock is a user context only semaphore used for write access */
+/*           to the conditions' list.                               */
+static DEFINE_MUTEX(proc_lock);
+
+static int condition_proc_read(char __user *buffer, char **start, off_t offset,
+			       int length, int *eof, void *data)
+{
+	const struct condition_variable *var = data;
+
+	buffer[0] = var->enabled ? '1' : '0';
+	buffer[1] = '\n';
+	if (length >= 2)
+		*eof = true;
+	return 2;
+}
+
+static int condition_proc_write(struct file *file, const char __user *buffer,
+				unsigned long length, void *data)
+{
+	struct condition_variable *var = data;
+	char newval;
+
+	if (length > 0) {
+		if (get_user(newval, buffer) != 0)
+			return -EFAULT;
+		/* Match only on the first character */
+		switch (newval) {
+		case '0':
+			var->enabled = false;
+			break;
+		case '1':
+			var->enabled = true;
+			break;
+		}
+	}
+	return length;
+}
+
+static bool
+condition_mt(const struct sk_buff *skb, struct xt_action_param *par)
+{
+	const struct xt_condition_mtinfo *info = par->matchinfo;
+	const struct condition_variable *var   = info->condvar;
+
+	return var->enabled ^ info->invert;
+}
+
+static int condition_mt_check(const struct xt_mtchk_param *par)
+{
+	struct xt_condition_mtinfo *info = par->matchinfo;
+	struct condition_variable *var;
+	struct condition_net *cond_net =
+		condition_pernet(current->nsproxy->net_ns);
+
+	/* Forbid certain names */
+	if (*info->name == '\0' || *info->name == '.' ||
+	    info->name[sizeof(info->name)-1] != '\0' ||
+	    memchr(info->name, '/', sizeof(info->name)) != NULL) {
+		pr_info("name not allowed or too long: \"%.*s\"\n",
+			(unsigned int)sizeof(info->name), info->name);
+		return -EINVAL;
+	}
+
+	/*
+	 * Let's acquire the lock, check for the condition and add it
+	 * or increase the reference counter.
+	 */
+	mutex_lock(&proc_lock);
+	list_for_each_entry(var, &cond_net->list, list) {
+		if (strcmp(info->name, var->status_proc->name) == 0) {
+			++var->refcount;
+			mutex_unlock(&proc_lock);
+			info->condvar = var;
+			return 0;
+		}
+	}
+
+	/* At this point, we need to allocate a new condition variable. */
+	var = kmalloc(sizeof(struct condition_variable), GFP_KERNEL);
+	if (var == NULL) {
+		mutex_unlock(&proc_lock);
+		return -ENOMEM;
+	}
+
+	/* Create the condition variable's proc file entry. */
+	var->status_proc = create_proc_entry(info->name,
+					     cond_net->list_perms,
+					     cond_net->proc_dir);
+	if (var->status_proc == NULL) {
+		kfree(var);
+		mutex_unlock(&proc_lock);
+		return -ENOMEM;
+	}
+
+	var->refcount = 1;
+	var->enabled  = false;
+	var->status_proc->data       = var;
+	var->status_proc->read_proc  = condition_proc_read;
+	var->status_proc->write_proc = condition_proc_write;
+	var->status_proc->uid        = cond_net->uid_perms;
+	var->status_proc->gid        = cond_net->gid_perms;
+	list_add(&var->list, &cond_net->list);
+	mutex_unlock(&proc_lock);
+	info->condvar = var;
+	return 0;
+}
+
+static void condition_mt_destroy(const struct xt_mtdtor_param *par)
+{
+	const struct xt_condition_mtinfo *info = par->matchinfo;
+	struct condition_variable *var = info->condvar;
+	struct condition_net *cond_net =
+		condition_pernet(current->nsproxy->net_ns);
+
+	mutex_lock(&proc_lock);
+	if (--var->refcount == 0) {
+		list_del(&var->list);
+		remove_proc_entry(var->status_proc->name,
+				  cond_net->proc_dir);
+		mutex_unlock(&proc_lock);
+		kfree(var);
+		return;
+	}
+	mutex_unlock(&proc_lock);
+}
+
+static struct xt_match condition_mt_reg __read_mostly = {
+	.name       = "condition",
+	.revision   = 1,
+	.family     = NFPROTO_UNSPEC,
+	.matchsize  = sizeof(struct xt_condition_mtinfo),
+	.match      = condition_mt,
+	.checkentry = condition_mt_check,
+	.destroy    = condition_mt_destroy,
+	.me         = THIS_MODULE,
+};
+
+static const char *const dir_name = "nf_condition";
+
+static int __net_init condnet_mt_init(struct net *net)
+{
+	struct condition_net *cond_net = condition_pernet(net);
+
+	INIT_LIST_HEAD(&cond_net->list);
+	cond_net->list_perms = S_IRUSR | S_IWUSR;
+	cond_net->uid_perms  = S_IRUSR | S_IWUSR;
+	cond_net->gid_perms  = S_IRUSR | S_IWUSR;
+
+	cond_net->proc_dir = proc_mkdir(dir_name, net->proc_net);
+
+	return (cond_net->proc_dir == NULL) ? -EACCES : 0;
+}
+
+static void __net_exit condnet_mt_exit(struct net *net)
+{
+	remove_proc_entry(dir_name, net->proc_net);
+}
+
+static struct pernet_operations condition_mt_netops = {
+	.init = condnet_mt_init,
+	.exit = condnet_mt_exit,
+	.id   = &condition_net_id,
+	.size = sizeof(struct condition_net),
+};
+
+static int __init condition_mt_init(void)
+{
+	int ret;
+
+	mutex_init(&proc_lock);
+	ret = xt_register_match(&condition_mt_reg);
+	if (ret < 0)
+		return ret;
+
+	ret = register_pernet_subsys(&condition_mt_netops);
+	if (ret < 0) {
+		xt_unregister_match(&condition_mt_reg);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void __exit condition_mt_exit(void)
+{
+	unregister_pernet_subsys(&condition_mt_netops);
+	xt_unregister_match(&condition_mt_reg);
+}
+
+int xt_condition_set_module_perms(const char *val, struct kernel_param *kp)
+{
+	unsigned long l;
+	int ret;
+	struct condition_net *cond_net =
+		condition_pernet(current->nsproxy->net_ns);
+
+	if (!val) return -EINVAL;
+	ret = strict_strtoul(val, 0, &l);
+	if (ret == -EINVAL || ((uint)l != l))
+		return -EINVAL;
+	*((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xt_condition_set_module_perms);
+
+int xt_condition_get_module_perms(char *buffer, struct kernel_param *kp)
+{
+	struct condition_net *cond_net =
+		condition_pernet(current->nsproxy->net_ns);
+
+	return sprintf(buffer, "%u",
+		       *((u32 *) ((u8 *) cond_net + (size_t) kp->arg)));
+}
+EXPORT_SYMBOL_GPL(xt_condition_get_module_perms);
+
+module_param_call(list_perms,
+		  xt_condition_set_module_perms,
+		  xt_condition_get_module_perms,
+		  (void *) offsetof(struct condition_net, list_perms),
+		  0600);
+module_param_call(uid_perms,
+		  xt_condition_set_module_perms,
+		  xt_condition_get_module_perms,
+		  (void *) offsetof(struct condition_net, uid_perms),
+		  0600);
+module_param_call(gid_perms,
+		  xt_condition_set_module_perms,
+		  xt_condition_get_module_perms,
+		  (void *) offsetof(struct condition_net, gid_perms),
+		  0600);
+
+module_init(condition_mt_init);
+module_exit(condition_mt_exit);
-- 
1.7.0.4

Re: [RFC 1/1] netfilter: xtables: inclusion of xt_condition

[Jan Engelhardt]

On Thursday 2010-07-22 16:09, Luciano Coelho wrote:
>+static int condition_mt_check(const struct xt_mtchk_param *par)
>+{
>+	struct xt_condition_mtinfo *info = par->matchinfo;
>+	struct condition_variable *var;
>+	struct condition_net *cond_net =
>+		condition_pernet(current->nsproxy->net_ns);

Cc'ing Alexey who has done the netns support.

Alexey, you added par->net, but given Luciano just did it with 
current->nsproxy->net_ns, do we really need par->net?


>+int xt_condition_set_module_perms(const char *val, struct kernel_param *kp)
>+{
>+	unsigned long l;
>+	int ret;
>+	struct condition_net *cond_net =
>+		condition_pernet(current->nsproxy->net_ns);
>+
>+	if (!val) return -EINVAL;

newline before return.

>+	ret = strict_strtoul(val, 0, &l);
>+	if (ret == -EINVAL || ((uint)l != l))
>+		return -EINVAL;

>+	*((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l;

I don't think we need this level of granularity; let the options be 
global, similar to what xt_hashlimit does.
(I am not even sure if kp->arg can be non-multiples-of-4, in which case 
this would be an alignment violation even.)

>+
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(xt_condition_set_module_perms);
>+
>+int xt_condition_get_module_perms(char *buffer, struct kernel_param *kp)
>+{
>+	struct condition_net *cond_net =
>+		condition_pernet(current->nsproxy->net_ns);
>+
>+	return sprintf(buffer, "%u",
>+		       *((u32 *) ((u8 *) cond_net + (size_t) kp->arg)));
>+}
>+EXPORT_SYMBOL_GPL(xt_condition_get_module_perms);
>+
>+module_param_call(list_perms,
>+		  xt_condition_set_module_perms,
>+		  xt_condition_get_module_perms,
>+		  (void *) offsetof(struct condition_net, list_perms),
>+		  0600);
>+module_param_call(uid_perms,
>+		  xt_condition_set_module_perms,
>+		  xt_condition_get_module_perms,
>+		  (void *) offsetof(struct condition_net, uid_perms),
>+		  0600);
>+module_param_call(gid_perms,
>+		  xt_condition_set_module_perms,
>+		  xt_condition_get_module_perms,
>+		  (void *) offsetof(struct condition_net, gid_perms),
>+		  0600);
>+
>+module_init(condition_mt_init);
>+module_exit(condition_mt_exit);
>-- 
>1.7.0.4
>

Re: [RFC 1/1] netfilter: xtables: inclusion of xt_condition

[Luciano Coelho]

On Thu, 2010-07-22 at 16:44 +0200, ext Jan Engelhardt wrote:
> On Thursday 2010-07-22 16:09, Luciano Coelho wrote:
> >+static int condition_mt_check(const struct xt_mtchk_param *par)
> >+{
> >+	struct xt_condition_mtinfo *info = par->matchinfo;
> >+	struct condition_variable *var;
> >+	struct condition_net *cond_net =
> >+		condition_pernet(current->nsproxy->net_ns);
> 
> Cc'ing Alexey who has done the netns support.
> 
> Alexey, you added par->net, but given Luciano just did it with 
> current->nsproxy->net_ns, do we really need par->net?
> 
> 
> >+int xt_condition_set_module_perms(const char *val, struct kernel_param *kp)
> >+{
> >+	unsigned long l;
> >+	int ret;
> >+	struct condition_net *cond_net =
> >+		condition_pernet(current->nsproxy->net_ns);
> >+
> >+	if (!val) return -EINVAL;
> 
> newline before return.

Sure! I copied this from params.c.  I'll fix it.


> >+	ret = strict_strtoul(val, 0, &l);
> >+	if (ret == -EINVAL || ((uint)l != l))
> >+		return -EINVAL;
> 
> >+	*((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l;
> 
> I don't think we need this level of granularity; let the options be 
> global, similar to what xt_hashlimit does.

I did this according to Patrick's comment:
> > proc_net_condition is a global variable, so this won't work for
> > namespaces. What the code does is reinitialize it when instantiating
> > a new namespace, so it will always point to the last instantiated
> > namespace.
> > 
> > The same problem exists for the condition_list, each namespace
> > should only be able to access its own conditions.
> 
> This also applies to the permission variables. Basically, we shouldn't
> be having any globals except perhaps the mutex. You probably need a
> module_param_call function to set them for the correct namespace (you
> can access that through current->nsproxy->net_ns).

I found it a bit strange to be able to change the module params in a
per-netns basis, but it is actually possible if you're changing the
parameters via sysfs.  I tried it and it even seems to work. ;)

I can't see any module parameters in the xt_hashlimit.c file.  Am I
looking in the wrong place?

I would be fine with making the module params global (as they were
before), if that's fine with Patrick too.


> (I am not even sure if kp->arg can be non-multiples-of-4, in which case 
> this would be an alignment violation even.)

I'm passing size_t in kp->arg.  It looks quite ugly, because usually
kp->arg is a pointer to some data.  But at least this way, using
offsetof(), I could avoid lots of repeated code for the options...


-- 
Cheers,
Luca.

Re: [RFC 1/1] netfilter: xtables: inclusion of xt_condition

[Jan Engelhardt]

On Thursday 2010-07-22 17:16, Luciano Coelho wrote:
>> >+	ret = strict_strtoul(val, 0, &l);
>> >+	if (ret == -EINVAL || ((uint)l != l))
>> >+		return -EINVAL;
>> 
>> >+	*((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l;
>> 
>> I don't think we need this level of granularity; let the options be 
>> global, similar to what xt_hashlimit does.
>
>I did this according to Patrick's comment:
>> > proc_net_condition is a global variable, so this won't work for
>> > namespaces. What the code does is reinitialize it when instantiating
>> > a new namespace, so it will always point to the last instantiated
>> > namespace.
>> > 
>> > The same problem exists for the condition_list, each namespace
>> > should only be able to access its own conditions.
>> 
>> This also applies to the permission variables. Basically, we shouldn't
>> be having any globals except perhaps the mutex. You probably need a
>> module_param_call function to set them for the correct namespace (you
>> can access that through current->nsproxy->net_ns).
>
>I found it a bit strange to be able to change the module params in a
>per-netns basis, but it is actually possible if you're changing the
>parameters via sysfs.  I tried it and it even seems to work. ;)
>
>I can't see any module parameters in the xt_hashlimit.c file.  Am I
>looking in the wrong place?

Oops, xt_recent.c.

>I would be fine with making the module params global (as they were
>before), if that's fine with Patrick too.

"When was the last time you needed to change the default ownership
when you _also_ have the possibility to chown each procfs file
individually?"

>> (I am not even sure if kp->arg can be non-multiples-of-4, in which case 
>> this would be an alignment violation even.)
>
>I'm passing size_t in kp->arg.  It looks quite ugly, because usually
>kp->arg is a pointer to some data.  But at least this way, using
>offsetof(), I could avoid lots of repeated code for the options...

if kp->arg is 1, ((u8*)cond_net + kp->arg) yields a pointer that's
usually not aligned for u32. (And C pedants would probably argue
that is should be char* not u8*, even if the one is a typedef
of another.)

Re: [RFC 1/1] netfilter: xtables: inclusion of xt_condition

[Luciano Coelho]

On Thu, 2010-07-22 at 17:36 +0200, ext Jan Engelhardt wrote:
> On Thursday 2010-07-22 17:16, Luciano Coelho wrote:
> >> >+	ret = strict_strtoul(val, 0, &l);
> >> >+	if (ret == -EINVAL || ((uint)l != l))
> >> >+		return -EINVAL;
> >> 
> >> >+	*((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l;
> >> 
> >> I don't think we need this level of granularity; let the options be 
> >> global, similar to what xt_hashlimit does.
> >
> >I did this according to Patrick's comment:
> >> > proc_net_condition is a global variable, so this won't work for
> >> > namespaces. What the code does is reinitialize it when instantiating
> >> > a new namespace, so it will always point to the last instantiated
> >> > namespace.
> >> > 
> >> > The same problem exists for the condition_list, each namespace
> >> > should only be able to access its own conditions.
> >> 
> >> This also applies to the permission variables. Basically, we shouldn't
> >> be having any globals except perhaps the mutex. You probably need a
> >> module_param_call function to set them for the correct namespace (you
> >> can access that through current->nsproxy->net_ns).
> >
> >I found it a bit strange to be able to change the module params in a
> >per-netns basis, but it is actually possible if you're changing the
> >parameters via sysfs.  I tried it and it even seems to work. ;)
> >
> >I can't see any module parameters in the xt_hashlimit.c file.  Am I
> >looking in the wrong place?
> 
> Oops, xt_recent.c.
> 
> >I would be fine with making the module params global (as they were
> >before), if that's fine with Patrick too.
> 
> "When was the last time you needed to change the default ownership
> when you _also_ have the possibility to chown each procfs file
> individually?"
> 
> >> (I am not even sure if kp->arg can be non-multiples-of-4, in which case 
> >> this would be an alignment violation even.)
> >
> >I'm passing size_t in kp->arg.  It looks quite ugly, because usually
> >kp->arg is a pointer to some data.  But at least this way, using
> >offsetof(), I could avoid lots of repeated code for the options...
> 
> if kp->arg is 1, ((u8*)cond_net + kp->arg) yields a pointer that's
> usually not aligned for u32. (And C pedants would probably argue
> that is should be char* not u8*, even if the one is a typedef
> of another.)

Oh, I see.  In the current case it works because all the parameters are
32-bit.

In any case, I'll just remove the per-net module parameters code.  It is
indeed over-fine-grained as you said earlier.


-- 
Cheers,
Luca.

Re: [RFC 1/1] netfilter: xtables: inclusion of xt_condition

[Alexey Dobriyan]

On Thu, Jul 22, 2010 at 04:44:35PM +0200, Jan Engelhardt wrote:
> 
> On Thursday 2010-07-22 16:09, Luciano Coelho wrote:
> >+static int condition_mt_check(const struct xt_mtchk_param *par)
> >+{
> >+	struct xt_condition_mtinfo *info = par->matchinfo;
> >+	struct condition_variable *var;
> >+	struct condition_net *cond_net =
> >+		condition_pernet(current->nsproxy->net_ns);
> 
> Cc'ing Alexey who has done the netns support.
> 
> Alexey, you added par->net, but given Luciano just did it with 
> current->nsproxy->net_ns, do we really need par->net?

In ->check, maybe, we can get away with current->nsproxy->net_ns.

But definitely not in ->destroy(), because destruction can happen
when _no_ task is in netns, so current->nsproxy->net_ns is 100% bogus.

Steps to reproduce:
	iptables -A ...
	exit

->destroy hook gets netns from par->net, ->checkentry does the same
for symmetry and less confusion.

Re: [RFC 1/1] netfilter: xtables: inclusion of xt_condition

[Luciano Coelho]

On Thu, 2010-07-22 at 21:19 +0200, ext Alexey Dobriyan wrote:
> On Thu, Jul 22, 2010 at 04:44:35PM +0200, Jan Engelhardt wrote:
> > 
> > On Thursday 2010-07-22 16:09, Luciano Coelho wrote:
> > >+static int condition_mt_check(const struct xt_mtchk_param *par)
> > >+{
> > >+	struct xt_condition_mtinfo *info = par->matchinfo;
> > >+	struct condition_variable *var;
> > >+	struct condition_net *cond_net =
> > >+		condition_pernet(current->nsproxy->net_ns);
> > 
> > Cc'ing Alexey who has done the netns support.
> > 
> > Alexey, you added par->net, but given Luciano just did it with 
> > current->nsproxy->net_ns, do we really need par->net?
> 
> In ->check, maybe, we can get away with current->nsproxy->net_ns.
> 
> But definitely not in ->destroy(), because destruction can happen
> when _no_ task is in netns, so current->nsproxy->net_ns is 100% bogus.
> 
> Steps to reproduce:
> 	iptables -A ...
> 	exit
> 
> ->destroy hook gets netns from par->net, ->checkentry does the same
> for symmetry and less confusion.

Very good point.  I guess that when Patrick suggested using
current->nsproxy->net_ns, he meant only for the module_params part.
I'll be removing that anyway.  And I'll change the code to use par->net
instead of current->nsproxy->net_ns to avoid the problem in _destroy.

Thanks for your comments!

I must admit that I was a bit insecure about this code.  That's why I
sent a RFC early enough. ;)


-- 
Cheers,
Luca.