[prev in list] [next in list] [prev in thread] [next in thread] 

List:       netfilter-devel
Subject:    Re: [PATCH 2/3] set SEEN_REPLY before destroying conntrack on TCP
From:       Patrick McHardy <kaber () trash ! net>
Date:       2008-05-27 14:48:52
Message-ID: 483C1F54.6040505 () trash ! net
[Download RAW message or body]

Fabian Hugelshofer wrote:
> Patrick McHardy wrote:
>> Fabian Hugelshofer wrote:
>>> On Thu, 2008-05-22 at 10:13 +0100, Fabian Hugelshofer wrote:
>>>> If a connection fails with a TCP reset, the conntrack is destroyed 
>>>> immediately. This patch sets the SEEN_REPLY bit before destroying 
>>>> the conntrack.
>>>
>>> This updated version also increments the accounting counters.
>>
>> Thanks, but this needs to be changed slightly.
> [...]
>> I think a better way is to encapsulate the del_timer/timeout.function
>> calls in a nf_ct_kill() function and perform accounting there.
>> Since all manual invocations of timeout.function are/should be
>> performed only while handling packets (that are usually not
>> accounted), this seems like the right way.
> 
> Ok, I see. But for accounting ctinfo and skbuf are required. I'll 
> include them in the argument list of nf_ct_kill() and update the 
> function invocations, ok? Or should I introduce an nf_ct_kill_acct()?

All callers in my patch want the accounting, so a seperate function
would make this one useless. So I'd go with adding a struct sk_buff *
argument, even though its not particulary pretty :)

> I just did another test where my SEEN_REPLY patch was not applied. 
> Surprisingly the SEEN_REPLY bit was set in the destroy events. I am 
> afraid, but I have to assume, that I did not evaluate the bahavior 
> carefully enough. Probably I confused the accounting, no status and no 
> related packets issues.
> 
> Unless a race condition might be thinkable, we should leave the 
> SEEN_REPLY patch. If it is possible, that the timeout function 
> immediately triggers the destroy event to be exported over netlink, then 
> the patch is still necessary. I don't see things detailed enough to 
> judge this. If it is necessary, should it be included in nf_ct_kill()?

Right, I missed this as well. The connection killing only removes
the timer, but the packet will still be handled as a valid packet.
So nf_conntrack_core sets the SEEN_REPLY bit, before destroying it
when the final refcount (from the packet) is released. Thanks for
noticing this :)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]