condition for 2.6.16

condition for 2.6.16

[Massimiliano Hofer]

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

Hi,
I wrote here some time ago about condition and Krzysztof told me it wasn't 
supposed to work.
I tested it and it seemed to work, but reading the code I agree that it 
shouldn't have. :)

I find this extension very useful, so I did a complete porting to 2.6.16 
complete with XT. I fixed various things (mostly fossils of the 2.4 era) in 
the process and it works in my test environment.
I am about to test it in a few high load machines, but I'd appreciate if 
someone else could have a look at it.

What are the next steps in order to have it included in the core extension and 
finally merged in the kernel?

-- 
Saluti,
   Massimiliano Hofer
        Nucleus

[-- Attachment #2: condition.patch --]
[-- Type: text/x-diff, Size: 13594 bytes --]

diff -Nru patch-o-matic-ng-20060419/patchlets/condition/help patch-o-matic-ng-20060419.new/patchlets/condition/help
--- patch-o-matic-ng-20060419/patchlets/condition/help	2003-12-20 17:19:17.000000000 +0100
+++ patch-o-matic-ng-20060419.new/patchlets/condition/help	2006-04-20 18:36:22.000000000 +0200
@@ -7,8 +7,10 @@
 iptables -A INPUT -p tcp -m condition --condition web_ok --dport 80 -j ACCEPT
 
 To allow this rule to match:
-echo 1 > /proc/net/ipt_condition/web_ok
+echo 1 > /proc/net/nf_condition/web_ok
 
 To disable this rule: 
-echo 0 > /proc/net/ipt_condition/web_ok
+echo 0 > /proc/net/nf_condition/web_ok
+
+NB: it was /proc/net/ipt_condition on 2.4.
 
diff -Nru patch-o-matic-ng-20060419/patchlets/condition/info patch-o-matic-ng-20060419.new/patchlets/condition/info
--- patch-o-matic-ng-20060419/patchlets/condition/info	2004-02-20 00:25:33.000000000 +0100
+++ patch-o-matic-ng-20060419.new/patchlets/condition/info	2006-04-20 18:52:56.000000000 +0200
@@ -1,4 +1,4 @@
-Author: Stephane Ouellette <ouellettes@videotron.ca>
+Author: Stephane Ouellette <ouellettes@videotron.ca> and Massimiliano Hofer <max@nucleus.it>
 Status: ItWorksForMe(tm)
+Requires: linux == 2.4 || linux >= 2.6.16
 Repository: extra
-Requires: linux < 2.6.0
diff -Nru patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/Documentation/Configure.help.ladd patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/Documentation/Configure.help.ladd
--- patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/Documentation/Configure.help.ladd	1970-01-01 01:00:00.000000000 +0100
+++ patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/Documentation/Configure.help.ladd	2006-04-20 17:24:15.000000000 +0200
@@ -0,0 +1,6 @@
+NETFILTER_XT_MATCH_CONDITION
+  This option allows you to match firewall rules against condition
+  variables stored in the /proc/net/nf_condition directory.
+
+  If you want to compile it as a module, say M here and read
+  Documentation/modules.txt.  If unsure, say `N'.
diff -Nru patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/include/linux/netfilter/xt_condition.h patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/include/linux/netfilter/xt_condition.h
--- patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/include/linux/netfilter/xt_condition.h	1970-01-01 01:00:00.000000000 +0100
+++ patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/include/linux/netfilter/xt_condition.h	2006-04-20 17:18:11.000000000 +0200
@@ -0,0 +1,11 @@
+#ifndef _XT_CONDITION_H
+#define _XT_CONDITION_H
+
+#define CONDITION_NAME_LEN  32
+
+struct condition_info {
+	char name[CONDITION_NAME_LEN];
+	int  invert;
+};
+
+#endif /* _XT_CONDITION_H */
diff -Nru patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/net/netfilter/Kconfig.ladd patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/net/netfilter/Kconfig.ladd
--- patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/net/netfilter/Kconfig.ladd	1970-01-01 01:00:00.000000000 +0100
+++ patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/net/netfilter/Kconfig.ladd	2006-04-20 17:22:13.000000000 +0200
@@ -0,0 +1,9 @@
+config NETFILTER_XT_MATCH_CONDITION
+        tristate  '"condition" match support'
+        depends on NETFILTER_XTABLES
+        help
+          This option allows you to match firewall rules against condition
+          variables stored in the /proc/net/ipt_condition directory.
+
+          If you want to compile it as a module, say M here and read
+          Documentation/modules.txt.  If unsure, say `N'.
diff -Nru patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/net/netfilter/Makefile.ladd patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/net/netfilter/Makefile.ladd
--- patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/net/netfilter/Makefile.ladd	1970-01-01 01:00:00.000000000 +0100
+++ patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/net/netfilter/Makefile.ladd	2006-04-20 18:34:08.000000000 +0200
@@ -0,0 +1,2 @@
+obj-$(CONFIG_NETFILTER_XT_MATCH_PHYSDEV) += xt_physdev.o
+obj-$(CONFIG_NETFILTER_XT_MATCH_LENGTH) += xt_condition.o
diff -Nru patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/net/netfilter/xt_condition.c patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/net/netfilter/xt_condition.c
--- patch-o-matic-ng-20060419/patchlets/condition/linux-2.6.16/net/netfilter/xt_condition.c	1970-01-01 01:00:00.000000000 +0100
+++ patch-o-matic-ng-20060419.new/patchlets/condition/linux-2.6.16/net/netfilter/xt_condition.c	2006-04-20 17:21:07.000000000 +0200
@@ -0,0 +1,318 @@
+/*-------------------------------------------*\
+|          Netfilter Condition Module         |
+|                                             |
+|  Description: This module allows firewall   |
+|    rules to match using condition variables |
+|    stored in /proc files.                   |
+|                                             |
+|  Author: Stephane Ouellette     2002-10-22  |
+|          <ouellettes@videotron.ca>          |
+|          Massimiliano Hofer     2006-05-15  |
+|          <max@nucleus.it>                   |
+|                                             |
+|  History:                                   |
+|    2003-02-10  Second version with improved |
+|                locking and simplified code. |
+|    2006-05-15  2.6.16 adaptations.          |
+|                Locking overhaul.            |
+|                Various bug fixes.           |
+|                                             |
+|  This software is distributed under the     |
+|  terms of the GNU GPL.                      |
+\*-------------------------------------------*/
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/proc_fs.h>
+#include <linux/spinlock.h>
+#include <asm/semaphore.h>
+#include <linux/string.h>
+#include <asm/atomic.h>
+#include <asm/uaccess.h>
+#include <linux/netfilter/x_tables.h>
+#include <linux/netfilter/xt_condition.h>
+
+#ifndef CONFIG_PROC_FS
+#error  "Proc file system support is required for this module"
+#endif
+
+/* Defaults, these can be overridden on the module command-line. */
+static unsigned int condition_list_perms = 0644;
+
+MODULE_AUTHOR("Stephane Ouellette <ouellettes@videotron.ca> and Massimiliano Hofer <max@nucleus.it>");
+MODULE_DESCRIPTION("Allows rules to match against condition variables");
+MODULE_LICENSE("GPL");
+module_param(condition_list_perms, uint, 0400);
+MODULE_PARM_DESC(condition_list_perms,"permissions on /proc/net/nf_condition/* files");
+MODULE_ALIAS("ipt_condition");
+MODULE_ALIAS("ip6t_condition");
+
+struct condition_variable {
+	struct condition_variable *next;
+	struct proc_dir_entry *status_proc;
+	atomic_t refcount;
+        int enabled;   /* TRUE == 1, FALSE == 0 */
+};
+
+/* list_lock is a spin lock used to synchronize user and bottom half contexts.  */
+/*    It is used only for short period requiring reads from BH or               */
+/*    read/write from user.                                                     */
+/* proc_lock is a user context only semaphor used for more extensive operations */
+/*    during insertion and deletion of rules. It must never be acquired by      */
+/*    by someone already having a list_lock, but you can obtain a list_lock     */
+/*    while having a proc_lock.                                                 */
+static DEFINE_SPINLOCK(list_lock);
+static DECLARE_MUTEX(proc_lock);
+
+static struct condition_variable *head = NULL;
+static struct proc_dir_entry *proc_net_condition = NULL;
+
+
+static int
+xt_condition_read_info(char __user *buffer, char **start, off_t offset,
+			int length, int *eof, void *data)
+{
+	struct condition_variable *var =
+	    (struct condition_variable *) data;
+
+	buffer[0] = (var->enabled) ? '1' : '0';
+	buffer[1] = '\n';
+	if (length>=2)
+		*eof = 1;
+
+	return 2;
+}
+
+
+static int
+xt_condition_write_info(struct file *file, const char __user *buffer,
+			 unsigned long length, void *data)
+{
+	struct condition_variable *var =
+	    (struct condition_variable *) data;
+	char newval;
+
+	if (length>0) {
+		if (get_user(newval, buffer))
+			return -EFAULT;
+	        /* Match only on the first character */
+		switch (newval) {
+		case '0':
+			var->enabled = 0;
+			break;
+		case '1':
+			var->enabled = 1;
+			break;
+		}
+	}
+
+	return (int) length;
+}
+
+
+static int
+match(const struct sk_buff *skb, const struct net_device *in,
+      const struct net_device *out, const void *matchinfo, int offset,
+      unsigned int protoff, int *hotdrop)
+{
+	const struct condition_info *info =
+	    (const struct condition_info *) matchinfo;
+	struct condition_variable *var;
+	int condition_status = 0;
+
+	spin_lock_bh(&list_lock);
+
+	for (var = head; var != NULL; var = var->next) {
+		if (strcmp(info->name, var->status_proc->name) == 0) {
+			condition_status = var->enabled;
+			break;
+		}
+	}
+
+	spin_unlock_bh(&list_lock);
+
+	return condition_status ^ info->invert;
+}
+
+
+
+static int
+checkentry(const char *tablename, const void *ip,
+	   void *matchinfo, unsigned int matchsize, unsigned int hook_mask)
+{
+	static const char * const forbidden_names[]={ "", ".", ".." };
+	struct condition_info *info = (struct condition_info *) matchinfo;
+	struct condition_variable *var, *newvar;
+
+	int i;
+
+	if (matchsize != XT_ALIGN(sizeof(struct condition_info)))
+		return 0;
+
+	/* We don't want a '/' in a proc file name. */
+	for(i=0; i < CONDITION_NAME_LEN && info->name[i] != '\0'; i++)
+		if(info->name[i] == '/')
+			return 0;
+	/* We can't handle file names longer than CONDITION_NAME_LEN and */
+	/* we want a NULL terminated string. */
+	if(i == CONDITION_NAME_LEN)
+		return 0;
+
+	/* We don't want certain reserved names. */
+	for(i=0; i < sizeof(forbidden_names)/sizeof(char *); i++)
+		if(strcmp(info->name, forbidden_names[i])==0)
+			return 0;
+
+	/* The first step is to check if the condition variable already exists.  */
+	/* This lock is to prevent other processes performing the same check and */
+	/* modify code and from altering proc entries.                           */
+	down(&proc_lock);
+
+	for (var = head; var; var = var->next) {
+		if (strcmp(info->name, var->status_proc->name) == 0) {
+			atomic_inc(&var->refcount);
+			up(&proc_lock);
+			return 1;
+		}
+	}
+
+	/* At this point, we need to allocate a new condition variable */
+	newvar = kmalloc(sizeof(struct condition_variable), GFP_KERNEL);
+
+	if (!newvar) {
+		up(&proc_lock);
+		return -ENOMEM;
+	}
+
+	/* Create the condition variable's proc file entry */
+	newvar->status_proc = create_proc_entry(info->name, condition_list_perms, proc_net_condition);
+
+	if (!newvar->status_proc) {
+	  /*
+	   * There are two possibilities:
+	   *  1- Another condition variable with the same name has been created, which is valid.
+	   *  2- There was a memory allocation error.
+	   */
+		kfree(newvar);
+
+		for (var = head; var; var = var->next) {
+			if (strcmp(info->name, var->status_proc->name) == 0) {
+				atomic_inc(&var->refcount);
+				up(&proc_lock);
+				return 1;
+			}
+		}
+
+		up(&proc_lock);
+		return -ENOMEM;
+	}
+
+	atomic_set(&newvar->refcount, 1);
+	newvar->enabled = 0;
+	newvar->status_proc->owner = THIS_MODULE;
+	newvar->status_proc->data = newvar;
+	wmb();
+	newvar->status_proc->read_proc = xt_condition_read_info;
+	newvar->status_proc->write_proc = xt_condition_write_info;
+
+	/* There used to be a lock here, but a memory barrier should be good enough. */
+	newvar->next = head;
+	wmb();
+	head = newvar;
+
+	up(&proc_lock);
+
+	return 1;
+}
+
+
+static void
+destroy(void *matchinfo, unsigned int matchsize)
+{
+	struct condition_info *info = (struct condition_info *) matchinfo;
+	struct condition_variable *var, *prev;
+
+	if (matchsize != XT_ALIGN(sizeof(struct condition_info)))
+		return;
+
+	down(&proc_lock);
+
+	for (prev = NULL, var = head; var && strcmp(info->name, var->status_proc->name);
+	     prev = var, var = var->next);
+
+	if (var && atomic_dec_and_test(&var->refcount)) {
+		/* This lock is to prevent clashing with netfilter matching. */
+		spin_lock_bh(&list_lock);
+		if (prev)
+			prev->next = var->next;
+		else
+			head = var->next;
+		spin_unlock_bh(&list_lock);
+
+		remove_proc_entry(var->status_proc->name, proc_net_condition);
+		kfree(var);
+	}
+
+	up(&proc_lock);
+}
+
+
+static struct xt_match condition_match = {
+	.name = "condition",
+	.match = &match,
+	.checkentry = &checkentry,
+	.destroy = &destroy,
+	.me = THIS_MODULE
+};
+
+static struct xt_match condition6_match = {
+	.name = "condition",
+	.match = &match,
+	.checkentry = &checkentry,
+	.destroy = &destroy,
+	.me = THIS_MODULE
+};
+
+static int __init
+init(void)
+{
+	int errorcode;
+
+	spin_lock_init(&list_lock);
+
+	proc_net_condition = proc_mkdir("nf_condition", proc_net);
+	if (!proc_net_condition) {
+		errorcode = -EACCES;
+		goto remove_proc;
+	}
+
+        errorcode = xt_register_match(AF_INET, &condition_match);
+	if (errorcode)
+		goto remove_inet;
+
+	errorcode = xt_register_match(AF_INET6, &condition6_match);
+	if (errorcode)
+		goto remove_inet6;
+	return 0;
+
+remove_inet6:
+		xt_unregister_match(AF_INET6, &condition6_match);
+remove_inet:
+		xt_unregister_match(AF_INET, &condition_match);
+remove_proc:
+		remove_proc_entry("nf_condition", proc_net);
+
+	return errorcode;
+}
+
+
+static void __exit
+fini(void)
+{
+	xt_unregister_match(AF_INET6, &condition6_match);
+	xt_unregister_match(AF_INET, &condition_match);
+	remove_proc_entry("nf_condition", proc_net);
+}
+
+module_init(init);
+module_exit(fini);

Re: condition for 2.6.16

[Patrick McHardy]

Massimiliano Hofer wrote:
> I find this extension very useful, so I did a complete porting to 2.6.16 
> complete with XT. I fixed various things (mostly fossils of the 2.4 era) in 
> the process and it works in my test environment.
> I am about to test it in a few high load machines, but I'd appreciate if 
> someone else could have a look at it.
> 
> What are the next steps in order to have it included in the core extension and 
> finally merged in the kernel?

We have already decided that the condition match will not be merged
because the same thing can easily be done by adding/removing rules
from userspace.

Re: condition for 2.6.16

[Massimiliano Hofer]

On Thursday 20 April 2006 8:49 pm, Patrick McHardy wrote:
> We have already decided that the condition match will not be merged
> because the same thing can easily be done by adding/removing rules
> from userspace.

Sorry, I missed that discussion.
Is it still possible to include it in the external patches?

Anyway I think condition allows for a much more elegant solution in cases 
where adding and removing rules would lead to a messy and unpredictable 
sequence of overlapping rules, not to mention races between different 
modifications.
Just my humble opinion.

-- 
Saluti,
   Massimiliano Hofer
        Nucleus

Re: condition for 2.6.16

[Martijn Lievaart]

Massimiliano Hofer wrote:

>On Thursday 20 April 2006 8:49 pm, Patrick McHardy wrote:
>  
>
>>We have already decided that the condition match will not be merged
>>because the same thing can easily be done by adding/removing rules
>>from userspace.
>>    
>>
>
>Sorry, I missed that discussion.
>Is it still possible to include it in the external patches?
>
>Anyway I think condition allows for a much more elegant solution in cases 
>where adding and removing rules would lead to a messy and unpredictable 
>sequence of overlapping rules, not to mention races between different 
>modifications.
>Just my humble opinion.
>  
>

I concur. The condition match is much more elegant then dynamically 
messing with the rules.

M4

Re: condition for 2.6.16

[Patrick McHardy]

Massimiliano Hofer wrote:
> On Thursday 20 April 2006 8:49 pm, Patrick McHardy wrote:
> 
>>We have already decided that the condition match will not be merged
>>because the same thing can easily be done by adding/removing rules
>>from userspace.
> 
> 
> Sorry, I missed that discussion.

It was discussed at the netfilter workshops, summaries are available at
workshop.netfilter.org.

> Is it still possible to include it in the external patches?

You can set up a pomng repository and send us the URL to include in
the sources.list file once we finish the pomng reorganization.

> Anyway I think condition allows for a much more elegant solution in cases 
> where adding and removing rules would lead to a messy and unpredictable 
> sequence of overlapping rules, not to mention races between different 
> modifications.
> Just my humble opinion.


Thats true, mainly because were missing a better kernel-userspace
interface and better userspace tools. This is also the reason
why we don't want to include it, its just a workaround for these
problems.

Re: condition for 2.6.16

[Massimiliano Hofer]

On Friday 21 April 2006 12:47 am, Patrick McHardy wrote:

> It was discussed at the netfilter workshops, summaries are available at
> workshop.netfilter.org.

I did a cursory check of the site, but I only found a 5 or 6 lines summary for 
every conference. Google doesn't help either.
May you direct me to a more comprehensive report, please?

> You can set up a pomng repository and send us the URL to include in
> the sources.list file once we finish the pomng reorganization.

I'll do it as soon as possible.

> Thats true, mainly because were missing a better kernel-userspace
> interface and better userspace tools. This is also the reason
> why we don't want to include it, its just a workaround for these
> problems.

Are you talking about a grand plan for radically innovative user tools?
Is there anything ready for testing or is it just in the design phase?

Anyway, for the kind of use I have in mind, some global state is needed. Part 
of that state is in the kernel by definition (that's were the rules are), so 
we can enrich that state (condition or something better) or we can maintain a 
second state in userspace and deal with the interactions.

You could say that registering userspace variables is not kernel business, but 
the alternative is just messy (with the tools available in the near future).

If your aim is a minimal set of targets and matches, you could slash many 
other extensions. ROUTE isn't necessary when you can use MARK and iproute2,
but I think it is really convenient. In fact I use ROUTE and condition 
together in some installation with failover connections.

I think I'm missing your goal.

-- 
Saluti,
   Massimiliano Hofer
        Nucleus

Re: condition for 2.6.16

[Gerd v. Egidy]

On Friday 21 April 2006 01:26, Massimiliano Hofer wrote:
> On Friday 21 April 2006 12:47 am, Patrick McHardy wrote:
> > It was discussed at the netfilter workshops, summaries are available at
> > workshop.netfilter.org.
>
> I did a cursory check of the site, but I only found a 5 or 6 lines summary
> for every conference. Google doesn't help either.
> May you direct me to a more comprehensive report, please?

There is no big discussion about condition online but one line at 
http://workshop.netfilter.org/2004/ :

6.2. Decisions about individual patches
...
condition  Stays in POM, because it's ugly and ruleset updates are faster 
these days 

I don't know more than that because I didn't attend that netfilter workshop.

I already had a discussion about the future of condition with Harald back in 
2004 and he basically said the same as Patrick now:

http://thread.gmane.org/gmane.comp.security.firewalls.netfilter.devel/5694

I think something like condition is needed as temporary solution until the 
proposed fast and reliable way to change rules is available. I very much 
appreciate your work on porting it to 2.6, I already had planned doing that 
in the nex two or three month.

Kind regards,

Gerd

Re: condition for 2.6.16

[Gerd v. Egidy]

On Friday 21 April 2006 01:26, Massimiliano Hofer wrote:
> On Friday 21 April 2006 12:47 am, Patrick McHardy wrote:
> > It was discussed at the netfilter workshops, summaries are available at
> > workshop.netfilter.org.
>
> I did a cursory check of the site, but I only found a 5 or 6 lines summary
> for every conference. Google doesn't help either.
> May you direct me to a more comprehensive report, please?

There is no big discussion about condition online but one line at 
http://workshop.netfilter.org/2004/ :

6.2. Decisions about individual patches
...
condition  Stays in POM, because it's ugly and ruleset updates are faster 
these days 

I don't know more than that because I didn't attend that netfilter workshop.

I already had a discussion about the future of condition with Harald back in 
2004 and he basically said the same as Patrick now:

http://thread.gmane.org/gmane.comp.security.firewalls.netfilter.devel/5694

I think something like condition is needed as temporary solution until the 
proposed fast and reliable way to change rules is available. I very much 
appreciate your work on porting it to 2.6, I already had planned doing that 
in the nex two or three month.

Kind regards,

Gerd

Re: condition for 2.6.16

[Amin Azez]

Gerd v. Egidy wrote:
> On Friday 21 April 2006 01:26, Massimiliano Hofer wrote:
> 
>>On Friday 21 April 2006 12:47 am, Patrick McHardy wrote:
>>
>>>It was discussed at the netfilter workshops, summaries are available at
>>>workshop.netfilter.org.
>>
>>I did a cursory check of the site, but I only found a 5 or 6 lines summary
>>for every conference. Google doesn't help either.
>>May you direct me to a more comprehensive report, please?
> 
> 
> There is no big discussion about condition online but one line at 
> http://workshop.netfilter.org/2004/ :
> 
> 6.2. Decisions about individual patches
> ...
> condition  Stays in POM, because it's ugly and ruleset updates are faster 
> these days 

I like it's "ugliness"

> I don't know more than that because I didn't attend that netfilter workshop.
> 
> I already had a discussion about the future of condition with Harald back in 
> 2004 and he basically said the same as Patrick now:
> 
> http://thread.gmane.org/gmane.comp.security.firewalls.netfilter.devel/5694
> 
> I think something like condition is needed as temporary solution until the 
> proposed fast and reliable way to change rules is available. I very much 
> appreciate your work on porting it to 2.6, I already had planned doing that 
> in the nex two or three month.

ipt_condition allows the abstraction of enabling/disabling features of 
an iptables ruleset.

If you remove a rule, you have destroyed the information as to where in 
the ruleset it belonged. Removing a rule is conceptually different from 
disabling it.

One exaple is use of ipt_redirect to a trasparent web proxy, the rule 
exists among a sequence of rules as not all users should have their 
traffic via the proxy depnding on source-ip/target-ip combinations as 
well as marks (from other rules) and then ipt_condition to enable or 
disable the interception.

To remove the intercept rule is to lose the prioritisation information.
It could be implemented via sub-chains, which is to my mind more messy.

I will say that ipt_condition will not go away. The code is simple 
enough, the only objection I find is that one if its uses is to overcome 
a deficiency in iptables management tools. It may stay in pom, but it 
provides too useful an interface to go away.

Sam

Re: condition for 2.6.16

[Simon Lodal]

On Thursday 20 April 2006 20:49, Patrick McHardy wrote:

> We have already decided that the condition match will not be merged
> because the same thing can easily be done by adding/removing rules
> from userspace.

Conditions enable role separation between us admins.

No matter how good or fast the userspace tools are, there are cases where you 
simply do not want to (let others) run them, but it is acceptable to (let 
others) turn on/off some predefined blocks of rules.

Plus it is faster, less risky, and does not reset counters.


Simon

Re: condition for 2.6.16

[Patrick McHardy]

Simon Lodal wrote:
> On Thursday 20 April 2006 20:49, Patrick McHardy wrote:
> 
> 
>>We have already decided that the condition match will not be merged
>>because the same thing can easily be done by adding/removing rules
>>from userspace.
> 
> 
> Conditions enable role separation between us admins.
> 
> No matter how good or fast the userspace tools are, there are cases where you 
> simply do not want to (let others) run them, but it is acceptable to (let 
> others) turn on/off some predefined blocks of rules.
> 
> Plus it is faster, less risky, and does not reset counters.

I'm not really buying that argument, this can all also be done in
userspace. But a lot of people seem to consider it useful, so I might
reconsider if someone cleans it up so it at least doesn't need to walk
the list of conditions for every packet it matches .. but no promises.

Re: condition for 2.6.16

[Massimiliano Hofer]

On Friday 28 April 2006 9:12 am, Patrick McHardy wrote:

> I'm not really buying that argument, this can all also be done in
> userspace. But a lot of people seem to consider it useful, so I might
> reconsider if someone cleans it up so it at least doesn't need to walk
> the list of conditions for every packet it matches .. but no promises.

I'll set to work on it. I'll need to change the userspace interface, though.
The only O(1) way to do it is to store a pointer (or any other id) in the rule 
itself. I didn't do it in the previous version because I though this was 
really ugly. I can't find any other match doing a similar thing. Anyway I can 
do it.

On the other hand I can make it a guaranteed O(log n) or average O(1) without 
meddling the rule descriptor and with compatible userspace. What do you 
prefer?

-- 
Saluti,
   Massimiliano Hofer
        Nucleus

Re: condition for 2.6.16

[Patrick McHardy]

Massimiliano Hofer wrote:
> On Friday 28 April 2006 9:12 am, Patrick McHardy wrote:
> 
> 
>>I'm not really buying that argument, this can all also be done in
>>userspace. But a lot of people seem to consider it useful, so I might
>>reconsider if someone cleans it up so it at least doesn't need to walk
>>the list of conditions for every packet it matches .. but no promises.
> 
> 
> I'll set to work on it. I'll need to change the userspace interface, though.
> The only O(1) way to do it is to store a pointer (or any other id) in the rule 
> itself. I didn't do it in the previous version because I though this was 
> really ugly. I can't find any other match doing a similar thing. Anyway I can 
> do it.

Unfortunately its ugly, but this is a well-known limitation of iptables
itself. Since its the only way to do certain things, I won't complain
if this part is ugly :)

> On the other hand I can make it a guaranteed O(log n) or average O(1) without 
> meddling the rule descriptor and with compatible userspace. What do you 
> prefer?

How would you achieve O(1) average?

Re: condition for 2.6.16

[Massimiliano Hofer]

On Friday 28 April 2006 1:06 pm, Patrick McHardy wrote:

> > I'll set to work on it. I'll need to change the userspace interface,
> > though. The only O(1) way to do it is to store a pointer (or any other
> > id) in the rule itself. I didn't do it in the previous version because I
> > though this was really ugly. I can't find any other match doing a similar
> > thing. Anyway I can do it.
>
> Unfortunately its ugly, but this is a well-known limitation of iptables
> itself. Since its the only way to do certain things, I won't complain
> if this part is ugly :)

OK. This time I warned you. :)

> > On the other hand I can make it a guaranteed O(log n) or average O(1)
> > without meddling the rule descriptor and with compatible userspace. What
> > do you prefer?
>
> How would you achieve O(1) average?

Hash. But it adds complexity to the code and unnecessary complexity is a form 
of ugliness.

While we're talking about varying degrees of ugliness, how bad would it be if 
I optionally allowed to keep a persistent state across rule removal and 
reinsertion (for example whene someone flushes the table and restarts the 
firewalling script)?
I concede that this would really be easy to do in userspace, so maybe I'm 
answering myself. :)

-- 
Saluti,
   Massimiliano Hofer
        Nucleus

Re: condition for 2.6.16

[Jozsef Kadlecsik]

On Fri, 28 Apr 2006, Massimiliano Hofer wrote:

> > How would you achieve O(1) average?
>
> Hash. But it adds complexity to the code and unnecessary complexity is a form
> of ugliness.

Why don't you choose an array and its indices? Array size could be
specified by module parameter if the default were not sufficient for
someone.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: condition for 2.6.16

[Patrick McHardy]

Jozsef Kadlecsik wrote:
> On Fri, 28 Apr 2006, Massimiliano Hofer wrote:
> 
> 
>>>How would you achieve O(1) average?
>>
>>Hash. But it adds complexity to the code and unnecessary complexity is a form
>>of ugliness.
> 
> 
> Why don't you choose an array and its indices? Array size could be
> specified by module parameter if the default were not sufficient for
> someone.

I just removed something similar from the nth match :) It used a
hard-coded size, but either way I don't like introducing this
limitation just to work around the small uglyness required to
keep a pointer inside the per-instance match data. We already
have multiple precedents for this.

Re: condition for 2.6.16

[KOVACS Krisztian]

  Hi,

On Friday 28 April 2006 15.07, Patrick McHardy wrote:
> hard-coded size, but either way I don't like introducing this
> limitation just to work around the small uglyness required to
> keep a pointer inside the per-instance match data. We already
> have multiple precedents for this.

  Indeed, there are multiple precedents for this. For example: CLUSTERIP 
(already in mainline) is by far the ugliest hack I've seen in any Netfilter 
code. :)

-- 
 Regards,
  Krisztian Kovacs

Re: condition for 2.6.16

[Patrick McHardy]

KOVACS Krisztian wrote:
>   Hi,
> 
> On Friday 28 April 2006 15.07, Patrick McHardy wrote:
> 
>>hard-coded size, but either way I don't like introducing this
>>limitation just to work around the small uglyness required to
>>keep a pointer inside the per-instance match data. We already
>>have multiple precedents for this.
> 
> 
>   Indeed, there are multiple precedents for this. For example: CLUSTERIP 
> (already in mainline) is by far the ugliest hack I've seen in any Netfilter 
> code. :)

Hehe :) Yes, its not nice, but I refuse to add stupid limits just
because the infrastructure can't cleanly support per-instance state
(which is what makes it ugly, its not the shared state). And its not
so bad, the uglyness comes down to two or three extra lines of code,
one additional pointer in the data structure and one offsetof in
userspace.

Re: condition for 2.6.16

[Massimiliano Hofer]

On Friday 28 April 2006 5:34 pm, Patrick McHardy wrote:

> Hehe :) Yes, its not nice, but I refuse to add stupid limits just
> because the infrastructure can't cleanly support per-instance state
> (which is what makes it ugly, its not the shared state). And its not
> so bad, the uglyness comes down to two or three extra lines of code,
> one additional pointer in the data structure and one offsetof in
> userspace.

Nontheless, I think modifying userspace because 2 kernel functions can't talk 
to each other is a bit silly.
It would be relatively easy to modify the kernel in order to support instance 
data (extensive, but simple and relatively safe).
Do you think there would be a strong resistance to a patch in mainline kernel?

-- 
Saluti,
   Massimiliano Hofer
        Nucleus

Re: condition for 2.6.16

[Patrick McHardy]

Massimiliano Hofer wrote:
> On Friday 28 April 2006 5:34 pm, Patrick McHardy wrote:
> 
> 
>>Hehe :) Yes, its not nice, but I refuse to add stupid limits just
>>because the infrastructure can't cleanly support per-instance state
>>(which is what makes it ugly, its not the shared state). And its not
>>so bad, the uglyness comes down to two or three extra lines of code,
>>one additional pointer in the data structure and one offsetof in
>>userspace.
> 
> 
> Nontheless, I think modifying userspace because 2 kernel functions can't talk 
> to each other is a bit silly.
> It would be relatively easy to modify the kernel in order to support instance 
> data (extensive, but simple and relatively safe).
> Do you think there would be a strong resistance to a patch in mainline kernel?

It is silly, but we have to live with it. I see two possibilities, one
breaks userspace, one is extremly expensive. I like neither one.
We should just add a clean netlink configuration interface modelled
after the existing ones instead of building workarounds in this
clearly broken interface. I mostly don't care about costs of a
compatibility layer if we get a sane infrastructure in return.

Re: condition for 2.6.16

[Massimiliano Hofer]

On Saturday 29 April 2006 4:56 am, Patrick McHardy wrote:
> > Nontheless, I think modifying userspace because 2 kernel functions can't
> > talk to each other is a bit silly.
> > It would be relatively easy to modify the kernel in order to support
> > instance data (extensive, but simple and relatively safe).
> > Do you think there would be a strong resistance to a patch in mainline
> > kernel?
>
> It is silly, but we have to live with it. I see two possibilities, one
> breaks userspace, one is extremly expensive. I like neither one.
> We should just add a clean netlink configuration interface modelled
> after the existing ones instead of building workarounds in this
> clearly broken interface. I mostly don't care about costs of a
> compatibility layer if we get a sane infrastructure in return.

I agree about the 2 non invasive (with respect to the mainline kernel) 
solutions.

If I could choose freely I would change these structures (and all 
corresponding references and initialization code) as follows:



struct xt_match
{
...
        int (*match)(const struct sk_buff *skb,
                     const struct net_device *in,
                     const struct net_device *out,
                     const void *matchinfo,
                     int offset,
                     unsigned int protoff,
                     int *hotdrop,
                     void *instance_data);

        int (*checkentry)(const char *tablename,
                          const void *ip,
                          void *matchinfo,
                          unsigned int matchinfosize,
                          unsigned int hook_mask,
                          void **instance_data);

        void (*destroy)(void *matchinfo,
                        unsigned int matchinfosize,
                        void *instance_data);
...
};


struct ipt_entry_match
{
...
	void *instance_data;
        unsigned char data[0];
};


This would solve the general problem of rule specific data (tracking, state, 
accounting, whatever).
It gives more expressiveness to the API in general and would have little 
impact on the parts of the code that do not use it (instance_data is 
initialized as NULL and never used).
The performance penalty would be negligible (4 or 8 more bytes in every 
descriptor and in every function call to the match function).

We could minimize impact on the existing code introducing a "normal" and an 
"extended" match funciton prototype, but this would crete complexity and its 
only purpose is if we don't want to upgrade everything.

The problem with my solution is:
- it's work (I could do it, so it's not your problem :));
- the kernel people may reject it or delay it for an extended period of time;
- while we the patch is available and the kernel people does not accept it 
we'll be stuck with 2 sets of incompatible netfilter APIs with all kinds of 
inconveniences while everyone updates their function declarations (we already 
survived one such change with 2.6.16).

I ask you these questions:
- is my proposal a desirable improvement?
- is there any other way or any additional improvement that we can do?
- would you support it for inclusion in mainline?
- would the kernel people like it?

If you think this is worth the trouble and promise to support the patch  for 
inclusion in mainline (at least after we agree to the implemetation details) 
I will start working on it.

-- 
Saluti,
   Massimiliano Hofer
        Nucleus

Re: condition for 2.6.16

[Massimiliano Hofer]

On Friday 28 April 2006 2:58 pm, Jozsef Kadlecsik wrote:

> Why don't you choose an array and its indices? Array size could be
> specified by module parameter if the default were not sufficient for
> someone.

I would need a way to get the index.
If I put information back in the rule descriptor I can just store a pointer 
(the ultimate array index). If I don't touch the descriptor I'm stuck with a 
pointer to the descriptor and any information I can obtain with it, but no 
unique identifier that is immediately be used as an index in an array.

-- 
Saluti,
   Massimiliano Hofer
        Nucleus

Re: condition for 2.6.16

[Patrick McHardy]

Massimiliano Hofer wrote:
> While we're talking about varying degrees of ugliness, how bad would it be if 
> I optionally allowed to keep a persistent state across rule removal and 
> reinsertion (for example whene someone flushes the table and restarts the 
> firewalling script)?
> I concede that this would really be easy to do in userspace, so maybe I'm 
> answering myself. :)

Now we're talking about _really_ ugly :) How and when would you remove
these? An iptables rule state garbage collector? :) I think this really
should be done in userspace.

Re: condition for 2.6.16

[Massimiliano Hofer]

On Friday 28 April 2006 3:09 pm, Patrick McHardy wrote:
> Now we're talking about _really_ ugly :) How and when would you remove
> these? An iptables rule state garbage collector? :) I think this really
> should be done in userspace.

rm /proc/net/nf_condition/x?

Uglier than ever! :)

-- 
Saluti,
   Massimiliano Hofer
        Nucleus