ctnetlink & helper_locking_fix conflict

ctnetlink & helper_locking_fix conflict

[Henrik Nordstrom]

Hi,

there is a conflict between the above two patches. The helper_locking_fix 
removes the locks from the helpers, but ctnetlink adds code using these 
locks.

>From reading the code I think the helper side locking should be removed 
from the ctnetlink additions as well, but just wanted to verify with you.

Example of the offending code (ip_conntrack_irc.c, and many more):

+static void ctnl_change(struct ip_conntrack *ct, union ip_conntrack_help 
*h)
+{
+	LOCK_BH(&ip_irc_lock);
+	memcpy(&ct->help, h, sizeof(ct->help));
+	UNLOCK_BH(&ip_irc_lock);
+}
+
+static void ctnl_new_expect(struct ip_conntrack_expect *exp,
+                            union ip_conntrack_expect_proto *p,
+                            union ip_conntrack_expect_help *h)
+{
+	if (h == NULL)
+		return;
+	LOCK_BH(&ip_irc_lock);
+	memcpy(&exp->help, h, sizeof(exp->help));
+	UNLOCK_BH(&ip_irc_lock);
+}
+

where ip_irc_lock was killed by helper_locking_fix.


Regards
Henrik

Re: ctnetlink & helper_locking_fix conflict

[Harald Welte]

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

On Tue, Nov 09, 2004 at 08:13:33AM +0100, Henrik Nordstrom wrote:
> Hi,
> 
> there is a conflict between the above two patches. The helper_locking_fix 
> removes the locks from the helpers, but ctnetlink adds code using these 
> locks.
> 
> >From reading the code I think the helper side locking should be removed 
> from the ctnetlink additions as well, but just wanted to verify with you.

I think it is safe.  We're holding a write lock on ip_conntrack_lock and
thus it's safe to change the helper data.

If you have a fix for ctnetlink, that would be appreciated

> Regards
> Henrik

-- 
- 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: ctnetlink & helper_locking_fix conflict

[Pablo Neira]

Harald Welte wrote:

>On Tue, Nov 09, 2004 at 08:13:33AM +0100, Henrik Nordstrom wrote:
>  
>
>>Hi,
>>
>>there is a conflict between the above two patches. The helper_locking_fix 
>>removes the locks from the helpers, but ctnetlink adds code using these 
>>locks.
>>
>>>From reading the code I think the helper side locking should be removed 
>>from the ctnetlink additions as well, but just wanted to verify with you.
>>    
>>
>
>I think it is safe.  We're holding a write lock on ip_conntrack_lock and
>thus it's safe to change the helper data.
>
>If you have a fix for ctnetlink, that would be appreciated
>  
>

I think that there's a minor race. In ip_conntrack_in()

855                 ret = ct->helper->help(*pskb, ct, ctinfo)

This call isn't in a locked section. Someone could be calling the helper 
at the same time that ctnetlink is modifying the private helper info. 
I've annotated this issue since I'll be working in that code (change 
API) once I've done with the event stuff.

Pablo