[PATCH] Dump mark even if event is a DESTROY event

[PATCH] Dump mark even if event is a DESTROY event

[Eric Leblond]

[-- Attachment #1.1: Type: text/plain, Size: 350 bytes --]

Hi,

Mark is scarcely dump by nfnetlink_conntrack event handler. As it can be
used to test that a connection is part of a given subset, I think it
should be almost always dumped.

The attached patch implements this behaviour.

BR,
-- 
Éric Leblond, eleblond@inl.fr
Téléphone : 01 44 89 46 39, Fax : 01 44 89 45 01
INL, http://www.inl.fr

[-- Attachment #1.2: 0002-Dump-mark-even-if-event-is-a-DESTROY-event.txt --]
[-- Type: text/plain, Size: 1159 bytes --]

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

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index c64f029..3e291e3 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -389,17 +389,17 @@ static int ctnetlink_conntrack_event(struct notifier_block *this,
 		    && ctnetlink_dump_helpinfo(skb, ct) < 0)
 		    	goto nfattr_failure;
 
-#ifdef CONFIG_NF_CONNTRACK_MARK
-		if ((events & IPCT_MARK || ct->mark)
-		    && ctnetlink_dump_mark(skb, ct) < 0)
-		    	goto nfattr_failure;
-#endif
-
 		if (events & IPCT_COUNTER_FILLING &&
 		    (ctnetlink_dump_counters(skb, ct, IP_CT_DIR_ORIGINAL) < 0 ||
 		     ctnetlink_dump_counters(skb, ct, IP_CT_DIR_REPLY) < 0))
 			goto nfattr_failure;
 	}
+	
+#ifdef CONFIG_NF_CONNTRACK_MARK
+	if ((events & IPCT_MARK || ct->mark)
+	    && ctnetlink_dump_mark(skb, ct) < 0)
+	    	goto nfattr_failure;
+#endif
 
 	nlh->nlmsg_len = skb->tail - b;
 	nfnetlink_send(skb, 0, group, 0);
-- 
1.4.4.3


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

Re: [PATCH] Dump mark even if event is a DESTROY event

[Pablo Neira Ayuso]

Hi Eric,

Eric Leblond wrote:
> Mark is scarcely dump by nfnetlink_conntrack event handler. As it can be
> used to test that a connection is part of a given subset, I think it
> should be almost always dumped.

I don't see why you may need the mark in the destroy message. You can 
keep a cache in userspace with the connections that belong to a certain 
subset and their marks, then if the mark changes, move such connection 
the a different subset. It doesn't make sense to me the idea of 
including the mark in the destroy message since such mark didn't change 
with regards to the previous event delivered.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of 
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: [PATCH] Dump mark even if event is a DESTROY event

[Eric Leblond]

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

Hi,

Le mercredi 21 février 2007 à 13:51 +0100, Pablo Neira Ayuso a écrit :
> Hi Eric,
> 
> I don't see why you may need the mark in the destroy message. You can 
> keep a cache in userspace with the connections that belong to a certain 
> subset and their marks, then if the mark changes, move such connection 
> the a different subset. It doesn't make sense to me the idea of 
> including the mark in the destroy message since such mark didn't change 
> with regards to the previous event delivered.

I do not agree with the idea of having a cache in userspace. It has been
coded in kernel and for this kind of stuff, once is enough. I really
want to avoid all synchronisation problems we could have to do this in
userspace.

Furthermore, mark exists to be able to create subset for other tools
like tc or ip.

BR,
-- 
Éric Leblond, eleblond@inl.fr
Téléphone : 01 44 89 46 39, Fax : 01 44 89 45 01
INL, http://www.inl.fr

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

Re: [PATCH] Dump mark even if event is a DESTROY event

[Amin Azez]

* Eric Leblond wrote, On 21/02/07 13:13:
> Hi,
> 
> Le mercredi 21 février 2007 à 13:51 +0100, Pablo Neira Ayuso a écrit :
>> Hi Eric,
>>
>> I don't see why you may need the mark in the destroy message. You can 
>> keep a cache in userspace with the connections that belong to a certain 
>> subset and their marks, then if the mark changes, move such connection 
>> the a different subset. It doesn't make sense to me the idea of 
>> including the mark in the destroy message since such mark didn't change 
>> with regards to the previous event delivered.
> 
> I do not agree with the idea of having a cache in userspace. It has been
> coded in kernel and for this kind of stuff, once is enough. I really
> want to avoid all synchronisation problems we could have to do this in
> userspace.
> 
> Furthermore, mark exists to be able to create subset for other tools
> like tc or ip.


I'm with Eric.  I'm already using a patch like his.

Why keep a cache in user-space when there is already a cache in
kernel-space?

There are two clear choices; the old ulogd that keeps a full cache in
user space and receives every packet over netlink; or conntrack that
keeps a whole cache in kernel-space, updates counters in kernel space
and only sends significant updates over netlink.

To replicate the whole conntrack hash in user-space to avoid sending a
couple of bytes over netlink on connection-destroy is to seek after some
kind of optimization that I don't rightly understand.

Sam