Date: Sat, 13 Sep 2008 22:56:02 +0300 From: Giorgos Keramidas <keramida@freebsd.org> To: Julian Elischer <julian@elischer.org> Cc: freebsd-current@freebsd.org, Robert Watson <rwatson@freebsd.org> Subject: Re: panic in rt_check_fib() Message-ID: <87zlmbstv1.fsf@kobe.laptop> In-Reply-To: <874p4ju8t3.fsf@kobe.laptop> (Giorgos Keramidas's message of "Sat, 13 Sep 2008 22:47:52 %2B0300") References: <87prnjh80z.fsf@kobe.laptop> <alpine.BSF.1.10.0809131105280.55411@fledge.watson.org> <48CC14AD.4090708@elischer.org> <874p4ju8t3.fsf@kobe.laptop>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-=-=
Content-Transfer-Encoding: quoted-printable
On Sat, 13 Sep 2008 22:47:52 +0300, Giorgos Keramidas <keramida@freebsd.org=
> wrote:
> +#define RT_RELOCK(_rt) do { \
> + RT_LOCK(_rt) \
> + if ((_rt)->rt_refcnt <=3D 1) \
> + rtfree(_rt); \
> + _rt =3D 0; /* signal that it went away */ \
> + else { \
> + RT_REMREF(_rt); \
> + /* note that _rt is still valid */ \
> + } \
> +} while (0)
> +
This macro needs a semicolon after Rt_LOCK(_rt) and a pair of { ... } in
the if part too:
%%%
#define RT_RELOCK(_rt) do { \
RT_LOCK(_rt); \
if ((_rt)->rt_refcnt <=3D 1) { \
rtfree(_rt); \
_rt =3D 0; /* signal that it went away */ \
} else { \
RT_REMREF(_rt); \
/* note that _rt is still valid */ \
} \
} while (0)
%%%
and the `error' variable is unused in the new rt_check_fib(), or it
breaks the kernel with a warning about an unused variable:
/usr/src/sys/net/route.c: In function 'rt_check_fib':
/usr/src/sys/net/route.c:1716: warning: unused variable 'error'
The updated patch that I'm build-testing now is:
%%%
diff -r ef8e7f2fc284 sys/net/route.c
=2D-- a/sys/net/route.c Fri Sep 12 02:12:33 2008 +0300
+++ b/sys/net/route.c Sat Sep 13 22:55:45 2008 +0300
@@ -1676,19 +1676,31 @@
* *lrt0 points to the cached route to the final destination;
* *lrt is not meaningful;
* fibnum is the index to the correct network fib for this packet
+ * (*lrt0 has not ref held on it so REMREF is not needed )
*
* =3D=3D=3D Operation =3D=3D=3D
* If the route is marked down try to find a new route. If the route
* to the gateway is gone, try to setup a new route. Otherwise,
* if the route is marked for packets to be rejected, enforce that.
+ * Note that rtalloc returns an rtentry with an extra REF that we need to =
lose.
*
* =3D=3D=3D On return =3D=3D=3D
* *dst is unchanged;
* *lrt0 points to the (possibly new) route to the final destination
=2D * *lrt points to the route to the next hop
+ * *lrt points to the route to the next hop [LOCKED]
*
* Their values are meaningful ONLY if no error is returned.
+ *
+ * To follow this you have to remember that:
+ * RT_REMREF reduces the reference count by 1 but doesn't check it for 0 (=
!)
+ * RTFREE_LOCKED includes an RT_REMREF (or an rtfree if refs =3D=3D 1)
+ * and an RT_UNLOCK
+ * RTFREE does an RT_LOCK and an RTFREE_LOCKED
+ * The gwroute pointer counts as a reference on the rtentry to which it po=
ints.
+ * so when we add it we use the ref that rtalloc gives us and when we lose=
it
+ * we need to remove the reference.
*/
+
int
rt_check(struct rtentry **lrt, struct rtentry **lrt0, struct sockaddr *dst)
{
@@ -1701,58 +1713,81 @@
{
struct rtentry *rt;
struct rtentry *rt0;
=2D int error;
=20
KASSERT(*lrt0 !=3D NULL, ("rt_check"));
=2D rt =3D rt0 =3D *lrt0;
+ rt0 =3D *lrt0;
+ rt =3D NULL;
=20
/* NB: the locking here is tortuous... */
=2D RT_LOCK(rt);
=2D if ((rt->rt_flags & RTF_UP) =3D=3D 0) {
=2D RT_UNLOCK(rt);
=2D rt =3D rtalloc1_fib(dst, 1, 0UL, fibnum);
=2D if (rt !=3D NULL) {
=2D RT_REMREF(rt);
=2D /* XXX what about if change? */
=2D } else
+ RT_LOCK(rt0);
+retry:
+ if (rt0 && (rt0->rt_flags & RTF_UP) =3D=3D 0) {
+ /* Current rt0 is useless, try get a replacement. */
+ RT_UNLOCK(rt0);
+ rt0 =3D NULL;
+ }
+ if (rt0 =3D=3D NULL) {
+ rt0 =3D rtalloc1_fib(dst, 1, 0UL, fibnum);
+ if (rt0 =3D=3D NULL) {
return (EHOSTUNREACH);
=2D rt0 =3D rt;
+ }
+ RT_REMREF(rt0); /* don't need the reference. */
}
=2D /* XXX BSD/OS checks dst->sa_family !=3D AF_NS */
=2D if (rt->rt_flags & RTF_GATEWAY) {
=2D if (rt->rt_gwroute =3D=3D NULL)
=2D goto lookup;
=2D rt =3D rt->rt_gwroute;
=2D RT_LOCK(rt); /* NB: gwroute */
=2D if ((rt->rt_flags & RTF_UP) =3D=3D 0) {
=2D RTFREE_LOCKED(rt); /* unlock gwroute */
=2D rt =3D rt0;
=2D rt0->rt_gwroute =3D NULL;
=2D lookup:
=2D RT_UNLOCK(rt0);
=2D/* XXX MRT link level looked up in table 0 */
=2D rt =3D rtalloc1_fib(rt->rt_gateway, 1, 0UL, 0);
=2D if (rt =3D=3D rt0) {
=2D RT_REMREF(rt0);
=2D RT_UNLOCK(rt0);
+
+ if (rt0->rt_flags & RTF_GATEWAY) {
+ if ((rt =3D rt0->rt_gwroute) !=3D NULL) {
+ RT_LOCK(rt); /* NB: gwroute */
+ if ((rt->rt_flags & RTF_UP) =3D=3D 0) {
+ /* gw route is dud. ignore/lose it */
+ RTFREE_LOCKED(rt); /* unref (&unlock) gwroute */
+ rt =3D rt0->rt_gwroute =3D NULL;
+ }
+ }
+ if (rt =3D=3D NULL) { /* NOT AN ELSE CLAUSE */
+ RT_TEMP_UNLOCK(rt0); /* MUST return to undo this */
+ rt =3D rtalloc1_fib(rt0->rt_gateway, 1, 0UL, fibnum);
+ if ((rt =3D=3D rt0) || (rt =3D=3D NULL)) {
+ /* the best we can do is not good enough */
+ if (rt) {
+ RT_REMREF(rt); /* assumes ref > 0 */
+ RT_UNLOCK(rt);
+ }
+ RTFREE(rt0); /* lock, unref, (unlock) */
return (ENETUNREACH);
}
=2D RT_LOCK(rt0);
=2D if (rt0->rt_gwroute !=3D NULL)
=2D RTFREE(rt0->rt_gwroute);
=2D rt0->rt_gwroute =3D rt;
=2D if (rt =3D=3D NULL) {
=2D RT_UNLOCK(rt0);
=2D return (EHOSTUNREACH);
+ /*
+ * 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_RELOCK(rt0);
+ if (rt0 =3D=3D NULL || ((rt0->rt_flags & RTF_UP) =3D=3D 0))
+ /* Ru-roh.. what we had is no longer any good */
+ 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 !=3D rt) {
+ RTFREE_LOCKED(rt);
+ goto retry;
+ }
+ } else {
+ rt0->rt_gwroute =3D rt;
}
}
+ RT_LOCK_ASSERT(rt);
RT_UNLOCK(rt0);
+ } else {
+ /* think of rt as having the lock from now on.. */
+ rt =3D rt0;
}
/* XXX why are we inspecting rmx_expire? */
=2D error =3D (rt->rt_flags & RTF_REJECT) &&
=2D (rt->rt_rmx.rmx_expire =3D=3D 0 ||
=2D time_uptime < rt->rt_rmx.rmx_expire);
=2D if (error) {
+ if ((rt->rt_flags & RTF_REJECT) &&
+ (rt->rt_rmx.rmx_expire =3D=3D 0 ||
+ time_uptime < rt->rt_rmx.rmx_expire)) {
RT_UNLOCK(rt);
return (rt =3D=3D rt0 ? EHOSTDOWN : EHOSTUNREACH);
}
diff -r ef8e7f2fc284 sys/net/route.h
=2D-- a/sys/net/route.h Fri Sep 12 02:12:33 2008 +0300
+++ b/sys/net/route.h Sat Sep 13 22:55:45 2008 +0300
@@ -315,6 +315,22 @@
(_rt)->rt_refcnt--; \
} while (0)
=20
+#define RT_TEMP_UNLOCK(_rt) do { \
+ RT_ADDREF(_rt); \
+ RT_UNLOCK(_rt); \
+} while (0)
+
+#define RT_RELOCK(_rt) do { \
+ RT_LOCK(_rt); \
+ if ((_rt)->rt_refcnt <=3D 1) { \
+ rtfree(_rt); \
+ _rt =3D 0; /* signal that it went away */ \
+ } else { \
+ RT_REMREF(_rt); \
+ /* note that _rt is still valid */ \
+ } \
+} while (0)
+
#define RTFREE_LOCKED(_rt) do { \
if ((_rt)->rt_refcnt <=3D 1) \
rtfree(_rt); \
%%%
--=-=-=
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (FreeBSD)
iEYEARECAAYFAkjMGtIACgkQ1g+UGjGGA7beIgCgn+VarskCxZYLPTzbogI3140h
JjYAoJOhhY6T3+LC3soI9UXH8m2fXCrv
=b3Yo
-----END PGP SIGNATURE-----
--=-=-=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?87zlmbstv1.fsf>