Date: Sun, 22 Dec 2002 21:33:29 +0200 From: Ruslan Ermilov <ru@freebsd.org> To: Garrett Wollman <wollman@freebsd.org>, Julian Elischer <julian@freebsd.org>, ume@freebsd.org Cc: net@freebsd.org Subject: Broken logic in rtrequest1(RTM_DELETE, ..., ret_nrt) Message-ID: <20021222193329.GA37916@sunbay.com>
next in thread | raw e-mail | index | archive | help
--R3G7APHDIzY6R/pk Content-Type: multipart/mixed; boundary="82I3+IH0IqGh5yIs" Content-Disposition: inline --82I3+IH0IqGh5yIs Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi! The attached patch fixes the logic of rtrequest1() when handling RTM_DELETE requests and the caller asked for a copy of the removed entry. Reviewers are highly welcome. This makes the code more natural, similar to the RTM_ADD and RTM_RESOLVE cases (wrt to handling the returned rtentry's refcnt), and reduces some code bloat in applications. It also makes it safe to turn the RTF_UP bit before rt_fixdelete() walk -- that was the start of my looking at this part of code. Julian, keeping RTF_UP during the rt_fixdelete() walk was necessary because we remove any cloned routes. When the last cloned route is getting removed, it calls RTFREE(rt->rt_parent), and is this was a last reference, it would blow our route away. Does this answer your question in route.c,v 1.41 from 05-Mar-97? You were right that this is handled better by holding a reference on the route (by incrementing rt_refcnt). Cheers, --=20 Ruslan Ermilov Sysadmin and DBA, ru@sunbay.com Sunbay Software AG, ru@FreeBSD.org FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age --82I3+IH0IqGh5yIs Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=p Content-Transfer-Encoding: quoted-printable Index: net/route.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /home/ncvs/src/sys/net/route.c,v retrieving revision 1.72 diff -u -p -r1.72 route.c --- net/route.c 18 Dec 2002 11:45:27 -0000 1.72 +++ net/route.c 22 Dec 2002 19:07:24 -0000 @@ -557,6 +557,8 @@ rtrequest1(req, info, ret_nrt) if (rn->rn_flags & (RNF_ACTIVE | RNF_ROOT)) panic ("rtrequest delete"); rt =3D (struct rtentry *)rn; + rt->rt_refcnt++; + rt->rt_flags &=3D ~RTF_UP; =20 /* * Now search what's left of the subtree for any cloned @@ -580,15 +582,6 @@ rtrequest1(req, info, ret_nrt) } =20 /* - * NB: RTF_UP must be set during the search above, - * because we might delete the last ref, causing - * rt to get freed prematurely. - * eh? then why not just add a reference? - * I'm not sure how RTF_UP helps matters. (JRE) - */ - rt->rt_flags &=3D ~RTF_UP; - - /* * give the protocol a chance to keep things in sync. */ if ((ifa =3D rt->rt_ifa) && ifa->ifa_rtrequest) @@ -607,10 +600,8 @@ rtrequest1(req, info, ret_nrt) */ if (ret_nrt) *ret_nrt =3D rt; - else if (rt->rt_refcnt <=3D 0) { - rt->rt_refcnt++; /* make a 1->0 transition */ - rtfree(rt); - } + else + RTFREE(rt); break; =20 case RTM_RESOLVE: Index: net/rtsock.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /home/ncvs/src/sys/net/rtsock.c,v retrieving revision 1.80 diff -u -p -r1.80 rtsock.c --- net/rtsock.c 18 Dec 2002 11:45:27 -0000 1.80 +++ net/rtsock.c 22 Dec 2002 19:07:25 -0000 @@ -356,8 +356,7 @@ route_output(m, so) case RTM_DELETE: error =3D rtrequest1(RTM_DELETE, &info, &saved_nrt); if (error =3D=3D 0) { - if ((rt =3D saved_nrt)) - rt->rt_refcnt++; + rt =3D saved_nrt; goto report; } break; Index: netinet6/in6.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /home/ncvs/src/sys/netinet6/in6.c,v retrieving revision 1.22 diff -u -p -r1.22 in6.c --- netinet6/in6.c 18 Dec 2002 11:46:59 -0000 1.22 +++ netinet6/in6.c 22 Dec 2002 19:07:26 -0000 @@ -197,11 +197,7 @@ in6_ifloop_request(int cmd, struct ifadd if (nrt) { rt_newaddrmsg(cmd, ifa, e, nrt); if (cmd =3D=3D RTM_DELETE) { - if (nrt->rt_refcnt <=3D 0) { - /* XXX: we should free the entry ourselves. */ - nrt->rt_refcnt++; - rtfree(nrt); - } + RTFREE(nrt); } else { /* the cmd must be RTM_ADD here */ nrt->rt_refcnt--; Index: netinet6/nd6_rtr.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /home/ncvs/src/sys/netinet6/nd6_rtr.c,v retrieving revision 1.11 diff -u -p -r1.11 nd6_rtr.c --- netinet6/nd6_rtr.c 19 Apr 2002 04:46:23 -0000 1.11 +++ netinet6/nd6_rtr.c 22 Dec 2002 19:07:26 -0000 @@ -574,14 +574,7 @@ defrouter_delreq(dr, dofree) RTF_GATEWAY, &oldrt); if (oldrt) { nd6_rtmsg(RTM_DELETE, oldrt); - if (oldrt->rt_refcnt <=3D 0) { - /* - * XXX: borrowed from the RTM_DELETE case of - * rtrequest(). - */ - oldrt->rt_refcnt++; - rtfree(oldrt); - } + RTFREE(oldrt); } =20 if (dofree) /* XXX: necessary? */ @@ -1583,13 +1576,8 @@ nd6_prefix_offlink(pr) error)); } =20 - if (rt !=3D NULL) { - if (rt->rt_refcnt <=3D 0) { - /* XXX: we should free the entry ourselves. */ - rt->rt_refcnt++; - rtfree(rt); - } - } + if (rt !=3D NULL) + RTFREE(rt); =20 return(error); } --82I3+IH0IqGh5yIs-- --R3G7APHDIzY6R/pk Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.1 (FreeBSD) iD8DBQE+BhOJUkv4P6juNwoRAtskAJ9uC3DvRHfBJFQ63SMHETe2SDBL2gCfbZVh gNGvCNNdMzJgr70AqTPsirU= =y4+c -----END PGP SIGNATURE----- --R3G7APHDIzY6R/pk-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021222193329.GA37916>