[RFC] h323 module unloading fix

[RFC] h323 module unloading fix

[Harald Welte]

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

Hi!

If you unload the ip_conntrack_h323 module while a h323 session is in
progress, the conntrack table is left with corrupted entries (->helper
pointer still pointing to a now no longer existing h245 helper).

This is because the h245 helper is not registered with the core, and
therefore the helper_unregister() function will only be called for h245,
but not for h245.

The proposed solution:

1) to make ip_conntrack_core:ip_conntrack_helper_unregister() work with
   a helper that was never registered (and thus has an empty list.next)
2) to have ip_conntrack_h323 unregister the h245 helper.

Any comments?

Patches for h323(pom) and 2.4.29 attached.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #1.2: patch-2.4.29-h323-h245-unregister.patch --]
[-- Type: text/plain, Size: 586 bytes --]

--- a/net/ipv4/netfilter/ip_conntrack_core.c	2005-02-08 15:26:57.000000000 +0100
+++ b/net/ipv4/netfilter/ip_conntrack_core.c	2005-02-08 15:27:36.000000000 +0100
@@ -1149,7 +1149,11 @@
 
 	/* Need write lock here, to delete helper. */
 	WRITE_LOCK(&ip_conntrack_lock);
-	LIST_DELETE(&helpers, me);
+	
+	/* Conditional since some helpers (h245) are not really registered
+	 * and thus don't appear in our global list*/
+	if (me->list.next)
+		LIST_DELETE(&helpers, me);
 
 	/* Get rid of expecteds, set helpers to NULL. */
 	for (i = 0; i < ip_conntrack_htable_size; i++)

[-- Attachment #1.3: f --]
[-- Type: text/plain, Size: 349 bytes --]


--- a/net/ipv4/netfilter/ip_conntrack_h323.c	2005-02-08 15:33:36.000000000 +0100
+++ b/net/ipv4/netfilter/ip_conntrack_h323.c	2005-02-08 15:33:51.000000000 +0100
@@ -300,6 +300,7 @@
 {
 	/* Unregister H.225 helper */	
 	ip_conntrack_helper_unregister(&h225);
+	ip_conntrack_helper_unregister(&h245);
 }
 
 EXPORT_SYMBOL(ip_h323_lock);


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC] h323 module unloading fix

[Max Kellermann]

On 2005/02/08 15:59, Harald Welte <laforge@netfilter.org> wrote:
[...]
> This is because the h245 helper is not registered with the core, and
> therefore the helper_unregister() function will only be called for h245,
> but not for h245.
> 
> The proposed solution:
> 
> 1) to make ip_conntrack_core:ip_conntrack_helper_unregister() work with
>    a helper that was never registered (and thus has an empty list.next)
> 2) to have ip_conntrack_h323 unregister the h245 helper.

I think your patch regarding ip_conntrack_core.c is a workaround, that
does not look clean to me. I prefer to make H.323 register the H.245
helper with a disabled mask (only to be activated internally by
H.225), and then unregister it cleanly.

I guess your proposition (1) would make another workaround.

Max

Re: [RFC] h323 module unloading fix

[Harald Welte]

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

On Tue, Feb 08, 2005 at 04:26:20PM +0100, Max Kellermann wrote:

> I think your patch regarding ip_conntrack_core.c is a workaround, that
> does not look clean to me. I prefer to make H.323 register the H.245
> helper with a disabled mask (only to be activated internally by
> H.225), and then unregister it cleanly.

Yes, but every additional entry in the list of helpers is matched
against every packet...  and I don't want to waste performance just for
being a bit more clean.

But maybe we could add a flag to the helper like F_DISABLED which would
disable matching for that particular helper.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC] h323 module unloading fix

[Max Kellermann]

On 2005/02/08 16:52, Harald Welte <laforge@netfilter.org> wrote:
> Yes, but every additional entry in the list of helpers is matched
> against every packet...  and I don't want to waste performance just for
> being a bit more clean.
> 
> But maybe we could add a flag to the helper like F_DISABLED which would
> disable matching for that particular helper.

That flag could be determined automatically in the register function
by looking at the mask.

Then the helper could be inserted into another list, which is only
used in the unregister function for cleanup. This way, the netfilter
core does not even have to traverse these helpers in the list to check
the "disabled" flag.

Max

Re: [RFC] h323 module unloading fix

[Patrick McHardy]

Harald Welte wrote:

>Hi!
>
>If you unload the ip_conntrack_h323 module while a h323 session is in
>progress, the conntrack table is left with corrupted entries (->helper
>pointer still pointing to a now no longer existing h245 helper).
>
>This is because the h245 helper is not registered with the core, and
>therefore the helper_unregister() function will only be called for h245,
>but not for h245.
>
>The proposed solution:
>
>1) to make ip_conntrack_core:ip_conntrack_helper_unregister() work with
>   a helper that was never registered (and thus has an empty list.next)
>2) to have ip_conntrack_h323 unregister the h245 helper.
>
>Any comments?
>
You can initialize helper->list and replace LIST_DELETE by list_del, 
then you
can avoid the check for list.next. But I think splitting the code to evict
expectations and expected conntracks from ip_conntrack_helper_unregister is
a cleaner solution.

Regards
Patrick

Re: [RFC] h323 module unloading fix

[Harald Welte]

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

On Wed, Feb 09, 2005 at 12:10:55AM +0100, Patrick McHardy wrote:
> You can initialize helper->list and replace LIST_DELETE by list_del, 
> then you can avoid the check for list.next. 

I thought about that for a second, but somehow deferred the idea.
Looking at other options, I think I'll go for that solution.

> But I think splitting the code to evict expectations and expected
> conntracks from ip_conntrack_helper_unregister is a cleaner solution.

Mh, but then every author of a helper would explicitly have to take care
of calling the 'evicting function'.  Considering that any existing
helper, tutorial and other documentation tells him that he doesn't have
to, it's easy to get it wrong and introduce further bugs.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC] h323 module unloading fix

[Jozsef Kadlecsik]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 991 bytes --]

Hi,

On Wed, 9 Feb 2005, Harald Welte wrote:

> On Wed, Feb 09, 2005 at 12:10:55AM +0100, Patrick McHardy wrote:
> > But I think splitting the code to evict expectations and expected
> > conntracks from ip_conntrack_helper_unregister is a cleaner solution.
>
> Mh, but then every author of a helper would explicitly have to take care
> of calling the 'evicting function'.  Considering that any existing
> helper, tutorial and other documentation tells him that he doesn't have
> to, it's easy to get it wrong and introduce further bugs.

I like Patrick's idea. There are not so many modules wich rely on
unregistered helpers, and we could retain backward compatibility in the
API like in the attached (untested) patch. What do you think?

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

[-- Attachment #2: unhelp.patch --]
[-- Type: TEXT/PLAIN, Size: 3052 bytes --]

diff -urN --exclude-from=/usr/src/diff.exclude linux-2.4.26-orig/include/linux/netfilter_ipv4/ip_conntrack_helper.h linux-2.4.26-unhelp/include/linux/netfilter_ipv4/ip_conntrack_helper.h
--- linux-2.4.26-orig/include/linux/netfilter_ipv4/ip_conntrack_helper.h	2002-11-29 00:53:15.000000000 +0100
+++ linux-2.4.26-unhelp/include/linux/netfilter_ipv4/ip_conntrack_helper.h	2005-02-09 09:28:50.000000000 +0100
@@ -42,4 +42,7 @@
 				      struct ip_conntrack_tuple *newtuple);
 extern void ip_conntrack_unexpect_related(struct ip_conntrack_expect *exp);
 
+/* Delete expectations belonging to a (unregistered) helper */
+extern void ip_conntrack_evict_expectations(struct ip_conntrack_helper *);
+
 #endif /*_IP_CONNTRACK_HELPER_H*/
diff -urN --exclude-from=/usr/src/diff.exclude linux-2.4.26-orig/net/ipv4/netfilter/ip_conntrack_core.c linux-2.4.26-unhelp/net/ipv4/netfilter/ip_conntrack_core.c
--- linux-2.4.26-orig/net/ipv4/netfilter/ip_conntrack_core.c	2004-02-18 14:36:32.000000000 +0100
+++ linux-2.4.26-unhelp/net/ipv4/netfilter/ip_conntrack_core.c	2005-02-09 09:26:33.000000000 +0100
@@ -1147,27 +1147,39 @@
 	return 0;
 }
 
-void ip_conntrack_helper_unregister(struct ip_conntrack_helper *me)
+static inline void __evict_expectations(struct ip_conntrack_helper *me)
 {
 	unsigned int i;
 
-	/* Need write lock here, to delete helper. */
 	WRITE_LOCK(&ip_conntrack_lock);
-	LIST_DELETE(&helpers, me);
-
 	/* Get rid of expecteds, set helpers to NULL. */
 	for (i = 0; i < ip_conntrack_htable_size; i++)
 		LIST_FIND_W(&ip_conntrack_hash[i], unhelp,
 			    struct ip_conntrack_tuple_hash *, me);
 	WRITE_UNLOCK(&ip_conntrack_lock);
+}
+
+void ip_conntrack_helper_unregister(struct ip_conntrack_helper *me)
+{
+	WRITE_LOCK(&ip_conntrack_lock);
+	LIST_DELETE(&helpers, me);
+	WRITE_UNLOCK(&ip_conntrack_lock);
 
 	/* Someone could be still looking at the helper in a bh. */
 	br_write_lock_bh(BR_NETPROTO_LOCK);
 	br_write_unlock_bh(BR_NETPROTO_LOCK);
 
+	/* Delete expectations belonging to the helper */
+	__evict_expectations(me);
+	
 	MOD_DEC_USE_COUNT;
 }
 
+void ip_conntrack_evict_expectations(struct ip_conntrack_helper *me)
+{
+	__evict_expectations(me);
+}
+
 /* Refresh conntrack for this many jiffies. */
 void ip_ct_refresh(struct ip_conntrack *ct, unsigned long extra_jiffies)
 {
diff -urN --exclude-from=/usr/src/diff.exclude linux-2.4.26-orig/net/ipv4/netfilter/ip_conntrack_standalone.c linux-2.4.26-unhelp/net/ipv4/netfilter/ip_conntrack_standalone.c
--- linux-2.4.26-orig/net/ipv4/netfilter/ip_conntrack_standalone.c	2004-02-18 14:36:32.000000000 +0100
+++ linux-2.4.26-unhelp/net/ipv4/netfilter/ip_conntrack_standalone.c	2005-02-09 09:26:58.000000000 +0100
@@ -466,6 +466,7 @@
 EXPORT_SYMBOL(ip_conntrack_get);
 EXPORT_SYMBOL(ip_conntrack_helper_register);
 EXPORT_SYMBOL(ip_conntrack_helper_unregister);
+EXPORT_SYMBOL(ip_conntrack_evict_expectations);
 EXPORT_SYMBOL(ip_ct_selective_cleanup);
 EXPORT_SYMBOL(ip_ct_refresh);
 EXPORT_SYMBOL(ip_ct_find_proto);

[-- Attachment #3: h323.patch --]
[-- Type: TEXT/PLAIN, Size: 416 bytes --]


--- a/net/ipv4/netfilter/ip_conntrack_h323.c	2005-02-08 15:33:36.000000000 +0100
+++ b/net/ipv4/netfilter/ip_conntrack_h323.c	2005-02-08 15:33:51.000000000 +0100
@@ -300,6 +300,8 @@
 {
 	/* Unregister H.225 helper */	
 	ip_conntrack_helper_unregister(&h225);
+	/* Delete expectations belonging to the unregistered helper */
+	ip_conntrack_evict_expectations(&h245);
 }
 
 EXPORT_SYMBOL(ip_h323_lock);

Re: [RFC] h323 module unloading fix

[Max Kellermann]

On 2005/02/09 09:59, Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:
> I like Patrick's idea. There are not so many modules wich rely on
> unregistered helpers, and we could retain backward compatibility in the
> API like in the attached (untested) patch. What do you think?

Calling an "evicting" function in a module destructor for something
which the netfilter core didn't even know about until now doesn't look
elegant to me. For me, to every destructing function, there must be a
prior constructor call.

There is no compatibility problem with Harald's/my solution. My
proposition allows the "register_helper" function to register
"disabled" helpers, meaning that you have to both register and
unregister the H.245 helper.

If we ever decide that it is required to register "disabled" helpers,
we don't have to change the API again, and it costs us near to nothing
to instruct that now.

It does not matter if "register_helper" ignores disabled helpers for
now, and "unregister_helper" works like the "evicting" function
Patrick proposed; we can change internals like this any time we want
without having to touch modules again.

Max

Re: [RFC] h323 module unloading fix

[Jozsef Kadlecsik]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1301 bytes --]

On Wed, 9 Feb 2005, Max Kellermann wrote:

> On 2005/02/09 09:59, Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:
> > I like Patrick's idea. There are not so many modules wich rely on
> > unregistered helpers, and we could retain backward compatibility in the
> > API like in the attached (untested) patch. What do you think?
>
> Calling an "evicting" function in a module destructor for something
> which the netfilter core didn't even know about until now doesn't look
> elegant to me. For me, to every destructing function, there must be a
> prior constructor call.
>
> There is no compatibility problem with Harald's/my solution. My
> proposition allows the "register_helper" function to register
> "disabled" helpers, meaning that you have to both register and
> unregister the H.245 helper.
>
> If we ever decide that it is required to register "disabled" helpers,
> we don't have to change the API again, and it costs us near to nothing
> to instruct that now.

Good reasoning. What about then the attached (again, untested) variants?
:-)

Bes 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

[-- Attachment #2: unhelp2.patch --]
[-- Type: TEXT/PLAIN, Size: 2818 bytes --]

diff -urN --exclude-from=/usr/src/diff.exclude linux-2.4.26-orig/include/linux/netfilter_ipv4/ip_conntrack_helper.h linux-2.4.26-unhelp/include/linux/netfilter_ipv4/ip_conntrack_helper.h
--- linux-2.4.26-orig/include/linux/netfilter_ipv4/ip_conntrack_helper.h	2002-11-29 00:53:15.000000000 +0100
+++ linux-2.4.26-unhelp/include/linux/netfilter_ipv4/ip_conntrack_helper.h	2005-02-09 10:23:07.000000000 +0100
@@ -8,6 +8,9 @@
 /* Reuse expectation when max_expected reached */
 #define IP_CT_HELPER_F_REUSE_EXPECT	0x01
 
+/* Dynamic helper, attached to conntrack by expectfn */
+#define IP_CT_HELPER_F_DYNAMIC		0x02
+
 struct ip_conntrack_helper
 {	
 	struct list_head list; 		/* Internal use. */
diff -urN --exclude-from=/usr/src/diff.exclude linux-2.4.26-orig/include/linux/netfilter_ipv4/ip_nat_helper.h linux-2.4.26-unhelp/include/linux/netfilter_ipv4/ip_nat_helper.h
--- linux-2.4.26-orig/include/linux/netfilter_ipv4/ip_nat_helper.h	2003-06-13 16:51:38.000000000 +0200
+++ linux-2.4.26-unhelp/include/linux/netfilter_ipv4/ip_nat_helper.h	2005-02-09 10:40:22.000000000 +0100
@@ -11,6 +11,8 @@
 #define IP_NAT_HELPER_F_ALWAYS		0x01
 /* Standalone NAT helper, without a conntrack part */
 #define IP_NAT_HELPER_F_STANDALONE	0x02
+/* Dynamically attached helper */
+#define IP_NAT_HELPER_F_DYNAMIC		0x03
 
 struct ip_nat_helper
 {
diff -urN --exclude-from=/usr/src/diff.exclude linux-2.4.26-orig/net/ipv4/netfilter/ip_conntrack_core.c linux-2.4.26-unhelp/net/ipv4/netfilter/ip_conntrack_core.c
--- linux-2.4.26-orig/net/ipv4/netfilter/ip_conntrack_core.c	2004-02-18 14:36:32.000000000 +0100
+++ linux-2.4.26-unhelp/net/ipv4/netfilter/ip_conntrack_core.c	2005-02-09 10:58:02.000000000 +0100
@@ -621,7 +621,8 @@
 static inline int helper_cmp(const struct ip_conntrack_helper *i,
 			     const struct ip_conntrack_tuple *rtuple)
 {
-	return ip_ct_tuple_mask_cmp(rtuple, &i->tuple, &i->mask);
+	return (!(i->flags & IP_CT_HELPER_F_DYNAMIC)
+		&& ip_ct_tuple_mask_cmp(rtuple, &i->tuple, &i->mask));
 }
 
 struct ip_conntrack_helper *ip_ct_find_helper(const struct ip_conntrack_tuple *tuple)
diff -urN --exclude-from=/usr/src/diff.exclude linux-2.4.26-orig/net/ipv4/netfilter/ip_nat_helper.c linux-2.4.26-unhelp/net/ipv4/netfilter/ip_nat_helper.c
--- linux-2.4.26-orig/net/ipv4/netfilter/ip_nat_helper.c	2003-11-28 19:26:21.000000000 +0100
+++ linux-2.4.26-unhelp/net/ipv4/netfilter/ip_nat_helper.c	2005-02-09 10:54:26.000000000 +0100
@@ -459,7 +459,8 @@
 helper_cmp(const struct ip_nat_helper *helper,
 	   const struct ip_conntrack_tuple *tuple)
 {
-	return ip_ct_tuple_mask_cmp(tuple, &helper->tuple, &helper->mask);
+	return (!(i->helper & IP_NAT_HELPER_F_DYNAMIC)
+		&& ip_ct_tuple_mask_cmp(tuple, &helper->tuple, &helper->mask));
 }
 
 #define MODULE_MAX_NAMELEN		32

[-- Attachment #3: h323_2.patch --]
[-- Type: TEXT/PLAIN, Size: 2246 bytes --]

diff -urN linux/net/ipv4/netfilter/ip_conntrack_h323.c a/net/ipv4/netfilter/ip_conntrack_h323.c
--- linux/net/ipv4/netfilter/ip_conntrack_h323.c	2004-10-14 11:42:40.000000000 +0200
+++ a/net/ipv4/netfilter/ip_conntrack_h323.c	2005-02-09 10:57:04.000000000 +0100
@@ -130,8 +130,8 @@
 /* H.245 helper is not registered! */
 static struct ip_conntrack_helper h245 = 
 	{ { NULL, NULL },
-          "H.245",				/* name */
-          IP_CT_HELPER_F_REUSE_EXPECT,		/* flags */
+          "H.245",				/* name, then flags */
+          IP_CT_HELPER_F_REUSE_EXPECT | IP_CT_HELPER_F_DYNAMIC,
           NULL,					/* module */
           8,					/* max_ expected */
           240,					/* timeout */
@@ -293,13 +293,25 @@
 
 static int __init init(void)
 {
-	return ip_conntrack_helper_register(&h225);
+	int ret;
+	
+	ret = ip_conntrack_helper_register(&h225);
+	if (ret != 0)
+		return ret;
+		
+	ret = ip_conntrack_helper_register(&h245);
+	if (ret != 0) {
+		ip_conntrack_helper_unregister(&h225);
+		return ret;
+	}
+	
+	return 0;
 }
 
 static void __exit fini(void)
 {
-	/* Unregister H.225 helper */	
 	ip_conntrack_helper_unregister(&h225);
+	ip_conntrack_helper_unregister(&h245);
 }
 
 EXPORT_SYMBOL(ip_h323_lock);
diff -urN linux/net/ipv4/netfilter/ip_nat_h323.c a/net/ipv4/netfilter/ip_nat_h323.c
--- linux/net/ipv4/netfilter/ip_nat_h323.c	2004-10-14 11:42:40.000000000 +0200
+++ a/net/ipv4/netfilter/ip_nat_h323.c	2005-02-09 10:56:49.000000000 +0100
@@ -54,7 +54,7 @@
 static struct ip_nat_helper h245 = 
 	{ { NULL, NULL },
           "H.245",				/* name */
-	  0,					/* flags */
+	  IP_NAT_HELPER_F_DYNAMIC,		/* flags */
 	  NULL,					/* module */
 	  { { 0, { 0 } },			/* tuple */
 	    { 0, { 0 }, IPPROTO_TCP } },
@@ -403,16 +403,21 @@
 	int ret;
 	
 	ret = ip_nat_helper_register(&h225);
-
 	if (ret != 0)
-		printk("ip_nat_h323: cannot initialize the module!\n");
-
-	return ret;
+		return ret;
+		
+	ret = ip_nat_helper_register(&h245);
+	if (ret != 0) {
+		ip_nat_helper_unregister(&h225);
+		return ret;
+	}
+	return 0;
 }
 
 static void __exit fini(void)
 {
 	ip_nat_helper_unregister(&h225);
+	ip_nat_helper_unregister(&h245);
 }
 
 module_init(init);

Re: [RFC] h323 module unloading fix

[Harald Welte]

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

On Wed, Feb 09, 2005 at 09:59:09AM +0100, Jozsef Kadlecsik wrote:
> Hi,
> 
> On Wed, 9 Feb 2005, Harald Welte wrote:
> 
> > On Wed, Feb 09, 2005 at 12:10:55AM +0100, Patrick McHardy wrote:
> > > But I think splitting the code to evict expectations and expected
> > > conntracks from ip_conntrack_helper_unregister is a cleaner solution.
> >
> > Mh, but then every author of a helper would explicitly have to take care
> > of calling the 'evicting function'.  Considering that any existing
> > helper, tutorial and other documentation tells him that he doesn't have
> > to, it's easy to get it wrong and introduce further bugs.
> 
> I like Patrick's idea. There are not so many modules wich rely on
> unregistered helpers, and we could retain backward compatibility in the
> API like in the attached (untested) patch. What do you think?

ok, but let's rename the function. 'evict expectations' sounds to me
(non-native-english-speaker) like we would be dropping the expectations.
'unhelp' is clumsy but more precise.  any better suggestions?

> Jozsef

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC] h323 module unloading fix

[Patrick McHardy]

Harald Welte wrote:

>On Wed, Feb 09, 2005 at 12:10:55AM +0100, Patrick McHardy wrote:
>
>>But I think splitting the code to evict expectations and expected
>>conntracks from ip_conntrack_helper_unregister is a cleaner solution.
>>
>
>Mh, but then every author of a helper would explicitly have to take care
>of calling the 'evicting function'.  Considering that any existing
>helper, tutorial and other documentation tells him that he doesn't have
>to, it's easy to get it wrong and introduce further bugs.
>
I meant you should still call it from ip_conntrack_helper_unregister,
but also from the h323 helper. Of course its mostly cosmetic, a comment
before calling unregister is probably just as good.

Regards
Patrick