[PATCH] locking issue in __unexpect_related

[PATCH] locking issue in __unexpect_related

[Pablo Neira]

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

Hi Harald and list,

In the function __unexpect_related:

         /* delete from global and local lists */
         list_del(&expect->list);
         list_del(&expect->expected_list);

If an entry of the global list of expectation is erased, shouldn't this 
operation be write_lock'ed? If missing something, please let me know.

regards,
Pablo

[-- Attachment #2: unexpect_related-lock.patch --]
[-- Type: text/plain, Size: 485 bytes --]

--- linux-2.6.3-old/net/ipv4/netfilter/ip_conntrack_core.c	2004-04-14 21:32:58.000000000 +0200
+++ linux-2.6.3/net/ipv4/netfilter/ip_conntrack_core.c	2004-04-22 00:55:42.000000000 +0200
@@ -226,7 +226,9 @@
 	IP_NF_ASSERT(!expect->sibling);
 
 	/* delete from global and local lists */
+	WRITE_LOCK(&ip_conntrack_expect_list);
 	list_del(&expect->list);
+	WRITE_UNLOCK(&ip_conntrack_expect_list);
 	list_del(&expect->expected_list);
 
 	/* decrement expect-count of master conntrack */

Re: [PATCH] locking issue in __unexpect_related

[Patrick McHardy]

Pablo Neira wrote:
> Hi Harald and list,
> 
> In the function __unexpect_related:
> 
>         /* delete from global and local lists */
>         list_del(&expect->list);
>         list_del(&expect->expected_list);
> 
> If an entry of the global list of expectation is erased, shouldn't this 
> operation be write_lock'ed? If missing something, please let me know.

Your patch will deadlock, the function should only be called under
write-locked ip_conntrack_lock, which you can see by the
MUST_BE_WRITE_LOCKED assertion. Check the callers ..

Regards
Patrick


> 
> regards,
> Pablo
> 
> 
> ------------------------------------------------------------------------
> 
> --- linux-2.6.3-old/net/ipv4/netfilter/ip_conntrack_core.c	2004-04-14 21:32:58.000000000 +0200
> +++ linux-2.6.3/net/ipv4/netfilter/ip_conntrack_core.c	2004-04-22 00:55:42.000000000 +0200
> @@ -226,7 +226,9 @@
>  	IP_NF_ASSERT(!expect->sibling);
>  
>  	/* delete from global and local lists */
> +	WRITE_LOCK(&ip_conntrack_expect_list);
>  	list_del(&expect->list);
> +	WRITE_UNLOCK(&ip_conntrack_expect_list);
>  	list_del(&expect->expected_list);
>  
>  	/* decrement expect-count of master conntrack */