Re: oops in tcp_v4_rcv.

Re: oops in tcp_v4_rcv.

[Manfred Spraul]

[netdev added to cc list]

I think I understand now what causes the crash:
The tcp_ehash assumes that the entries are of the type 'struct inet_sock'.
But the actual entry is of the type tcp_tw_bucket. And 'sk->inet.daddr' 
is not shared between both structures.


 << net/ipv4/tcp_ipv4, line 510:
        /* Must check for a TIME_WAIT'er before going to listener hash. */
        for (sk = (head + tcp_ehash_size)->chain; sk; sk = sk->next)
               if (TCP_IPV4_MATCH(sk, acookie, saddr, daddr, ports, dif))
                    goto hit;
<<
preprocessor output:
<<
for (sk = (head + (tcp_hashinfo.__tcp_ehash_size))->chain; sk; sk = 
sk->next)
      if ((((&((struct inet_sock *)sk)->inet)->daddr == (saddr)) &&
           ((&((struct inet_sock *)sk)->inet)->rcv_saddr == (daddr)) &&
           ((*((__u32 *)&((&((struct inet_sock *)sk)->inet)->dport)))== 
(ports)) &&
           (!((sk)->bound_dev_if) || ((sk)->bound_dev_if == (dif)))))
                       goto hit;
<<


Manfred Spraul wrote:

> Hi,
>
> I'm looking at crashes that occur during network stress testing with 
> the CONFIG_DEBUG_PAGEALLOC from -mm: Pages that are not in use are 
> immediately unmapped from the linear mapping, and thus reading stale 
> pointer causes an immediate oops.
>
> I've now analyzed one crash:
> the oops is in __tcp_v4_lookup_established, in the 2nd look [i.e. 
> looking at TIME_WAIT sockets. Easy to identify due to the access to 
> __tcp_ehash_size].
>
> The entry in the hash table is an tcp_tw_bucket, and that structure is 
> only ~88 bytes long. The oops is caused by an access to objp+0x168, 
> which doesn't exist.

Re: oops in tcp_v4_rcv.

[David S. Miller]

   From: Manfred Spraul <manfred@colorfullife.com>
   Date: Thu, 29 May 2003 03:15:57 +0200

   I think I understand now what causes the crash:
   The tcp_ehash assumes that the entries are of the type 'struct inet_sock'.
   But the actual entry is of the type tcp_tw_bucket. And 'sk->inet.daddr' 
   is not shared between both structures.

Thanks for figuring this out.  Indeed, I had suspected the
sock layout change Arnaldo did early in 2.5.x as the main
possible suspect.

I'll try to fix this.

Re: oops in tcp_v4_rcv.

[Arnaldo Carvalho de Melo]

Em Wed, May 28, 2003 at 06:40:54PM -0700, David S. Miller escreveu:
>    From: Manfred Spraul <manfred@colorfullife.com>
>    Date: Thu, 29 May 2003 03:15:57 +0200
> 
>    I think I understand now what causes the crash:
>    The tcp_ehash assumes that the entries are of the type 'struct inet_sock'.
>    But the actual entry is of the type tcp_tw_bucket. And 'sk->inet.daddr' 
>    is not shared between both structures.
> 
> Thanks for figuring this out.  Indeed, I had suspected the
> sock layout change Arnaldo did early in 2.5.x as the main
> possible suspect.
> 
> I'll try to fix this.

I'm as well looking at this, longstanding bug :-\

- Arnaldo

Re: oops in tcp_v4_rcv.

[David S. Miller]

   From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
   Date: Wed, 28 May 2003 22:50:20 -0300

   Em Wed, May 28, 2003 at 06:40:54PM -0700, David S. Miller escreveu:
   > I'll try to fix this.
   
   I'm as well looking at this, longstanding bug :-\

Here's a patch, should work:

--- include/net/tcp.h.~1~	Wed May 28 18:42:52 2003
+++ include/net/tcp.h	Wed May 28 18:49:47 2003
@@ -208,6 +208,8 @@
 #endif
 };
 
+#define tcptw_sk(__sk)	((struct tcp_tw_bucket *)(__sk))
+
 extern kmem_cache_t *tcp_timewait_cachep;
 
 static inline void tcp_tw_put(struct tcp_tw_bucket *tw)
@@ -246,7 +248,11 @@
 #endif /* __BIG_ENDIAN */
 #define TCP_IPV4_MATCH(__sk, __cookie, __saddr, __daddr, __ports, __dif)\
 	(((*((__u64 *)&(inet_sk(__sk)->daddr)))== (__cookie))	&&	\
-	 ((*((__u32 *)&(inet_sk(__sk)->dport)))== (__ports))   &&	\
+	 ((*((__u32 *)&(inet_sk(__sk)->dport)))== (__ports))	&&	\
+	 (!((__sk)->bound_dev_if) || ((__sk)->bound_dev_if == (__dif))))
+#define TCP_IPV4_TW_MATCH(__sk, __cookie, __saddr, __daddr, __ports, __dif)\
+	(((*((__u64 *)&(tcptw_sk(__sk)->daddr)))== (__cookie))	&&	\
+	 ((*((__u32 *)&(tcptw_sk(__sk)->dport)))== (__ports))	&&	\
 	 (!((__sk)->bound_dev_if) || ((__sk)->bound_dev_if == (__dif))))
 #else /* 32-bit arch */
 #define TCP_V4_ADDR_COOKIE(__name, __saddr, __daddr)
@@ -254,6 +260,11 @@
 	((inet_sk(__sk)->daddr			== (__saddr))	&&	\
 	 (inet_sk(__sk)->rcv_saddr		== (__daddr))	&&	\
 	 ((*((__u32 *)&(inet_sk(__sk)->dport)))== (__ports))	&&	\
+	 (!((__sk)->bound_dev_if) || ((__sk)->bound_dev_if == (__dif))))
+#define TCP_IPV4_TW_MATCH(__sk, __cookie, __saddr, __daddr, __ports, __dif)\
+	((tcptw_sk(__sk)->daddr			== (__saddr))	&&	\
+	 (tcptw_sk(__sk)->rcv_saddr		== (__daddr))	&&	\
+	 ((*((__u32 *)&(tcptw_sk(__sk)->dport)))== (__ports))	&&	\
 	 (!((__sk)->bound_dev_if) || ((__sk)->bound_dev_if == (__dif))))
 #endif /* 64-bit arch */
 
--- net/ipv4/tcp_ipv4.c.~1~	Wed May 28 18:44:59 2003
+++ net/ipv4/tcp_ipv4.c	Wed May 28 18:45:18 2003
@@ -509,7 +509,7 @@
 
 	/* Must check for a TIME_WAIT'er before going to listener hash. */
 	for (sk = (head + tcp_ehash_size)->chain; sk; sk = sk->next)
-		if (TCP_IPV4_MATCH(sk, acookie, saddr, daddr, ports, dif))
+		if (TCP_IPV4_TW_MATCH(sk, acookie, saddr, daddr, ports, dif))
 			goto hit;
 out:
 	read_unlock(&head->lock);
@@ -570,7 +570,7 @@
 	     skp = &sk2->next) {
 		tw = (struct tcp_tw_bucket *)sk2;
 
-		if (TCP_IPV4_MATCH(sk2, acookie, saddr, daddr, ports, dif)) {
+		if (TCP_IPV4_TW_MATCH(sk2, acookie, saddr, daddr, ports, dif)) {
 			struct tcp_opt *tp = tcp_sk(sk);
 
 			/* With PAWS, it is safe from the viewpoint

Re: oops in tcp_v4_rcv.

[Arnaldo Carvalho de Melo]

Em Wed, May 28, 2003 at 06:51:42PM -0700, David S. Miller escreveu:
>    From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
>    Date: Wed, 28 May 2003 22:50:20 -0300
> 
>    Em Wed, May 28, 2003 at 06:40:54PM -0700, David S. Miller escreveu:
>    > I'll try to fix this.
>    
>    I'm as well looking at this, longstanding bug :-\
> 
> Here's a patch, should work:

At first look, agreed. Thanks David for fixing this.

- Arnaldo

Re: oops in tcp_v4_rcv.

[David S. Miller]

   From: Arnaldo Carvalho de Melo <acme@conectiva.com.br>
   Date: Wed, 28 May 2003 23:00:35 -0300

   Em Wed, May 28, 2003 at 06:51:42PM -0700, David S. Miller escreveu:
   > Here's a patch, should work:
   
   At first look, agreed. Thanks David for fixing this.

No problem.

And a big double-thanks for Mandred for tracking this down.
I wish him luck in tracking down the task struct use-after-free that
he's also seeing. :-)