Skip to contentSkip to conversation

Drop expectation refcount after unlinking expectation

PATCH In review Pablo Neira 4 participants 9 messages Aug 5, 2005 – Aug 8, 2005

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira

author
#0 lore raw

PATCH 1/1 Drop expectation refcount after unlinking expectation

This patch comes from the following thread:

[PATCH 6/7] Fix expectation creation

In unlink_expect, the expectation is removed from the list so the refcount must be dropped as well.

Signed-off-by Pablo Neira Ayuso <[email protected]>

This fixes the problem although I think that there's something wrong with current refcounting. Every time an expectation is created, the refcount of the master conntrack is incremented. Then, if the master conntrack is destroyed, say the timeout has expired, ip_ct_remove_expectation will be called. So firstly, all expectations linked to such conntrack are destroyed and then the conntrack itself. OK, my question is: why do we need to increase the master conntrack refcount for expectations, if they are always killed first? Actually I think that the master conntrack refcount should be increased once the expectation is confirmed but not in ip_conntrack_expect_alloc.

net/ipv4/netfilter/ip_conntrack_core.c +1 -0
Index: netfilter-2.6.14/net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
@@ -215,6 +215,7 @@
 	list_del(&exp->list);
 	CONNTRACK_STAT_INC(expect_delete);
 	exp->master->expecting--;
+	ip_conntrack_expect_put(exp);
 }
 
 void __ip_ct_expect_unlink_destroy(struct ip_conntrack_expect *exp)
#1 lore raw
Quoted 6 lines · Pablo Neira

This patch comes from the following thread:

[PATCH 6/7] Fix expectation creation

In unlink_expect, the expectation is removed from the list so the refcount must be dropped as well.

The patch is fine, but it leaves the somewhat broken function __ip_ct_expect_unlink_destroy around. I'm not sure what the intention of this function is, it looks like a lazy way of killing conntracks without manually dropping the timer refcnt. Anyway it invites people to use it in the wrong place and should die IMO.

#2 lore raw
Quoted 8 lines · Patrick McHardy
Quoted 6 lines · Pablo Neira

This patch comes from the following thread:

[PATCH 6/7] Fix expectation creation

In unlink_expect, the expectation is removed from the list so the refcount must be dropped as well.

Quoted 6 lines

The patch is fine, but it leaves the somewhat broken function __ip_ct_expect_unlink_destroy around. I'm not sure what the intention of this function is, it looks like a lazy way of killing conntracks without manually dropping the timer refcnt. Anyway it invites people to use it in the wrong place and should die IMO.

Ok, if it dies we'd have to export unlink_expect() for ctnetlink.

However, all places calling unlink_expect() also call expect_put() immediately after it - with one exception: find_expectation() :(

Getting back to your comment:

Quoted 2 lines

it looks like a lazy way of killing conntracks without manually dropping the timer refcnt.

Sorry, I can't follow you: Did you mean something like 'lazy way of killing expects without manually expiring the timer' ?

So your whole objection is to not have timer_del() inside
__ip_ct_expect_unlink_destroy() ? this can be changed easily.

Signature · 5 lines
- Harald Welte <[email protected]>                 http://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
#3 lore raw
Quoted 25 lines · Harald Welte
Quoted 9 lines · Patrick McHardy
Quoted 6 lines · Pablo Neira

This patch comes from the following thread:

[PATCH 6/7] Fix expectation creation

In unlink_expect, the expectation is removed from the list so the refcount must be dropped as well.

Quoted 6 lines

The patch is fine, but it leaves the somewhat broken function __ip_ct_expect_unlink_destroy around. I'm not sure what the intention of this function is, it looks like a lazy way of killing conntracks without manually dropping the timer refcnt. Anyway it invites people to use it in the wrong place and should die IMO.

Ok, if it dies we'd have to export unlink_expect() for ctnetlink.

However, all places calling unlink_expect() also call expect_put() immediately after it - with one exception: find_expectation() :(

Getting back to your comment:

Quoted 8 lines
Quoted 2 lines

it looks like a lazy way of killing conntracks without manually dropping the timer refcnt.

Sorry, I can't follow you: Did you mean something like 'lazy way of killing expects without manually expiring the timer' ?

So your whole objection is to not have timer_del() inside
__ip_ct_expect_unlink_destroy() ? this can be changed easily.

My objection was the other way around, unlink_expect drops the
reference taken for the lists, as it should. __ip_ct_expect_unlink_destroy() additionally drops a reference which I assume is the timer reference, but it doesn't delete
the timer. If the caller deletes the timer, he should be the
one to drop the reference. Your suggestion is of course perfectly
fine with me, if the timer is deleted within that function it
can also drop the reference.

#4 lore raw
In reply to Pablo Neira
Quoted 10 lines · Pablo Neira

This fixes the problem although I think that there's something wrong with current refcounting. Every time an expectation is created, the refcount of the master conntrack is incremented. Then, if the master conntrack is destroyed, say the timeout has expired, ip_ct_remove_expectation will be called. So firstly, all expectations linked to such conntrack are destroyed and then the conntrack itself. OK, my question is: why do we need to increase the master conntrack refcount for expectations, if they are always killed first? Actually I think that the master conntrack refcount should be increased once the expectation is confirmed but not in ip_conntrack_expect_alloc.

Only outstanding expectations are removed by remove_expectations(). The fullfilled ones need the reference for stuff like ip_nat_follow_master.

#5 lore raw
Quoted 12 lines · Patrick McHardy
Quoted 10 lines · Pablo Neira

This fixes the problem although I think that there's something wrong with current refcounting. Every time an expectation is created, the refcount of the master conntrack is incremented. Then, if the master conntrack is destroyed, say the timeout has expired, ip_ct_remove_expectation will be called. So firstly, all expectations linked to such conntrack are destroyed and then the conntrack itself. OK, my question is: why do we need to increase the master conntrack refcount for expectations, if they are always killed first? Actually I think that the master conntrack refcount should be increased once the expectation is confirmed but not in ip_conntrack_expect_alloc.

Quoted 3 lines

Only outstanding expectations are removed by remove_expectations(). The fullfilled ones need the reference for stuff like ip_nat_follow_master.

So Pablo's argument stil upholds... in theory it would be sufficient to increase the master refcount when expectation is fulfilled(confirmed) instead of allocation time. Or am I missing something?

Signature · 5 lines
- Harald Welte <[email protected]>                 http://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
#6 lore raw
Quoted 9 lines · Harald Welte
Quoted 7 lines · Patrick McHardy
Quoted 4 lines · Pablo Neira

OK, my question is: why do we need to increase the master conntrack refcount for expectations, if they are always killed first? Actually I think that the master conntrack refcount should be increased once the expectation is confirmed but not in ip_conntrack_expect_alloc.

Quoted 7 lines
Quoted 3 lines

Only outstanding expectations are removed by remove_expectations(). The fullfilled ones need the reference for stuff like ip_nat_follow_master.

So Pablo's argument stil upholds... in theory it would be sufficient to increase the master refcount when expectation is fulfilled(confirmed) instead of allocation time. Or am I missing something?

No, it is possible, but we do take a reference in expect_alloc, and I don't see the point of obscuring the reference counting by increasing the count at a later point in time.

#7 lore raw

From: Patrick McHardy <[email protected]>
Date: Sat, 06 Aug 2005 16:24:32 +0200

Quoted 14 lines
Quoted 13 lines · Harald Welte
Quoted 10 lines · Patrick McHardy
Quoted 4 lines · Pablo Neira

OK, my question is: why do we need to increase the master conntrack refcount for expectations, if they are always killed first? Actually I think that the master conntrack refcount should be increased once the expectation is confirmed but not in ip_conntrack_expect_alloc.

Only outstanding expectations are removed by remove_expectations(). The fullfilled ones need the reference for stuff like ip_nat_follow_master.

Quoted 7 lines
Quoted 3 lines

So Pablo's argument stil upholds... in theory it would be sufficient to increase the master refcount when expectation is fulfilled(confirmed) instead of allocation time. Or am I missing something?

No, it is possible, but we do take a reference in expect_alloc, and I don't see the point of obscuring the reference counting by increasing the count at a later point in time.

This refcounts handling has other issue of deadlock. ip_conntrack_expect_put() is called while holding nf_conntrack_lock by ip_conntrack_expect_related() and ip_conntrack_helper_unregister(). In logical, this may result in deadlock in destroy_conntrack() destroying master of expectation.

I cannot find the case that refcounts of conntrack become 0 by ip_conntrack_expect_put() because expectations seems to be destroyed earlier than conntrack. But Pablo and Harald idea will solve this issue if deadlock is really possible.

-----------------------------------------------------------------
Yasuyuki Kozakai @ USAGI Project <[email protected]>

#8 lore raw
In reply to Pablo Neira
Quoted 6 lines · Pablo Neira

This patch comes from the following thread:

[PATCH 6/7] Fix expectation creation

In unlink_expect, the expectation is removed from the list so the refcount must be dropped as well.

I'm applying this to netfilter-2.6.14 right now.

Any cleanups (as Patrick suggested) can follow later.

Signature · 5 lines
- Harald Welte <[email protected]>                 http://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