ip_ct_refresh lock issue

ip_ct_refresh lock issue

[Pablo Neira]

Hi list,

I can't see the reason why in ip_ct_refresh the conntrack is locked to 
modify an not confirmed conntrack timeout.

It isn't inserted in the conntrack table yet, so why do we need to do 
that? any possible race here?

        WRITE_LOCK(&ip_conntrack_lock);
        /* If not in hash table, timer will not be active yet */
        if (!is_confirmed(ct))
                ct->timeout.expires = extra_jiffies;
        else {
                /* Need del_timer for race avoidance (may already be 
dying). */
                if (del_timer(&ct->timeout)) {
                        ct->timeout.expires = jiffies + extra_jiffies;
                        add_timer(&ct->timeout);
                }
        }
        WRITE_UNLOCK(&ip_conntrack_lock);
}

best regards,
Pablo

Re: ip_ct_refresh lock issue

[Henrik Nordstrom]

On Sat, 28 Feb 2004, Pablo Neira wrote:

> I can't see the reason why in ip_ct_refresh the conntrack is locked to 
> modify an not confirmed conntrack timeout.

Good question.

But maybe there is some complication from NAT or related connections
requiring this locking?

If not the lock should probably be moved down to the is_confirmed case. If
the connections is not yet confirmed then the only reference should be
from this packet (not yet in the hashes) and there should never be any
concurrent accesses.

Regards
Henrik

Re: ip_ct_refresh lock issue

[Pablo Neira]

Hi henrik,

Henrik Nordstrom wrote:

>But maybe there is some complication from NAT or related connections
>requiring this locking?
>  
>
Actually a quick view at:

http://lxr.linux.no/ident?v=2.6.1&i=ip_ct_refresh

show me that this function is only called from conntrack related stuff, 
specifically from protocol and helpers, so i can't see that complication 
from NAT nor with related connections. Anyway, I'm not sure at all... 
that's why i need some feedback.

>If not the lock should probably be moved down to the is_confirmed case. If
>the connections is not yet confirmed then the only reference should be
>from this packet (not yet in the hashes) and there should never be any
>concurrent accesses.
>  
>
yes, this is exactly what i had in mind :-).

thanks,
Pablo