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

--

[ message continues ]

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
 	  ...
[ message continues ]

Cc'ing Alexey who has done the netns support.

Alexey, you added par->net, but given Luciano just did it with 


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 

--

[ message continues ]

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

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.

--

[ message continues ]

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

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.)
--

[ message continues ]

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.

--

[ message continues ]

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.
--

[ message continues ]

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.

--

[ message continues ]