mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
       [not found] <20020226.231934.116353439.davem@redhat.com>
@ 2002-02-27 19:04 ` kuznet
  2002-02-27 19:46   ` Raghu Angadi
  0 siblings, 1 reply; 19+ messages in thread
From: kuznet @ 2002-02-27 19:04 UTC (permalink / raw)
  To: David S. Miller; +Cc: raghuangadi, linux-kernel

Hello!

> I think his analysis is alright but he patch is questionable.

Yes. "if (tb) tcp_tw_put(tw)" cannot be right, no doubts.

Seems, it is enough to remove from bind hash _before_ established.

The idea was that bind hash is pure slave of another state, so that
it need not refcounting at all. Note that adding the second increment
does not help: when we verify that leakage (the situation, when
bucket is in bind hash, but has no timer running) is impossible
we immediately arrive to elimination of the refcount.

Raghu, could you check the variant with inverted order of removal?
Do you see holes? From my side... I need to think more. :-)

Alexey

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
  2002-02-27 19:04 ` Fw: memory corruption in tcp bind hash buckets on SMP? kuznet
@ 2002-02-27 19:46   ` Raghu Angadi
  2002-02-27 20:05     ` Raghu Angadi
  2002-02-27 20:08     ` Fw: " kuznet
  0 siblings, 2 replies; 19+ messages in thread
From: Raghu Angadi @ 2002-02-27 19:46 UTC (permalink / raw)
  To: kuznet, David S. Miller; +Cc: raghuangadi, linux-kernel


--- kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> > I think his analysis is alright but he patch is questionable.
> 
> Yes. "if (tb) tcp_tw_put(tw)" cannot be right, no doubts.

In timewait_kill(), tb is set only _after_ it has been removed from the
established (if it exist there). In _hashdance() tw is inserted into
established _before_ it is inserted into bindhash. So the above is one way of
saying do tw_put() when it has been deleted from _both_ the lists.
Also note that patch removes "if (!tw->pprev) return;" thingy. Infact this
return sort of implied you never thought tw could be deleted from ehash and
not from bind hash.

> Seems, it is enough to remove from bind hash _before_ established.
> 
> The idea was that bind hash is pure slave of another state, so that
> it need not refcounting at all. Note that adding the second increment
> does not help: when we verify that leakage (the situation, when
> bucket is in bind hash, but has no timer running) is impossible
> we immediately arrive to elimination of the refcount.
> 
> Raghu, could you check the variant with inverted order of removal?
> Do you see holes? From my side... I need to think more. :-)

Just the inteverted order of removal will fix _this_ perticular case.
But we still end up doing tw_put() in timewait_kill() even though tw is still
in the bind list (just got inserted on the other processor). This seems
conceptually incorrect or confusing. The refcnt increment in tw_hashdance()
is for these two lists and tw_put() in timewait_kill() should correspond to
the deletion from _both_ the lists. 
 
If you want to avoid timewait_kill() getting called twice altogether.. then
its a different issue.

Raghu.

> Alexey


__________________________________________________
Do You Yahoo!?
Yahoo! Greetings - Send FREE e-cards for every occasion!
http://greetings.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
  2002-02-27 19:46   ` Raghu Angadi
@ 2002-02-27 20:05     ` Raghu Angadi
  2002-02-27 20:12       ` kuznet
  2002-02-27 20:08     ` Fw: " kuznet
  1 sibling, 1 reply; 19+ messages in thread
From: Raghu Angadi @ 2002-02-27 20:05 UTC (permalink / raw)
  To: kuznet, David S. Miller; +Cc: raghuangadi, linux-kernel


inverted order of insertion into the lists in tw_hashdance() is probably
cleaner fix than inverted order of removal.. (that is.. if you dont like "if
(tb) put();" or double refcnting :-)

--- Raghu Angadi <raghuangadi@yahoo.com> wrote:
> 
> --- kuznet@ms2.inr.ac.ru wrote:
> > Hello!
> > 
> > > I think his analysis is alright but he patch is questionable.
> > 
> > Yes. "if (tb) tcp_tw_put(tw)" cannot be right, no doubts.
> 
> In timewait_kill(), tb is set only _after_ it has been removed from the
> established (if it exist there). In _hashdance() tw is inserted into
> established _before_ it is inserted into bindhash. So the above is one way
> of
> saying do tw_put() when it has been deleted from _both_ the lists.
> Also note that patch removes "if (!tw->pprev) return;" thingy. Infact this
> return sort of implied you never thought tw could be deleted from ehash and
> not from bind hash.
> 
> > Seems, it is enough to remove from bind hash _before_ established.
> > 
> > The idea was that bind hash is pure slave of another state, so that
> > it need not refcounting at all. Note that adding the second increment
> > does not help: when we verify that leakage (the situation, when
> > bucket is in bind hash, but has no timer running) is impossible
> > we immediately arrive to elimination of the refcount.
> > 
> > Raghu, could you check the variant with inverted order of removal?
> > Do you see holes? From my side... I need to think more. :-)
> 
> Just the inteverted order of removal will fix _this_ perticular case.
> But we still end up doing tw_put() in timewait_kill() even though tw is
> still
> in the bind list (just got inserted on the other processor). This seems
> conceptually incorrect or confusing. The refcnt increment in tw_hashdance()
> is for these two lists and tw_put() in timewait_kill() should correspond to
> the deletion from _both_ the lists. 
>  
> If you want to avoid timewait_kill() getting called twice altogether.. then
> its a different issue.
> 
> Raghu.
> 
> > Alexey
> 
> 
> __________________________________________________
> Do You Yahoo!?
> Yahoo! Greetings - Send FREE e-cards for every occasion!
> http://greetings.yahoo.com


__________________________________________________
Do You Yahoo!?
Yahoo! Greetings - Send FREE e-cards for every occasion!
http://greetings.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
  2002-02-27 19:46   ` Raghu Angadi
  2002-02-27 20:05     ` Raghu Angadi
@ 2002-02-27 20:08     ` kuznet
  2002-02-27 20:26       ` Raghu Angadi
  2002-02-28  5:53       ` Raghu Angadi
  1 sibling, 2 replies; 19+ messages in thread
From: kuznet @ 2002-02-27 20:08 UTC (permalink / raw)
  To: Raghu Angadi; +Cc: davem, raghuangadi, linux-kernel

Hello!

> the deletion from _both_ the lists. 

No, it is only for insertion to established hash table.

References from bind hash are not considered as references.
Look, if socket will sit only in bind table nobody ever will see it.
Where is the the reference? :-) It just must _not_ stay in bind hash,
if no other references remained, that's invariant which we should provide
now. If we will fail, we are in troubles.

So, its absence in bind hash must be guaranteed to the time of destruction.
Look at this from another aspect: imagine you increment refcnt when
adding to binding table. OK. So, what does guarantee that bucket
will not remain in bind hash forever? And "it will not" is equivalent
to "refcnt is not useful".

Anyway, I will think on this at night, I am not ready to tell how to
do this right.


> If you want to avoid timewait_kill() getting called twice altogether.

Sorry, I did not understand what do you mean here. It can be called
twice or three times or more. This is impossible to avoid without adding
spinlock to timewait bucket.

Alexey

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
  2002-02-27 20:05     ` Raghu Angadi
@ 2002-02-27 20:12       ` kuznet
  2002-02-27 20:31         ` Raghu Angadi
  0 siblings, 1 reply; 19+ messages in thread
From: kuznet @ 2002-02-27 20:12 UTC (permalink / raw)
  To: Raghu Angadi; +Cc: davem, raghuangadi, linux-kernel

Hello!

> inverted order of insertion into the lists in tw_hashdance() is probably
> cleaner fix than inverted order of removal.. 

Why are they not equivalent? Good question? :-)

Alexey

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
  2002-02-27 20:08     ` Fw: " kuznet
@ 2002-02-27 20:26       ` Raghu Angadi
  2002-02-28  5:53       ` Raghu Angadi
  1 sibling, 0 replies; 19+ messages in thread
From: Raghu Angadi @ 2002-02-27 20:26 UTC (permalink / raw)
  To: kuznet; +Cc: davem, raghuangadi, linux-kernel


--- kuznet@ms2.inr.ac.ru wrote:
> So, its absence in bind hash must be guaranteed to the time of destruction.
> Look at this from another aspect: imagine you increment refcnt when
> adding to binding table. 

> OK. So, what does guarantee that bucket
> will not remain in bind hash forever? 
ease of finding the bug :). 'cause this would leave tw in the list withought
deleting it.. that would blow tw_bind_bucket_cache pool and that is so much
easier to find. would've been fixed in very early days.. probably in 2.3.0.1
:->. Still not argueing for double refcount. Reversing the removal order
would work too.. now we will have "if (!tw->tb) return;" instead.

> And "it will not" is equivalent
> to "refcnt is not useful".
> 
> Anyway, I will think on this at night, I am not ready to tell how to
> do this right.
> 
> 
> > If you want to avoid timewait_kill() getting called twice altogether.
> 
> Sorry, I did not understand what do you mean here. It can be called
> twice or three times or more. This is impossible to avoid without adding
> spinlock to timewait bucket.

I didn't think you would want to avoid multiple calls to tw_kill() either. 

Raghu.
> Alexey


__________________________________________________
Do You Yahoo!?
Yahoo! Greetings - Send FREE e-cards for every occasion!
http://greetings.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
  2002-02-27 20:12       ` kuznet
@ 2002-02-27 20:31         ` Raghu Angadi
  2002-03-01 19:07           ` kuznet
  0 siblings, 1 reply; 19+ messages in thread
From: Raghu Angadi @ 2002-02-27 20:31 UTC (permalink / raw)
  To: kuznet; +Cc: davem, raghuangadi, linux-kernel


--- kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> > inverted order of insertion into the lists in tw_hashdance() is probably
> > cleaner fix than inverted order of removal.. 
> 
> Why are they not equivalent? Good question? :-)

they are. with "if (!tb->tb) return;" instead of "if (!tb->pprev) return;".
silly me was thinking of literal cut-n-paste invertion of the oder :->

> Alexey


__________________________________________________
Do You Yahoo!?
Yahoo! Greetings - Send FREE e-cards for every occasion!
http://greetings.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
  2002-02-27 20:08     ` Fw: " kuznet
  2002-02-27 20:26       ` Raghu Angadi
@ 2002-02-28  5:53       ` Raghu Angadi
  1 sibling, 0 replies; 19+ messages in thread
From: Raghu Angadi @ 2002-02-28  5:53 UTC (permalink / raw)
  To: kuznet; +Cc: davem, raghuangadi, linux-kernel

Hi,

I would like to bring your attention to 2nd oops posted in the first mail for
this thread (http://www.uwsg.iu.edu/hypermail/linux/kernel/0202.1/1193.html).
Initially I thought both might be same because they were both releated bind
hash lists. But I have to began suspect it. The first oops is because of race
caused by tw structures left out in the list.. for which a patch is being
discussed here. 

The second one seems to be purely because of a corrupted tb
(tcp_bind_hashbucket).. there is no race there.. tb just got pointers into
arbitary place. Also we saw second oops sometimes with relatively less load.
These are two locations it we have seen (with different bad values for
affending pointers each time it occured):

net/ipv4/tcp_ipv4.c:tcp_v4_get_port(): 

                        spin_lock(&head->lock);
                        for (tb = head->chain; tb; tb = tb->next)
1======>                         if (tb->port == rover)
/*tb is a bad pointer, not NULL */        goto next;
                        break;
                next:
                        spin_unlock(&head->lock);
	            /*
			 more stuff removed
                    */
                         struct sock *sk2 = tb->owners; 
                         int sk_reuse = sk->reuse; 
                         for( ; sk2 != NULL; sk2 = sk2->bind_next) { 
                              if (sk != sk2 && 
2======>                           sk->bound_dev_if == sk2->bound_dev_if) { 
/* sk2 (tb->owners) is a bad pointer */

I will think more about if the problem that causes 1st oops can cause this as
well.

Thanks,
Raghu.

--- kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> > the deletion from _both_ the lists. 
> 
> No, it is only for insertion to established hash table.
> 
> References from bind hash are not considered as references.
> Look, if socket will sit only in bind table nobody ever will see it.
> Where is the the reference? :-) It just must _not_ stay in bind hash,
> if no other references remained, that's invariant which we should provide
> now. If we will fail, we are in troubles.
> 
> So, its absence in bind hash must be guaranteed to the time of destruction.
> Look at this from another aspect: imagine you increment refcnt when
> adding to binding table. OK. So, what does guarantee that bucket
> will not remain in bind hash forever? And "it will not" is equivalent
> to "refcnt is not useful".
> 
> Anyway, I will think on this at night, I am not ready to tell how to
> do this right.
> 
> 
> > If you want to avoid timewait_kill() getting called twice altogether.
> 
> Sorry, I did not understand what do you mean here. It can be called
> twice or three times or more. This is impossible to avoid without adding
> spinlock to timewait bucket.
> 
> Alexey


__________________________________________________
Do You Yahoo!?
Yahoo! Greetings - Send FREE e-cards for every occasion!
http://greetings.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
  2002-02-27 20:31         ` Raghu Angadi
@ 2002-03-01 19:07           ` kuznet
  2002-03-04 20:48             ` Raghu Angadi
  2002-03-04 23:26             ` David S. Miller
  0 siblings, 2 replies; 19+ messages in thread
From: kuznet @ 2002-03-01 19:07 UTC (permalink / raw)
  To: Raghu Angadi; +Cc: davem, raghuangadi, linux-kernel

Hello!

> > > inverted order of insertion into the lists in tw_hashdance() is probably
> > > cleaner fix than inverted order of removal.. 
> > 
> > Why are they not equivalent? Good question? :-)
> 
> they are. with "if (!tb->tb) return;" instead of "if (!tb->pprev) return;".
> silly me was thinking of literal cut-n-paste invertion of the oder :->

However, you were 100% right. They are really not equivalent. :-)

Right solution is to exchange order of insertion. tcp_timewait_kill()
is right and need not changes.


Proof follows:

Main invariant: that guy who inserts/removes socket to/from established
hash table, must make this for binding table. That guy who did not find
socket in established hash, must not touch binding table.

In this case concurrent tcp_timewait_kill()s are happy: socket will
be removed from binding table once and only once, when it is removed
from established hash. The second tcp_timewait_kill() does not find
socket in established table and it must _not_ touch binding table,
despite of socket can be there at the moment.

The mess happens while concurrent remove and insert: insert adds
socket to established table and then to binding table. Racing remove
removes it from established table, but cannot satisfy invariant
because socket is still not in binding table. (This place should
be asserted with a BUG() for future)

The second statement, which completes the proof: removing is possible
only after the socket is added to established hash table (it is evident,
until this time bucket is private to creator).

Alexey

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Fw: memory corruption in tcp bind hash buckets on SMP?
  2002-03-01 19:07           ` kuznet
@ 2002-03-04 20:48             ` Raghu Angadi
  2002-03-04 23:26             ` David S. Miller
  1 sibling, 0 replies; 19+ messages in thread
From: Raghu Angadi @ 2002-03-04 20:48 UTC (permalink / raw)
  To: kuznet; +Cc: davem, raghuangadi, linux-kernel


We have been load testing the kernel with this patch (reverse the insertion
order in __tcp_tw_hashdance(). It seems to work fine till now.

--- kuznet@ms2.inr.ac.ru wrote:
> Hello!
> 
> The mess happens while concurrent remove and insert: insert adds
> socket to established table and then to binding table. Racing remove
> removes it from established table, but cannot satisfy invariant
> because socket is still not in binding table. (This place should
> be asserted with a BUG() for future)

I think we should just remove the conditional for tw->tb in
tcp_timewait_kill(). That way its clear to the reader that we expect tb to be
set by this time. If it is NULL the code will anyway oops there.. we dont
need BUG().

Raghu.
 
> The second statement, which completes the proof: removing is possible
> only after the socket is added to established hash table (it is evident,
> until this time bucket is private to creator).
> 
> Alexey


__________________________________________________
Do You Yahoo!?
Yahoo! Sports - sign up for Fantasy Baseball
http://sports.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: memory corruption in tcp bind hash buckets on SMP?
  2002-03-01 19:07           ` kuznet
  2002-03-04 20:48             ` Raghu Angadi
@ 2002-03-04 23:26             ` David S. Miller
  2002-03-05  0:54               ` Raghu Angadi
  2002-03-05  4:30               ` David S. Miller
  1 sibling, 2 replies; 19+ messages in thread
From: David S. Miller @ 2002-03-04 23:26 UTC (permalink / raw)
  To: raghuangadi; +Cc: kuznet, linux-kernel

   From: Raghu Angadi <raghuangadi@yahoo.com>
   Date: Mon, 4 Mar 2002 12:48:32 -0800 (PST)

   We have been load testing the kernel with this patch (reverse the insertion
   order in __tcp_tw_hashdance(). It seems to work fine till now.

Where was "this patch" posted?  I never saw anyone post
any code, all anyone did was merely describe the change :)

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: memory corruption in tcp bind hash buckets on SMP?
  2002-03-04 23:26             ` David S. Miller
@ 2002-03-05  0:54               ` Raghu Angadi
  2002-03-05  4:30               ` David S. Miller
  1 sibling, 0 replies; 19+ messages in thread
From: Raghu Angadi @ 2002-03-05  0:54 UTC (permalink / raw)
  To: David S. Miller; +Cc: kuznet, linux-kernel


1st patch: reverses the insertion order in __tcp_tw_hashdance(). This is what
we tested

2nd patch: removes conditional around tw->tb in tcp_timewait_kill(). I have
only made sure it compiles. "tw-tb == NULL;" is not strictly required.. but
esures oops if we end up in similar race again.

patch 1:
--------

--- linux-2.4.7-10/net/ipv4/tcp_minisocks.c	Thu Sep  6 13:11:49 2001
+++ linux-2.4.7-10-mod/net/ipv4/tcp_minisocks.c	Fri Mar  1 15:41:26 2002
@@ -304,9 +304,25 @@
 	struct tcp_bind_hashbucket *bhead;
 	struct sock **head, *sktw;
 
+
+	/* Step 1: Put TW into bind hash. Original socket stays there too.
+	   Note, that any socket with sk->num!=0 MUST be bound in binding
+	   cache, even if it is closed.
+	 */
+	bhead = &tcp_bhash[tcp_bhashfn(sk->num)];
+	spin_lock(&bhead->lock);
+	tw->tb = (struct tcp_bind_bucket *)sk->prev;
+	BUG_TRAP(sk->prev!=NULL);
+	if ((tw->bind_next = tw->tb->owners) != NULL)
+		tw->tb->owners->bind_pprev = &tw->bind_next;
+	tw->tb->owners = (struct sock*)tw;
+	tw->bind_pprev = &tw->tb->owners;
+	spin_unlock(&bhead->lock);
+
 	write_lock(&ehead->lock);
 
-	/* Step 1: Remove SK from established hash. */
+	/* Step 2: Remove SK from established hash. */
 	if (sk->pprev) {
 		if(sk->next)
 			sk->next->pprev = sk->pprev;
@@ -315,7 +331,7 @@
 		sock_prot_dec_use(sk->prot);
 	}
 
-	/* Step 2: Hash TW into TIMEWAIT half of established hash table. */
+	/* Step 3: Hash TW into TIMEWAIT half of established hash table. */
 	head = &(ehead + tcp_ehash_size)->chain;
 	sktw = (struct sock *)tw;
 	if((sktw->next = *head) != NULL)
@@ -325,20 +341,6 @@
 	atomic_inc(&tw->refcnt);
 
 	write_unlock(&ehead->lock);
-
-	/* Step 3: Put TW into bind hash. Original socket stays there too.
-	   Note, that any socket with sk->num!=0 MUST be bound in binding
-	   cache, even if it is closed.
-	 */
-	bhead = &tcp_bhash[tcp_bhashfn(sk->num)];
-	spin_lock(&bhead->lock);
-	tw->tb = (struct tcp_bind_bucket *)sk->prev;
-	BUG_TRAP(sk->prev!=NULL);
-	if ((tw->bind_next = tw->tb->owners) != NULL)
-		tw->tb->owners->bind_pprev = &tw->bind_next;
-	tw->tb->owners = (struct sock*)tw;
-	tw->bind_pprev = &tw->tb->owners;
-	spin_unlock(&bhead->lock);
 }
 
 /* 


patch 2:
--------

--- linux-2.4.7-10/net/ipv4/tcp_minisocks.c	Thu Sep  6 13:11:49 2001
+++ linux-2.4.7-10-mod/net/ipv4/tcp_minisocks.c	Mon Mar  4 16:41:35 2002
@@ -75,17 +75,16 @@
 	/* Disassociate with bind bucket. */
 	bhead = &tcp_bhash[tcp_bhashfn(tw->num)];
 	spin_lock(&bhead->lock);
-	if ((tb = tw->tb) != NULL) {
-		if(tw->bind_next)
-			tw->bind_next->bind_pprev = tw->bind_pprev;
-		*(tw->bind_pprev) = tw->bind_next;
-		tw->tb = NULL;
-		if (tb->owners == NULL) {
-			if (tb->next)
-				tb->next->pprev = tb->pprev;
-			*(tb->pprev) = tb->next;
-			kmem_cache_free(tcp_bucket_cachep, tb);
-		}
+	tb = tw->tb;
+	if(tw->bind_next)
+		tw->bind_next->bind_pprev = tw->bind_pprev;
+	*(tw->bind_pprev) = tw->bind_next;
+	tw->tb = NULL;
+	if (tb->owners == NULL) {
+		if (tb->next)
+			tb->next->pprev = tb->pprev;
+		*(tb->pprev) = tb->next;
+		kmem_cache_free(tcp_bucket_cachep, tb);
 	}
 	spin_unlock(&bhead->lock);


--- "David S. Miller" <davem@redhat.com> wrote:
>    From: Raghu Angadi <raghuangadi@yahoo.com>
>    Date: Mon, 4 Mar 2002 12:48:32 -0800 (PST)
> 
>    We have been load testing the kernel with this patch (reverse the
> insertion
>    order in __tcp_tw_hashdance(). It seems to work fine till now.
> 
> Where was "this patch" posted?  I never saw anyone post
> any code, all anyone did was merely describe the change :)


__________________________________________________
Do You Yahoo!?
Yahoo! Sports - sign up for Fantasy Baseball
http://sports.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: memory corruption in tcp bind hash buckets on SMP?
  2002-03-04 23:26             ` David S. Miller
  2002-03-05  0:54               ` Raghu Angadi
@ 2002-03-05  4:30               ` David S. Miller
  1 sibling, 0 replies; 19+ messages in thread
From: David S. Miller @ 2002-03-05  4:30 UTC (permalink / raw)
  To: raghuangadi; +Cc: kuznet, linux-kernel


Thanks, I've added both of these patches to my tree(s).

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: memory corruption in tcp bind hash buckets on SMP?
  2002-02-14  3:07   ` David S. Miller
  2002-02-21  4:16     ` Raghu Angadi
@ 2002-02-23  7:24     ` Raghu Angadi
  1 sibling, 0 replies; 19+ messages in thread
From: Raghu Angadi @ 2002-02-23  7:24 UTC (permalink / raw)
  To: David S. Miller; +Cc: linux-kernel


There is a scenario that leaves a tw in bind hash list even after it gets
deallocated: (seems like easy to fix):

There is a fix that went into between 2.4.16 & 17 (this fix made into into
both RH 2.4.9-etc.. _and_ 2.4.7-10). Fix is to do atomic_set(tw->refcnt, 1)
 instead of 0 and do tcp_tw_put(tw) after tcp_tw_schedule() in
tcp_time_wait(). It looks like this may not be complete fix for tw's 
refcnting issues.

Here is how it happens: reference code 2.4.9-21 (applies to 2.4.17 as well):

    Proc 0                                  Proc 1
--------------------------      |  ------------------------

gets ACK for an sk and sk       |  The remote side sent RST immediately
goes into FIN_WAIT2 state.      |  after ACK for our FIN (abortive
So we are in __tcp_hashdance(). |  close). RST was processed on proc 1.
We just did                     |  since this is an RST, we go to
write_unlock(ehead->lock);      |  tcp_timewait_state_process()
                                |  and kill: label in there 
                                | 
                                |  which
                                |  calls tcp_timewait_kill().
                                |  Here, we delete tw from ehash list.
                                |  but "if ((tb = tw->tb) != NULL)" fails
                                |  'cause this has not been inserted 
                                |   on proc 0 yet.
                                |  Now we go ahead and do tcp_tw_put().
                                
So now we schedule_tw() on proc 0 and when the timeout fires, we call
tcp_timewait_kill() again. But now tw->pprev is NULL (because it was deleted
from established hash above by proc 1). So we just return with out deleting
it from the bind hash list!!. A simple fix is to increment refcnt once for
each list addition and decrement it once for each list deletion or decremnt
only only when it is delete from bind hash. of course 'return if tw->pprev ==
NULL' should also be removed. 

we have seen this happening (eg, tw->tb is non null just before kfree(tw)).

Raghu.

--- "David S. Miller" <davem@redhat.com> wrote:
>    From: Raghu Angadi <raghuangadi@yahoo.com>
>    Date: Wed, 13 Feb 2002 16:51:52 -0800 (PST)
>    
>    --- "David S. Miller" <davem@redhat.com> wrote:
>    > 
>    > This bug is fixed in the 2.4.9 Red Hat 7.2 errata kernels.
>    
>    Thanks, Is the following diff the only culprit/fix?
>    
> There are others, seatch for more instances of tcp_tw_get()
> and tcp_tw_put() and things like atomic_set(tw->count, 1);


__________________________________________________
Do You Yahoo!?
Yahoo! Sports - Coverage of the 2002 Olympic Games
http://sports.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: memory corruption in tcp bind hash buckets on SMP?
  2002-02-14  3:07   ` David S. Miller
@ 2002-02-21  4:16     ` Raghu Angadi
  2002-02-23  7:24     ` Raghu Angadi
  1 sibling, 0 replies; 19+ messages in thread
From: Raghu Angadi @ 2002-02-21  4:16 UTC (permalink / raw)
  To: David S. Miller; +Cc: linux-kernel


--- "David S. Miller" <davem@redhat.com> wrote:
>    From: Raghu Angadi <raghuangadi@yahoo.com>
>    Date: Wed, 13 Feb 2002 16:51:52 -0800 (PST)    
>    --- "David S. Miller" <davem@redhat.com> wrote:
>    > 
>    > This bug is fixed in the 2.4.9 Red Hat 7.2 errata kernels.

We tried the latest errata kernel from redhat.com (2.4.9-21). This one still
has the same problem. We saw exactly the same problem at the same place in
the code. I can add the oops here if you like. 

(link to the original mail for this thread:
	http://www.uwsg.iu.edu/hypermail/linux/kernel/0202.1/1193.html)

We are trying to fix this problem but did not have much success yet. I will
happy to provide any more info if somebody needs.

Raghu.
   
>    Thanks, Is the following diff the only culprit/fix?
>    
> There are others, seatch for more instances of tcp_tw_get()
> and tcp_tw_put() and things like atomic_set(tw->count, 1);


__________________________________________________
Do You Yahoo!?
Yahoo! Sports - Coverage of the 2002 Olympic Games
http://sports.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: memory corruption in tcp bind hash buckets on SMP?
  2002-02-13  6:17 ` David S. Miller
  2002-02-14  0:51   ` Raghu Angadi
@ 2002-02-14  3:07   ` David S. Miller
  2002-02-21  4:16     ` Raghu Angadi
  2002-02-23  7:24     ` Raghu Angadi
  1 sibling, 2 replies; 19+ messages in thread
From: David S. Miller @ 2002-02-14  3:07 UTC (permalink / raw)
  To: raghuangadi; +Cc: linux-kernel

   From: Raghu Angadi <raghuangadi@yahoo.com>
   Date: Wed, 13 Feb 2002 16:51:52 -0800 (PST)
   
   --- "David S. Miller" <davem@redhat.com> wrote:
   > 
   > This bug is fixed in the 2.4.9 Red Hat 7.2 errata kernels.
   
   Thanks, Is the following diff the only culprit/fix?
   
There are others, seatch for more instances of tcp_tw_get()
and tcp_tw_put() and things like atomic_set(tw->count, 1);

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: memory corruption in tcp bind hash buckets on SMP?
  2002-02-13  6:17 ` David S. Miller
@ 2002-02-14  0:51   ` Raghu Angadi
  2002-02-14  3:07   ` David S. Miller
  1 sibling, 0 replies; 19+ messages in thread
From: Raghu Angadi @ 2002-02-14  0:51 UTC (permalink / raw)
  To: David S. Miller; +Cc: linux-kernel


--- "David S. Miller" <davem@redhat.com> wrote:
> 
> This bug is fixed in the 2.4.9 Red Hat 7.2 errata kernels.

Thanks, Is the following diff the only culprit/fix?

--- linux-2.4.7-10/net/ipv4/tcp_ipv4.c      Wed Feb 13 16:50:00 2002
+++ linux-2.4.9-21/net/ipv4/tcp_ipv4.c  Thu Jan 17 10:03:28 2002
@@ -1484,11 +1484,11 @@
        if (nsk) {
                if (nsk->state != TCP_TIME_WAIT) {
                        bh_lock_sock(nsk);
                        return nsk;
                }
-               tcp_tw_put((struct tcp_tw_bucket*)sk);
+               tcp_tw_put((struct tcp_tw_bucket*)nsk);
                return NULL;
        }
 

Raghu.

__________________________________________________
Do You Yahoo!?
Send FREE Valentine eCards with Yahoo! Greetings!
http://greetings.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: memory corruption in tcp bind hash buckets on SMP?
  2002-02-13  6:05 Raghu Angadi
@ 2002-02-13  6:17 ` David S. Miller
  2002-02-14  0:51   ` Raghu Angadi
  2002-02-14  3:07   ` David S. Miller
  0 siblings, 2 replies; 19+ messages in thread
From: David S. Miller @ 2002-02-13  6:17 UTC (permalink / raw)
  To: raghuangadi; +Cc: linux-kernel


This bug is fixed in the 2.4.9 Red Hat 7.2 errata kernels.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* memory corruption in tcp bind hash buckets on SMP?
@ 2002-02-13  6:05 Raghu Angadi
  2002-02-13  6:17 ` David S. Miller
  0 siblings, 1 reply; 19+ messages in thread
From: Raghu Angadi @ 2002-02-13  6:05 UTC (permalink / raw)
  To: linux-kernel

Hi,

We are seeing kernel crashes which seem to be caused because of some
of the members in tcp_bind_bucket object get corrupt. These crashes
seem to happen unpredictabily with network activity (need not always
be heavy laod).

Kernel: 2.4.7-10 SMP (RedHat 7.2) on Dual PIIIs. We kind of have to
work with Redhat kernels. Also I searched the list and did not find
any relevant info to think this bug was observed or fixed in later
kernels. Any insights or suggestions as how to fix it are most welcome.
We have seen crashes at two places:

oops 1 (added below) happens here:
---------
  net/ipv4/tcp_minisocks.c:__tcp_tw_hashdance():

        bhead = &tcp_bhash[tcp_bhashfn(sk->num)];
        spin_lock(&bhead->lock);
        tw->tb = (struct tcp_bind_bucket *)sk->prev;
        BUG_TRAP(sk->prev!=NULL);
        if ((tw->bind_next = tw->tb->owners) != NULL)
===>         tw->tb->owners->bind_pprev = &tw->bind_next; 
	   	     ^^^^^^ (owners is NULL _after_ the above if() succedes)
        tw->tb->owners = (struct sock*)tw;
        tw->bind_pprev = &tw->tb->owners;
        spin_unlock(&bhead->lock);

oops 2 (added below) happens here:
----------

  net/ipv4/tcp_ipv4.c:tcp_v4_get_port():

      if (tb != NULL && tb->owners != NULL) {
         if (tb->fastreuse != 0 && sk->reuse != 0 && sk->state != TCP_LISTEN)
{
                goto success;
         } else {
               struct sock *sk2 = tb->owners;
               int sk_reuse = sk->reuse;

               for( ; sk2 != NULL; sk2 = sk2->bind_next) {
                       if (sk != sk2 &&
======>                    sk->bound_dev_if == sk2->bound_dev_if) {
					       ^^^ sk2 is 0x1	
                               if (!sk_reuse   ||
                                   !sk2->reuse ||
                                   sk2->state == TCP_LISTEN) {

OOPS 1:
-------
pde = 00000000
Oops: 0002
CPU:    1
EIP:    0010:[<c01f77db>]
EFLAGS: 00010282
eax: f01edc38  ebx: f01edc20  ecx: f6ebf0d8  edx: 00000000
esi: e8b86a40  edi: f7271ce0  ebp: f6ebf0dc  esp: c5339d1c
ds: 0018  es: 0018  ss: 0018
Process Swapper (pid: 0, stackpage = c5339000)

Stack: f01edc20 e8b86b78 e8b86a40 00000066 c01f7967 e8b86a40 f01edc20
00000000
       e8b86b78 00000000 00000102 00000001 c01eb938 e8b86a40 00000001
9caca04c
       e2fe1ed7 e8b86b78 e8b86a40 ed86f042 c01eef9d e8b86a40 00000005
00001770
       
Call Trace: [<c01f7967>] [<c01eb938>] [<c01eef9d>] [<c01c9c84>] [<c01f5a01>] 
  [<c01f584b>] [<c01f5e3d>] [<c01ddfd0>] [<c01dd9db>] [<c01de1a4>]
[<c01ddfd0>] 
  [<c01d3363>] [<c01ddfd0>] [<c01d339a>] [<c01dde16>] [<c01ddfd0>]
[<c01cd0eb>] 
  [<c011efbb>] [<c0108c1d>] [<c0105410>] [<c0105410>] [<c02285d8>]
[<c0105410>] 
  [<c0105410>] [<c010543d>] [<c01054c2>] [<c011ad76>] [<c011aeeb>] 

Code: 89 42 1c 8b 43 4c 89 58 08 8b 43 4c 83 c0 08 89 43 1c c6 07
  <0> Kernel panic: Aiee, Killing interrupt handler!
In interrupt handler - not syncing

>>EIP; c01f77db <__tcp_tw_hashdance+eb/110> <=====
Trace; c01f7967 <tcp_time_wait+167/270>
Trace; c01eb938 <tcp_ack+138/310>
Trace; c01eef9d <tcp_rcv_state_process+72d/a00>
Trace; c01c9c84 <skb_checksum+54/320>
Trace; c01f5a01 <tcp_v4_do_rcv+131/180>
Trace; c01f584b <tcp_v4_checksum_init+6b/f0>
Trace; c01f5e3d <tcp_v4_rcv+3ed/660>
Trace; c01ddfd0 <ip_rcv_finish+0/220>
Trace; c01dd9db <ip_local_deliver+12b/1c0>
Trace; c01de1a4 <ip_rcv_finish+1d4/220>
Trace; c01ddfd0 <ip_rcv_finish+0/220>
Trace; c01d3363 <nf_hook_slow+d3/180>
Trace; c01ddfd0 <ip_rcv_finish+0/220>
Trace; c01d339a <nf_hook_slow+10a/180>
Trace; c01dde16 <ip_rcv+3a6/3f0>
Trace; c01ddfd0 <ip_rcv_finish+0/220>
Trace; c01cd0eb <net_rx_action+1eb/300>
Trace; c011efbb <do_softirq+7b/e0>
Trace; c0108c1d <do_IRQ+dd/f0>
Trace; c0105410 <default_idle+0/40>
Trace; c0105410 <default_idle+0/40>
Trace; c02285d8 <call_do_IRQ+5/d>
Trace; c0105410 <default_idle+0/40>
Trace; c0105410 <default_idle+0/40>
Trace; c010543d <default_idle+2d/40>
Trace; c01054c2 <cpu_idle+52/70>
Trace; c011ad76 <__call_console_drivers+46/60>
Trace; c011aeeb <call_console_drivers+eb/100>
Code;  c01f77db <__tcp_tw_hashdance+eb/110>
00000000 <_EIP>:
Code;  c01f77db <__tcp_tw_hashdance+eb/110> <=====
   0:   89 42 1c                  mov    %eax,0x1c(%edx)   <=====
Code;  c01f77de <__tcp_tw_hashdance+ee/110>
3:   8b 43 4c                  mov    0x4c(%ebx),%eax
Code;  c01f77e1 <__tcp_tw_hashdance+f1/110>
6:   89 58 08                  mov    %ebx,0x8(%eax)
Code;  c01f77e4 <__tcp_tw_hashdance+f4/110>
9:   8b 43 4c                  mov    0x4c(%ebx),%eax
Code;  c01f77e7 <__tcp_tw_hashdance+f7/110>
c:   83 c0 08                  add    $0x8,%eax
Code;  c01f77ea <__tcp_tw_hashdance+fa/110>
f:   89 43 1c                  mov    %eax,0x1c(%ebx)
Code;  c01f77ed <__tcp_tw_hashdance+fd/110>
12:   c6 07 00                  movb   $0x0,(%edi)

  <0> Kernel panic: Aiee, Killing interrupt handler!

11 warnings and 5 errors issued.  Results may not be reliable.


OOPS 2:
-------
ts-lnx16 login: Unable to handle Kernel Null pointer dereference at virtual
address 0000000d
*pde = 00000000
Oops: 0
CPU: 0
EIP: 0010: [<c01f343d>]
Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00010292
eax: 00000004 ebx: cff945c0 ecx: db635580 edx: 00000001
esi: 00001f90 edi: 00000001 ebp: db635580 esp: d12afeb4
ds: 0018 es: 0018 ss: 0018
Process traffic_manager (pid: 25128, stackpage=d12af000)
Stack: 00000000 c18cfc80 db635580 d12ae000 00000000 00001f90 c01ffc9d
db635580
       00001f90 ffffffea d0c41a64 d12aff00 00000010 bfffe550 c01c617f
d0c41a64
       d12aaf00 00000010 00000000 901f0002 00000000 00000000 00000000
00000000
Call trace:
[<c01ffc9d>][<c01c617f>][<c01ff2f0>][<c01ffaaa>][<c01c5f63>][<c0150905>][<c01c66f5>][<c01c6715>][<c01c6c6c>][<c014c0cd>][<c010716d>]
Code: 8b 42 0c 8b 4c 24 1c 39 41 0c 75 e7 85 ff 74 0e 80 7a 26 00

>>EIP; c01f343d <tcp_v4_get_port+14d/290>   <=====
Trace; c01ffc9d <inet_bind+17d/290>
Trace; c01c617f <sys_bind+4f/70>
Trace; c01ff2f0 <inet_sock_destruct+0/180>
Trace; c01ffaaa <inet_create+23a/260>
Trace; c01c5f63 <sock_create+f3/120>
Trace; c0150905 <dput+35/180>
Trace; c01c66f5 <sys_setsockopt+45/70>
Trace; c01c6715 <sys_setsockopt+65/70>
Trace; c01c6c6c <sys_socketcall+7c/200>
Trace; c014c0cd <sys_fcntl64+8d/a0>
Trace; c010716d <system_call+35/38>
Code;  c01f343d <tcp_v4_get_port+14d/290>
00000000 <_EIP>:
Code;  c01f343d <tcp_v4_get_port+14d/290>   <=====
   0:   8b 42 0c                  mov    0xc(%edx),%eax   <=====
Code;  c01f3440 <tcp_v4_get_port+150/290>
   3:   8b 4c 24 1c               mov    0x1c(%esp,1),%ecx
Code;  c01f3444 <tcp_v4_get_port+154/290>
   7:   39 41 0c                  cmp    %eax,0xc(%ecx)
Code;  c01f3447 <tcp_v4_get_port+157/290>
   a:   75 e7                     jne    fffffff3 <_EIP+0xfffffff3> c01f3430
<tcp_v4_get_port+140/290>
Code;  c01f3449 <tcp_v4_get_port+159/290>
   c:   85 ff                     test   %edi,%edi
Code;  c01f344b <tcp_v4_get_port+15b/290>
   e:   74 0e                     je     1e <_EIP+0x1e> c01f345b
<tcp_v4_get_port+16b/290>
Code;  c01f344d <tcp_v4_get_port+15d/290>
  10:   80 7a 26 00               cmpb   $0x0,0x26(%edx)

 <0>Kernel Panic: Aiee, killing interrupt handler!

Thanks,
Raghu

__________________________________________________
Do You Yahoo!?
Send FREE Valentine eCards with Yahoo! Greetings!
http://greetings.yahoo.com

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2002-03-05  4:33 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20020226.231934.116353439.davem@redhat.com>
2002-02-27 19:04 ` Fw: memory corruption in tcp bind hash buckets on SMP? kuznet
2002-02-27 19:46   ` Raghu Angadi
2002-02-27 20:05     ` Raghu Angadi
2002-02-27 20:12       ` kuznet
2002-02-27 20:31         ` Raghu Angadi
2002-03-01 19:07           ` kuznet
2002-03-04 20:48             ` Raghu Angadi
2002-03-04 23:26             ` David S. Miller
2002-03-05  0:54               ` Raghu Angadi
2002-03-05  4:30               ` David S. Miller
2002-02-27 20:08     ` Fw: " kuznet
2002-02-27 20:26       ` Raghu Angadi
2002-02-28  5:53       ` Raghu Angadi
2002-02-13  6:05 Raghu Angadi
2002-02-13  6:17 ` David S. Miller
2002-02-14  0:51   ` Raghu Angadi
2002-02-14  3:07   ` David S. Miller
2002-02-21  4:16     ` Raghu Angadi
2002-02-23  7:24     ` Raghu Angadi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®