From owner-freebsd-net@FreeBSD.ORG Fri Oct 3 00:00:41 2003 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0255016A4B3 for ; Fri, 3 Oct 2003 00:00:41 -0700 (PDT) Received: from arginine.spc.org (arginine.spc.org [195.206.69.236]) by mx1.FreeBSD.org (Postfix) with ESMTP id 6A75643FFD for ; Fri, 3 Oct 2003 00:00:39 -0700 (PDT) (envelope-from bms@spc.org) Received: from localhost (localhost [127.0.0.1]) by arginine.spc.org (Postfix) with ESMTP id 88387653D2 for ; Fri, 3 Oct 2003 08:00:38 +0100 (BST) Received: from arginine.spc.org ([127.0.0.1]) by localhost (arginine.spc.org [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 91181-05-3 for ; Fri, 3 Oct 2003 08:00:37 +0100 (BST) Received: from saboteur.dek.spc.org (unknown [81.3.72.68]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by arginine.spc.org (Postfix) with ESMTP id 233676538E for ; Fri, 3 Oct 2003 08:00:37 +0100 (BST) Received: by saboteur.dek.spc.org (Postfix, from userid 1001) id 06BF831; Fri, 3 Oct 2003 08:00:31 +0100 (BST) Date: Fri, 3 Oct 2003 08:00:31 +0100 From: Bruce M Simpson To: freebsd-net@freebsd.org Message-ID: <20031003070031.GL5194@saboteur.dek.spc.org> Mail-Followup-To: freebsd-net@freebsd.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="Xm/fll+QQv+hsKip" Content-Disposition: inline Subject: rtsock.c: eliminate masking of gotos, don't abuse M_RTABLE X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Oct 2003 07:00:41 -0000 --Xm/fll+QQv+hsKip Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi, Here's a diff to eliminate the senderr() macro from rtsock.c. This macro is masking goto statements, which is incredibly bad style, and makes it difficult to follow the flow of control in the file. This diff also stops rtsock.c from abusing the M_RTABLE malloc define for routing socket messages, for the sake of being clearer. [I would have liked to keep these separate but I haven't setup a local branch yet.] The size of an rtmsg can vary, but should perhaps be constant -- there are bugs present, as exercised by some code I posted to this list, and to ru@ in private, just over a month or so ago which panics the kernel by passing in a dubiously-formatted PF_ROUTE message. Making rtmsg constant and no longer using the packed sockaddr format would make it a candiate for a zone allocator, and would help eliminate some redundancy between rtsock.c and ifconfig(8)/route(8) in the userland. Comments? BMS --Xm/fll+QQv+hsKip Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="rtmsg-malloc.diff" Index: rtsock.c =================================================================== RCS file: /home/ncvs/src/sys/net/rtsock.c,v retrieving revision 1.89 diff -u -r1.89 rtsock.c --- rtsock.c 5 Mar 2003 19:24:22 -0000 1.89 +++ rtsock.c 3 Oct 2003 06:45:18 -0000 @@ -55,6 +55,14 @@ MALLOC_DEFINE(M_RTABLE, "routetbl", "routing tables"); +MALLOC_DEFINE(M_RTMSG, "rtmsg", "PF_ROUTE message data"); + +#define RTMSG_MALLOC(p, n) \ + ((p) = (struct rt_msghdr *) \ + malloc((unsigned long)(n), M_RTMSG, M_NOWAIT)) +#define RTMSG_FREE(p) \ + free((caddr_t)(p), M_RTMSG) + static struct sockaddr route_dst = { 2, PF_ROUTE, }; static struct sockaddr route_src = { 2, PF_ROUTE, }; static struct sockaddr sa_zero = { sizeof(sa_zero), AF_INET, }; @@ -280,7 +288,6 @@ struct ifnet *ifp = 0; struct ifaddr *ifa = 0; -#define senderr(e) { error = e; goto flush;} if (m == 0 || ((m->m_len < sizeof(long)) && (m = m_pullup(m, sizeof(long))) == 0)) return (ENOBUFS); @@ -290,37 +297,45 @@ if (len < sizeof(*rtm) || len != mtod(m, struct rt_msghdr *)->rtm_msglen) { dst = 0; - senderr(EINVAL); + error = EINVAL; + goto flush; } - R_Malloc(rtm, struct rt_msghdr *, len); + RTMSG_MALLOC(rtm, len); if (rtm == 0) { dst = 0; - senderr(ENOBUFS); + error = ENOBUFS; + goto flush; } m_copydata(m, 0, len, (caddr_t)rtm); if (rtm->rtm_version != RTM_VERSION) { dst = 0; - senderr(EPROTONOSUPPORT); + error = EPROTONOSUPPORT; + goto flush; } rtm->rtm_pid = curproc->p_pid; bzero(&info, sizeof(info)); info.rti_addrs = rtm->rtm_addrs; if (rt_xaddrs((caddr_t)(rtm + 1), len + (caddr_t)rtm, &info)) { dst = 0; - senderr(EINVAL); + error = EINVAL; + goto flush; } info.rti_flags = rtm->rtm_flags; if (dst == 0 || (dst->sa_family >= AF_MAX) - || (gate != 0 && (gate->sa_family >= AF_MAX))) - senderr(EINVAL); + || (gate != 0 && (gate->sa_family >= AF_MAX))) { + error = EINVAL; + goto flush; + } if (genmask) { struct radix_node *t; t = rn_addmask((caddr_t)genmask, 0, 1); if (t && Bcmp((caddr_t *)genmask + 1, (caddr_t *)t->rn_key + 1, *(u_char *)t->rn_key - 1) == 0) genmask = (struct sockaddr *)(t->rn_key); - else - senderr(ENOBUFS); + else { + error = ENOBUFS; + goto flush; + } } /* @@ -328,13 +343,15 @@ * is the only operation the non-superuser is allowed. */ if (rtm->rtm_type != RTM_GET && (error = suser(curthread)) != 0) - senderr(error); + goto flush; switch (rtm->rtm_type) { case RTM_ADD: - if (gate == 0) - senderr(EINVAL); + if (gate == 0) { + error = EINVAL; + goto flush; + } error = rtrequest1(RTM_ADD, &info, &saved_nrt); if (error == 0 && saved_nrt) { rt_setmetrics(rtm->rtm_inits, @@ -359,15 +376,18 @@ case RTM_CHANGE: case RTM_LOCK: if ((rnh = rt_tables[dst->sa_family]) == 0) { - senderr(EAFNOSUPPORT); + error = EAFNOSUPPORT; + goto flush; } RADIX_NODE_HEAD_LOCK(rnh); rt = (struct rtentry *) rnh->rnh_lookup(dst, netmask, rnh); RADIX_NODE_HEAD_UNLOCK(rnh); if (rt != NULL) rt->rt_refcnt++; - else - senderr(ESRCH); + else { + error = ESRCH; + goto flush; + } switch(rtm->rtm_type) { @@ -394,11 +414,13 @@ (struct walkarg *)0); if (len > rtm->rtm_msglen) { struct rt_msghdr *new_rtm; - R_Malloc(new_rtm, struct rt_msghdr *, len); - if (new_rtm == 0) - senderr(ENOBUFS); + RTMSG_MALLOC(new_rtm, len); + if (new_rtm == 0) { + error = ENOBUFS; + goto flush; + } Bcopy(rtm, new_rtm, rtm->rtm_msglen); - Free(rtm); rtm = new_rtm; + RTMSG_FREE(rtm); rtm = new_rtm; } (void)rt_msg2(rtm->rtm_type, &info, (caddr_t)rtm, (struct walkarg *)0); @@ -418,11 +440,11 @@ (ifaaddr != NULL && !sa_equal(ifaaddr, rt->rt_ifa->ifa_addr))) { if ((error = rt_getifa(&info)) != 0) - senderr(error); + goto flush; } if (gate != NULL && (error = rt_setgate(rt, rt_key(rt), gate)) != 0) - senderr(error); + goto flush; if ((ifa = info.rti_ifa) != NULL) { register struct ifaddr *oifa = rt->rt_ifa; if (oifa != ifa) { @@ -453,7 +475,7 @@ break; default: - senderr(EOPNOTSUPP); + error = EOPNOTSUPP; } flush: @@ -473,7 +495,7 @@ if ((so->so_options & SO_USELOOPBACK) == 0) { if (route_cb.any_count <= 1) { if (rtm) - Free(rtm); + RTMSG_FREE(rtm); m_freem(m); return (error); } @@ -487,7 +509,7 @@ m = NULL; } else if (m->m_pkthdr.len > rtm->rtm_msglen) m_adj(m, rtm->rtm_msglen - m->m_pkthdr.len); - Free(rtm); + RTMSG_FREE(rtm); } if (rp) rp->rcb_proto.sp_family = 0; /* Avoid us */ --Xm/fll+QQv+hsKip--