check timer active

check timer active

[Richard]

Hi,

It looks like that there are two ways to check if a timer for conntrack is
active or not.

ct->timeout->list.next != NULL

test_bit(IPS_CONFIRMED_BIT, &ct->status)

Is there any difference between these two checks?

Thanks,
Richard

Re: check timer active

[Henrik Nordstrom]

On Thu, 2 Dec 2004, Richard wrote:

> It looks like that there are two ways to check if a timer for conntrack is
> active or not.
>
> ct->timeout->list.next != NULL
>
> test_bit(IPS_CONFIRMED_BIT, &ct->status)

These two tests for slightly different things

The first checks that there is a timeout.

The second checks that the conntrack entry is active (in the hash table 
etc...).

The two is at most if not all times equal as all active conntrack entries 
have a timeout of some sort, but maybe it is possible for the timeout to 
be established before the conntrack becomes confirmed or something..

Regards
Henrik

Re: check timer active

[Krisztian Kovacs]

  Hi,

On Sat, 2004-12-04 at 15:55 +0100, Henrik Nordstrom wrote: 
> > ct->timeout->list.next != NULL
> >
> > test_bit(IPS_CONFIRMED_BIT, &ct->status)
> 
> The two is at most if not all times equal as all active conntrack entries 
> have a timeout of some sort, but maybe it is possible for the timeout to 
> be established before the conntrack becomes confirmed or something..

  It is definitely possible if you use ct_sync. Replicated entries on
the slave nodes do not have their timers started, but are in the hash
tables.

-- 
 Krisztian KOVACS

RE: check timer active

[Richard]

>   Hi,
> 
> On Sat, 2004-12-04 at 15:55 +0100, Henrik Nordstrom wrote:
> > > ct->timeout->list.next != NULL
> > >
> > > test_bit(IPS_CONFIRMED_BIT, &ct->status)
> >
> > The two is at most if not all times equal as all active conntrack
> entries
> > have a timeout of some sort, but maybe it is possible for the timeout to
> > be established before the conntrack becomes confirmed or something..
> 
>   It is definitely possible if you use ct_sync. Replicated entries on
> the slave nodes do not have their timers started, but are in the hash
> tables.
> 

I am writing a new target to manipulate the expire value of a conntrack,
i.e. (ct->timeout.expires). If the expire timeout is not fired, it uses the
timeout value. If it is, it uses jiffies+timeout. In order to check what
state it is at, I'd use "ct->timeout->list.next!=NULL)", right?

Thanks,
Richard

Re: check timer active

[Pablo Neira]

Richard wrote:

>>  Hi,
>>
>>On Sat, 2004-12-04 at 15:55 +0100, Henrik Nordstrom wrote:
>>    
>>
>>>>ct->timeout->list.next != NULL
>>>>
>>>>test_bit(IPS_CONFIRMED_BIT, &ct->status)
>>>>        
>>>>
>>>The two is at most if not all times equal as all active conntrack
>>>      
>>>
>>entries
>>    
>>
>>>have a timeout of some sort, but maybe it is possible for the timeout to
>>>be established before the conntrack becomes confirmed or something..
>>>      
>>>
>>  It is definitely possible if you use ct_sync. Replicated entries on
>>the slave nodes do not have their timers started, but are in the hash
>>tables.
>>
>>    
>>
>
>I am writing a new target to manipulate the expire value of a conntrack,
>i.e. (ct->timeout.expires). If the expire timeout is not fired, it uses the
>timeout value. If it is, it uses jiffies+timeout. In order to check what
>state it is at, I'd use "ct->timeout->list.next!=NULL)", right?
>  
>

better use ip_ct_refresh_acct to manipulate a conntrack timer.

--
Pablo

Re: check timer active

[Pablo Neira]

Pablo Neira wrote:

> Richard wrote:
>
>>>  Hi,
>>>
>>> On Sat, 2004-12-04 at 15:55 +0100, Henrik Nordstrom wrote:
>>>   
>>>
>>>>> ct->timeout->list.next != NULL
>>>>>
>>>>> test_bit(IPS_CONFIRMED_BIT, &ct->status)
>>>>>       
>>>>
>>>> The two is at most if not all times equal as all active conntrack
>>>>     
>>>
>>> entries
>>>   
>>>
>>>> have a timeout of some sort, but maybe it is possible for the 
>>>> timeout to
>>>> be established before the conntrack becomes confirmed or something..
>>>>     
>>>
>>>  It is definitely possible if you use ct_sync. Replicated entries on
>>> the slave nodes do not have their timers started, but are in the hash
>>> tables.
>>>
>>>   
>>
>>
>> I am writing a new target to manipulate the expire value of a conntrack,
>> i.e. (ct->timeout.expires). If the expire timeout is not fired, it 
>> uses the
>> timeout value. If it is, it uses jiffies+timeout. In order to check what
>> state it is at, I'd use "ct->timeout->list.next!=NULL)", right?
>>  
>>
>
> better use ip_ct_refresh_acct to manipulate a conntrack timer.


still think that you should use that function.

Hm, I just realized that the amanda helper is faking counters, I guess 
that we need a version of ip_ct_refresh_acct which doesn't the increase 
skb counters. I'll send a patch to fix this once I know what is 
scheduled with nf_conntrack.

--
Pablo

Re: check timer active

[Patrick McHardy]

Pablo Neira wrote:

> still think that you should use that function.
>
> Hm, I just realized that the amanda helper is faking counters, I guess 
> that we need a version of ip_ct_refresh_acct which doesn't the 
> increase skb counters. I'll send a patch to fix this once I know what 
> is scheduled with nf_conntrack.

Feel free to send it now, I still apply bugfixes to ip_conntrack
of course :)

Regards
Patrick

RE: check timer active

[Richard]

> 
> Pablo Neira wrote:
> 
> > still think that you should use that function.
> >
> > Hm, I just realized that the amanda helper is faking counters, I guess
> > that we need a version of ip_ct_refresh_acct which doesn't the
> > increase skb counters. I'll send a patch to fix this once I know what
> > is scheduled with nf_conntrack.

I only saw ip_ct_refresh. What's the difference between ip_ct_refresh_acct
and it?

Richard

Re: check timer active

[Pablo Neira]

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

Patrick McHardy wrote:

> Pablo Neira wrote:
>
>> still think that you should use that function.
>>
>> Hm, I just realized that the amanda helper is faking counters, I 
>> guess that we need a version of ip_ct_refresh_acct which doesn't the 
>> increase skb counters. I'll send a patch to fix this once I know what 
>> is scheduled with nf_conntrack.
>
>
> Feel free to send it now, I still apply bugfixes to ip_conntrack
> of course :)


arg, stupid me, such problem doesn't exist, if skb is NULL we don't 
increase counters, that's what happens in amanda. Maybe the patch 
attached could clean up the current API, anyway that's not a bugfix 
anymore, so I'll hold it here.

--
Pablo

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3347 bytes --]

===== include/linux/netfilter_ipv4/ip_conntrack.h 1.24 vs edited =====
--- 1.24/include/linux/netfilter_ipv4/ip_conntrack.h	2004-10-21 06:13:43 +02:00
+++ edited/include/linux/netfilter_ipv4/ip_conntrack.h	2004-12-05 23:18:54 +01:00
@@ -260,11 +260,15 @@
 extern int invert_tuplepr(struct ip_conntrack_tuple *inverse,
 			  const struct ip_conntrack_tuple *orig);
 
-/* Refresh conntrack for this many jiffies */
+/* Refresh conntrack for this many jiffies and increase counters */
 extern void ip_ct_refresh_acct(struct ip_conntrack *ct,
 			       enum ip_conntrack_info ctinfo,
 			       const struct sk_buff *skb,
 			       unsigned long extra_jiffies);
+
+/* Refresh conntrack for this many jiffies */
+extern void ip_ct_refresh(struct ip_conntrack *ct,
+			  unsigned long extra_jiffies);
 
 /* These are for NAT.  Icky. */
 /* Update TCP window tracking data when NAT mangles the packet */
===== net/ipv4/netfilter/ip_conntrack_amanda.c 1.11 vs edited =====
--- 1.11/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-11-06 08:41:23 +01:00
+++ edited/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-12-05 23:17:37 +01:00
@@ -59,7 +59,7 @@
 
 	/* increase the UDP timeout of the master connection as replies from
 	 * Amanda clients to the server can be quite delayed */
-	ip_ct_refresh_acct(ct, ctinfo, NULL, master_timeout * HZ);
+	ip_ct_refresh(ct, master_timeout * HZ);
 
 	/* No data? */
 	dataoff = skb->nh.iph->ihl*4 + sizeof(struct udphdr);
===== net/ipv4/netfilter/ip_conntrack_core.c 1.73 vs edited =====
--- 1.73/net/ipv4/netfilter/ip_conntrack_core.c	2004-10-29 01:35:43 +02:00
+++ edited/net/ipv4/netfilter/ip_conntrack_core.c	2004-12-05 23:19:31 +01:00
@@ -1089,11 +1089,10 @@
 #endif
 }
 
-/* Refresh conntrack for this many jiffies and do accounting (if skb != NULL) */
-void ip_ct_refresh_acct(struct ip_conntrack *ct, 
-		        enum ip_conntrack_info ctinfo,
-			const struct sk_buff *skb,
-			unsigned long extra_jiffies)
+static inline void __ip_ct_refresh(struct ip_conntrack *ct, 
+				   enum ip_conntrack_info ctinfo,
+				   const struct sk_buff *skb,
+				   unsigned long extra_jiffies)
 {
 	IP_NF_ASSERT(ct->timeout.data == (unsigned long)ct);
 
@@ -1111,6 +1110,23 @@
 		ct_add_counters(ct, ctinfo, skb);
 		WRITE_UNLOCK(&ip_conntrack_lock);
 	}
+}
+
+/* Refresh conntrack for this many jiffies and do accounting.
+ * You should use this function just in protocol helpers. */
+void ip_ct_refresh_acct(struct ip_conntrack *ct,
+			enum ip_conntrack_info ctinfo,
+			const struct sk_buff *skb,
+			unsigned long extra_jiffies)
+{
+	__ip_ct_refresh(ct, ctinfo, skb, extra_jiffies);
+}
+
+/* Just refresh a conntrack some extra jiffies. */
+void ip_ct_refresh(struct ip_conntrack *ct,
+		   unsigned long extra_jiffies)
+{
+	__ip_ct_refresh(ct, 0, NULL, extra_jiffies);
 }
 
 int ip_ct_no_defrag;
===== net/ipv4/netfilter/ip_conntrack_standalone.c 1.53 vs edited =====
--- 1.53/net/ipv4/netfilter/ip_conntrack_standalone.c	2004-10-21 06:13:43 +02:00
+++ edited/net/ipv4/netfilter/ip_conntrack_standalone.c	2004-12-05 23:17:37 +01:00
@@ -889,6 +889,7 @@
 EXPORT_SYMBOL(ip_conntrack_helper_register);
 EXPORT_SYMBOL(ip_conntrack_helper_unregister);
 EXPORT_SYMBOL(ip_ct_selective_cleanup);
+EXPORT_SYMBOL(ip_ct_refresh);
 EXPORT_SYMBOL(ip_ct_refresh_acct);
 EXPORT_SYMBOL(ip_ct_protos);
 EXPORT_SYMBOL(ip_ct_find_proto);

Re: check timer active

[Pablo Neira]

Richard wrote:

>>Pablo Neira wrote:
>>
>>    
>>
>>>still think that you should use that function.
>>>
>>>Hm, I just realized that the amanda helper is faking counters, I guess
>>>that we need a version of ip_ct_refresh_acct which doesn't the
>>>increase skb counters. I'll send a patch to fix this once I know what
>>>is scheduled with nf_conntrack.
>>>      
>>>
>
>I only saw ip_ct_refresh. What's the difference between ip_ct_refresh_acct
>and it?
>  
>

because you are using a caveman kernel tree :) try with 2.6.10-rc3

--
Pablo

Re: check timer active

[Patrick McHardy]

Pablo Neira wrote:

> arg, stupid me, such problem doesn't exist, if skb is NULL we don't 
> increase counters, that's what happens in amanda. Maybe the patch 
> attached could clean up the current API, anyway that's not a bugfix 
> anymore, so I'll hold it here.

As I said I don't want to apply cleanups to ip_conntrack currently,
at least until it's sorted out what happens to it. But I opened a
nf_conntrack tree and will add your cleanup to it once amanda is ported.

Regards
Patrick