Date: Sat, 17 Jan 2009 16:13:03 GMT From: Dmitrij Tejblum <tejblum@yandex-team.ru> To: freebsd-gnats-submit@FreeBSD.org Subject: kern/130652: Possible deadlock in rt_check() (sys/net/route.c) Message-ID: <200901171613.n0HGD3Qj009412@www.freebsd.org> Resent-Message-ID: <200901171620.n0HGK3Ta079790@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 130652
>Category: kern
>Synopsis: Possible deadlock in rt_check() (sys/net/route.c)
>Confidential: no
>Severity: serious
>Priority: high
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Sat Jan 17 16:20:03 UTC 2009
>Closed-Date:
>Last-Modified:
>Originator: Dmitrij Tejblum
>Release: 7.1-STABLE
>Organization:
OOO Yandex
>Environment:
FreeBSD 7.1-STABLE; net/route.c 1.120.2.7
>Description:
Some excerpt from rt_check():
rt_check()
{
/*1*/ RT_LOCK(rt0);
retry:
...
if ((rt = rt0->rt_gwroute) != NULL) {
/*2*/ RT_LOCK(rt); /* NB: gwroute */
....
}
if (rt == NULL) { /* NOT AN ELSE CLAUSE */
/*3*/ RT_TEMP_UNLOCK(rt0); /* MUST return to undo this */
/*4*/ rt = rtalloc1_fib(rt0->rt_gateway, 1, 0UL, fibnum);
....
/*5*/ RT_RELOCK(rt0);
....
rt0->rt_gwroute = rt;
}
RT_LOCK_ASSERT(rt);
RT_UNLOCK(rt0);
....
}
The function deals with route rt0 and rt. Usually, it locks rt0 in point /*1*/, then locks rt = rt0->rt_gwroute in point /*2*/, then unlock rt0 and done. But sometimes, in lock rt inside rtalloc1_fib() in point /*4*/. Then, in point /*5*/, it locks rt0, which was unlocked in point /*3*/. The order of locking of rt0 and rt is reversed, so a deadlock is possible.
(Also, if after RT_RELOCK(rt0) we found that rt0 is unusable, we should not forget to free rt before retry.)
>How-To-Repeat:
>Fix:
Patch attached with submission follows:
--- net/route.c 2008-12-05 20:40:46.000000000 +0300
+++ net/route.c 2009-01-17 18:59:12.000000000 +0300
@@ -1634,24 +1634,31 @@ retry:
* Relock it and lose the added reference.
* All sorts of things could have happenned while we
* had no lock on it, so check for them.
+ * rt need to be unlocked to avoid possible deadlock.
*/
+ RT_UNLOCK(rt);
RT_RELOCK(rt0);
- if (rt0 == NULL || ((rt0->rt_flags & RTF_UP) == 0))
+ if (rt0 == NULL || ((rt0->rt_flags & RTF_UP) == 0)) {
/* Ru-roh.. what we had is no longer any good */
+ RTFREE(rt);
goto retry;
+ }
/*
* While we were away, someone replaced the gateway.
* Since a reference count is involved we can't just
* overwrite it.
*/
if (rt0->rt_gwroute) {
- if (rt0->rt_gwroute != rt) {
- RTFREE_LOCKED(rt);
- goto retry;
- }
+ if (rt0->rt_gwroute != rt)
+ RTFREE(rt);
} else {
rt0->rt_gwroute = rt;
}
+ /*
+ * Since rt was not locked, we need recheck that
+ * it still may be used (e.g. up)
+ */
+ goto retry;
}
RT_LOCK_ASSERT(rt);
RT_UNLOCK(rt0);
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200901171613.n0HGD3Qj009412>