Skip to contentSkip to conversation

"Badness" again

DISCUSSION Herbert Xu 5 participants 6 messages Jan 15, 2005 3 messages not in the archive

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Herbert Xu

author
#3 lore raw
In reply to a message not in the archive
Quoted 2 lines · Jeff Garzik

Blah. Any other suggestions for debugging this thing?

Yes I have a better theory now :)

All your "badness" messages start with a call to udpv6_sendmsg(). That function calls ip6_dst_lookup() to get its dst entry. Note that udpv6_sendmsg() does not hold a lock on the sk at all. However, ip6_dst_lookup() uses __sk_dst_check() which is only safe if you can either guarantee single-threadedness or if you hold sk_dst_lock.

Neither is true here and therefore we may have a situation where the cached dst is released twice. In fact I tracked down the address closest to the "badness" messages and it belongs to one of your domain's name servers. That means the requests were probably made by named, which is multi-threaded.

So please give this patch a spin and see if it makes things any better. I've verified that no callers to ip6_dst_lookup() holds sk_dst_lock so it's safe (but possibly redundant in cases where they hold locks on the sk itself) to use sk_dst_check().

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

===== net/ipv6/ip6_output.c 1.79 vs edited =====

net/ipv6/ip6_output.c +3 -3
@@ -745,7 +745,7 @@
 	if (sk) {
 		struct ipv6_pinfo *np = inet6_sk(sk);
 	
-		*dst = __sk_dst_check(sk, np->dst_cookie);
+		*dst = sk_dst_check(sk, np->dst_cookie);
 		if (*dst) {
 			struct rt6_info *rt = (struct rt6_info*)*dst;
 	

@@ -772,9 +772,9 @@
 			     && (np->daddr_cache == NULL ||
 				 !ipv6_addr_equal(&fl->fl6_dst, np->daddr_cache)))
 			    || (fl->oif && fl->oif != (*dst)->dev->ifindex)) {
+				dst_release(*dst);
 				*dst = NULL;
-			} else
-				dst_hold(*dst);
+			}
 		}
 	}
#4 lore raw
Quoted 19 lines · Herbert Xu
Quoted 1 line · Jeff Garzik

Blah. Any other suggestions for debugging this thing?

Yes I have a better theory now :)

All your "badness" messages start with a call to udpv6_sendmsg(). That function calls ip6_dst_lookup() to get its dst entry. Note that udpv6_sendmsg() does not hold a lock on the sk at all. However, ip6_dst_lookup() uses __sk_dst_check() which is only safe if you can either guarantee single-threadedness or if you hold sk_dst_lock.

Neither is true here and therefore we may have a situation where the cached dst is released twice. In fact I tracked down the address closest to the "badness" messages and it belongs to one of your domain's name servers. That means the requests were probably made by named, which is multi-threaded.

Quoted 4 lines

So please give this patch a spin and see if it makes things any better. I've verified that no callers to ip6_dst_lookup() holds sk_dst_lock so it's safe (but possibly redundant in cases where they hold locks on the sk itself) to use sk_dst_check().

Running with this patch now, we'll see how it goes. Thanks.

FWIW I also see ICMP code paths in the tracebacks (but that may be "second message" noise).

Jeff

maintainer
#6 lore raw
In reply to Herbert Xu
Quoted 5 lines · On Sat, 15 Jan 2005 11:26:38 +1100 Herbert Xu <[email protected]>…

All your "badness" messages start with a call to udpv6_sendmsg(). That function calls ip6_dst_lookup() to get its dst entry. Note that udpv6_sendmsg() does not hold a lock on the sk at all. However, ip6_dst_lookup() uses __sk_dst_check() which is only safe if you can either guarantee single-threadedness or if you hold sk_dst_lock.

Good catch, that looks to be it.

I wouldn't have caught the necessary dst_hold(*dst) removal too, in fact I thought this hunk of your patch was a bug at first :-)

I'll apply this, thanks a lot Herbert.

#7 lore raw

In article <[email protected]> (at Fri, 14 Jan 2005 21:06:51 -0800), "David S. Miller" <[email protected]> says:

Quoted 10 lines
Quoted 5 lines · On Sat, 15 Jan 2005 11:26:38 +1100 Herbert Xu <[email protected]>…

All your "badness" messages start with a call to udpv6_sendmsg(). That function calls ip6_dst_lookup() to get its dst entry. Note that udpv6_sendmsg() does not hold a lock on the sk at all. However, ip6_dst_lookup() uses __sk_dst_check() which is only safe if you can either guarantee single-threadedness or if you hold sk_dst_lock.

Good catch, that looks to be it.

I agree too; this is one of changeset what I was going to send. Thanks.

--yoshfuji

author
#5 lore raw
In reply to Jeff Garzik
Quoted 3 lines · Jeff Garzik

FWIW I also see ICMP code paths in the tracebacks (but that may be "second message" noise).

I haven't figured out how they can occur as a result of the first problem but let's hope so :)

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#8 lore raw
In reply to Herbert Xu
Quoted 5 lines · Herbert Xu

Neither is true here and therefore we may have a situation where the cached dst is released twice. In fact I tracked down the address closest to the "badness" messages and it belongs to one of your domain's name servers. That means the requests were probably made by named, which is multi-threaded.

The machine where I was seeing this (a HT P4) was indeed running named as well. This is the slave server for some zones, and the master server, which had a pretty much identical sw configuration (both Fedora 2, both using 6to4), is a (non-HT) Celeron and I never saw it happening there.

--L