From owner-svn-src-head@FreeBSD.ORG Wed Dec 23 18:53:11 2009 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 46B37106566C; Wed, 23 Dec 2009 18:53:11 +0000 (UTC) (envelope-from luigi@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id 3495D8FC0C; Wed, 23 Dec 2009 18:53:11 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.3/8.14.3) with ESMTP id nBNIrB91006303; Wed, 23 Dec 2009 18:53:11 GMT (envelope-from luigi@svn.freebsd.org) Received: (from luigi@localhost) by svn.freebsd.org (8.14.3/8.14.3/Submit) id nBNIrBXd006301; Wed, 23 Dec 2009 18:53:11 GMT (envelope-from luigi@svn.freebsd.org) Message-Id: <200912231853.nBNIrBXd006301@svn.freebsd.org> From: Luigi Rizzo Date: Wed, 23 Dec 2009 18:53:11 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r200909 - head/sys/netinet/ipfw X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Dec 2009 18:53:11 -0000 Author: luigi Date: Wed Dec 23 18:53:11 2009 New Revision: 200909 URL: http://svn.freebsd.org/changeset/base/200909 Log: mostly style changes, such as removal of trailing whitespace, reformatting to avoid unnecessary line breaks, small block restructuring to avoid unnecessary nesting, replace macros with function calls, etc. As a side effect of code restructuring, this commit fixes one bug: previously, if a realloc() failed, memory was leaked. Now, the realloc is not there anymore, as we first count how much memory we need and then do a single malloc. Modified: head/sys/netinet/ipfw/ip_fw_nat.c Modified: head/sys/netinet/ipfw/ip_fw_nat.c ============================================================================== --- head/sys/netinet/ipfw/ip_fw_nat.c Wed Dec 23 18:42:25 2009 (r200908) +++ head/sys/netinet/ipfw/ip_fw_nat.c Wed Dec 23 18:53:11 2009 (r200909) @@ -58,7 +58,7 @@ __FBSDID("$FreeBSD$"); static VNET_DEFINE(eventhandler_tag, ifaddr_event_tag); #define V_ifaddr_event_tag VNET(ifaddr_event_tag) -static void +static void ifaddr_change(void *arg __unused, struct ifnet *ifp) { struct cfg_nat *ptr; @@ -66,25 +66,25 @@ ifaddr_change(void *arg __unused, struct struct ip_fw_chain *chain; chain = &V_layer3_chain; - IPFW_WLOCK(chain); + IPFW_WLOCK(chain); /* Check every nat entry... */ LIST_FOREACH(ptr, &chain->nat, _next) { /* ...using nic 'ifp->if_xname' as dynamic alias address. */ - if (strncmp(ptr->if_name, ifp->if_xname, IF_NAMESIZE) == 0) { - if_addr_rlock(ifp); - TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) { - if (ifa->ifa_addr == NULL) - continue; - if (ifa->ifa_addr->sa_family != AF_INET) - continue; - ptr->ip = ((struct sockaddr_in *) - (ifa->ifa_addr))->sin_addr; - LibAliasSetAddress(ptr->lib, ptr->ip); - } - if_addr_runlock(ifp); + if (strncmp(ptr->if_name, ifp->if_xname, IF_NAMESIZE) != 0) + continue; + if_addr_rlock(ifp); + TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) { + if (ifa->ifa_addr == NULL) + continue; + if (ifa->ifa_addr->sa_family != AF_INET) + continue; + ptr->ip = ((struct sockaddr_in *) + (ifa->ifa_addr))->sin_addr; + LibAliasSetAddress(ptr->lib, ptr->ip); } + if_addr_runlock(ifp); } - IPFW_WUNLOCK(chain); + IPFW_WUNLOCK(chain); } /* @@ -94,10 +94,12 @@ static void flush_nat_ptrs(struct ip_fw_chain *chain, const int ix) { int i; + ipfw_insn_nat *cmd; IPFW_WLOCK_ASSERT(chain); for (i = 0; i < chain->n_rules; i++) { - ipfw_insn_nat *cmd = (ipfw_insn_nat *)ACTION_PTR(chain->map[i]); + cmd = (ipfw_insn_nat *)ACTION_PTR(chain->map[i]); + /* XXX skip log and the like ? */ if (cmd->o.opcode == O_NAT && cmd->nat != NULL && (ix < 0 || cmd->nat->id == ix)) cmd->nat = NULL; @@ -132,9 +134,9 @@ del_redir_spool_cfg(struct cfg_nat *n, s free(r, M_IPFW); break; default: - printf("unknown redirect mode: %u\n", r->mode); + printf("unknown redirect mode: %u\n", r->mode); /* XXX - panic?!?!? */ - break; + break; } } } @@ -145,7 +147,6 @@ add_redir_spool_cfg(char *buf, struct cf struct cfg_redir *r, *ser_r; struct cfg_spool *s, *ser_s; int cnt, off, i; - char *panic_err; for (cnt = 0, off = 0; cnt < ptr->redir_cnt; cnt++) { ser_r = (struct cfg_redir *)&buf[off]; @@ -168,7 +169,7 @@ add_redir_spool_cfg(char *buf, struct cf remotePortCopy = 0; r->alink[i] = LibAliasRedirectPort(ptr->lib, r->laddr, htons(r->lport + i), r->raddr, - htons(remotePortCopy), r->paddr, + htons(remotePortCopy), r->paddr, htons(r->pport + i), r->proto); if (r->alink[i] == NULL) { r->alink[0] = NULL; @@ -182,30 +183,26 @@ add_redir_spool_cfg(char *buf, struct cf break; default: printf("unknown redirect mode: %u\n", r->mode); - break; + break; + } + /* XXX perhaps return an error instead of panic ? */ + if (r->alink[0] == NULL) + panic("LibAliasRedirect* returned NULL"); + /* LSNAT handling. */ + for (i = 0; i < r->spool_cnt; i++) { + ser_s = (struct cfg_spool *)&buf[off]; + s = malloc(SOF_REDIR, M_IPFW, M_WAITOK | M_ZERO); + memcpy(s, ser_s, SOF_SPOOL); + LibAliasAddServer(ptr->lib, r->alink[0], + s->addr, htons(s->port)); + off += SOF_SPOOL; + /* Hook spool entry. */ + LIST_INSERT_HEAD(&r->spool_chain, s, _next); } - if (r->alink[0] == NULL) { - panic_err = "LibAliasRedirect* returned NULL"; - goto bad; - } else /* LSNAT handling. */ - for (i = 0; i < r->spool_cnt; i++) { - ser_s = (struct cfg_spool *)&buf[off]; - s = malloc(SOF_REDIR, M_IPFW, - M_WAITOK | M_ZERO); - memcpy(s, ser_s, SOF_SPOOL); - LibAliasAddServer(ptr->lib, r->alink[0], - s->addr, htons(s->port)); - off += SOF_SPOOL; - /* Hook spool entry. */ - LIST_INSERT_HEAD(&r->spool_chain, s, _next); - } /* And finally hook this redir entry. */ LIST_INSERT_HEAD(&ptr->redir_chain, r, _next); } return (1); -bad: - /* something really bad happened: panic! */ - panic("%s\n", panic_err); } static int @@ -219,101 +216,84 @@ ipfw_nat(struct ip_fw_args *args, struct ldt = 0; retval = 0; - if ((mcl = m_megapullup(m, m->m_pkthdr.len)) == - NULL) - goto badnat; + mcl = m_megapullup(m, m->m_pkthdr.len); + if (mcl == NULL) { + args->m = NULL; + return (IP_FW_DENY); + } ip = mtod(mcl, struct ip *); if (args->eh == NULL) { ip->ip_len = htons(ip->ip_len); ip->ip_off = htons(ip->ip_off); } - /* + /* * XXX - Libalias checksum offload 'duct tape': - * - * locally generated packets have only - * pseudo-header checksum calculated - * and libalias will screw it[1], so - * mark them for later fix. Moreover - * there are cases when libalias - * modify tcp packet data[2], mark it - * for later fix too. * - * [1] libalias was never meant to run - * in kernel, so it doesn't have any - * knowledge about checksum - * offloading, and it expects a packet - * with a full internet - * checksum. Unfortunately, packets - * generated locally will have just the - * pseudo header calculated, and when - * libalias tries to adjust the - * checksum it will actually screw it. + * locally generated packets have only pseudo-header checksum + * calculated and libalias will break it[1], so mark them for + * later fix. Moreover there are cases when libalias modifies + * tcp packet data[2], mark them for later fix too. + * + * [1] libalias was never meant to run in kernel, so it does + * not have any knowledge about checksum offloading, and + * expects a packet with a full internet checksum. + * Unfortunately, packets generated locally will have just the + * pseudo header calculated, and when libalias tries to adjust + * the checksum it will actually compute a wrong value. * - * [2] when libalias modify tcp's data - * content, full TCP checksum has to - * be recomputed: the problem is that - * libalias doesn't have any idea - * about checksum offloading To - * workaround this, we do not do - * checksumming in LibAlias, but only - * mark the packets in th_x2 field. If - * we receive a marked packet, we - * calculate correct checksum for it - * aware of offloading. Why such a - * terrible hack instead of - * recalculating checksum for each - * packet? Because the previous - * checksum was not checked! - * Recalculating checksums for EVERY - * packet will hide ALL transmission - * errors. Yes, marked packets still - * suffer from this problem. But, - * sigh, natd(8) has this problem, - * too. + * [2] when libalias modifies tcp's data content, full TCP + * checksum has to be recomputed: the problem is that + * libalias does not have any idea about checksum offloading. + * To work around this, we do not do checksumming in LibAlias, + * but only mark the packets in th_x2 field. If we receive a + * marked packet, we calculate correct checksum for it + * aware of offloading. Why such a terrible hack instead of + * recalculating checksum for each packet? + * Because the previous checksum was not checked! + * Recalculating checksums for EVERY packet will hide ALL + * transmission errors. Yes, marked packets still suffer from + * this problem. But, sigh, natd(8) has this problem, too. * * TODO: -make libalias mbuf aware (so * it can handle delayed checksum and tso) */ - if (mcl->m_pkthdr.rcvif == NULL && - mcl->m_pkthdr.csum_flags & - CSUM_DELAY_DATA) + if (mcl->m_pkthdr.rcvif == NULL && + mcl->m_pkthdr.csum_flags & CSUM_DELAY_DATA) ldt = 1; c = mtod(mcl, char *); if (args->oif == NULL) - retval = LibAliasIn(t->lib, c, + retval = LibAliasIn(t->lib, c, mcl->m_len + M_TRAILINGSPACE(mcl)); else - retval = LibAliasOut(t->lib, c, + retval = LibAliasOut(t->lib, c, mcl->m_len + M_TRAILINGSPACE(mcl)); if (retval == PKT_ALIAS_RESPOND) { - m->m_flags |= M_SKIP_FIREWALL; - retval = PKT_ALIAS_OK; + m->m_flags |= M_SKIP_FIREWALL; + retval = PKT_ALIAS_OK; } if (retval != PKT_ALIAS_OK && retval != PKT_ALIAS_FOUND_HEADER_FRAGMENT) { /* XXX - should i add some logging? */ m_free(mcl); - badnat: args->m = NULL; return (IP_FW_DENY); } - mcl->m_pkthdr.len = mcl->m_len = - ntohs(ip->ip_len); + mcl->m_pkthdr.len = mcl->m_len = ntohs(ip->ip_len); - /* - * XXX - libalias checksum offload - * 'duct tape' (see above) + /* + * XXX - libalias checksum offload + * 'duct tape' (see above) */ - if ((ip->ip_off & htons(IP_OFFMASK)) == 0 && + if ((ip->ip_off & htons(IP_OFFMASK)) == 0 && ip->ip_p == IPPROTO_TCP) { - struct tcphdr *th; + struct tcphdr *th; th = (struct tcphdr *)(ip + 1); - if (th->th_x2) + if (th->th_x2) ldt = 1; } @@ -325,38 +305,33 @@ ipfw_nat(struct ip_fw_args *args, struct ip->ip_len = ntohs(ip->ip_len); cksum = in_pseudo( ip->ip_src.s_addr, - ip->ip_dst.s_addr, + ip->ip_dst.s_addr, htons(ip->ip_p + ip->ip_len - (ip->ip_hl << 2)) ); - + switch (ip->ip_p) { case IPPROTO_TCP: th = (struct tcphdr *)(ip + 1); - /* - * Maybe it was set in - * libalias... + /* + * Maybe it was set in + * libalias... */ th->th_x2 = 0; th->th_sum = cksum; - mcl->m_pkthdr.csum_data = + mcl->m_pkthdr.csum_data = offsetof(struct tcphdr, th_sum); break; case IPPROTO_UDP: uh = (struct udphdr *)(ip + 1); uh->uh_sum = cksum; - mcl->m_pkthdr.csum_data = + mcl->m_pkthdr.csum_data = offsetof(struct udphdr, uh_sum); - break; + break; } - /* - * No hw checksum offloading: do it - * by ourself. - */ - if ((mcl->m_pkthdr.csum_flags & - CSUM_DELAY_DATA) == 0) { + /* No hw checksum offloading: do it ourselves */ + if ((mcl->m_pkthdr.csum_flags & CSUM_DELAY_DATA) == 0) { in_delayed_cksum(mcl); - mcl->m_pkthdr.csum_flags &= - ~CSUM_DELAY_DATA; + mcl->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA; } ip->ip_len = htons(ip->ip_len); } @@ -370,24 +345,19 @@ ipfw_nat(struct ip_fw_args *args, struct return (IP_FW_NAT); } -#define LOOKUP_NAT(head, i, p) do { \ - LIST_FOREACH((p), head, _next) { \ - if ((p)->id == (i)) { \ - break; \ - } \ - } \ - } while (0) - static struct cfg_nat * lookup_nat(struct nat_list *l, int nat_id) { struct cfg_nat *res; - LOOKUP_NAT(l, nat_id, res); + LIST_FOREACH(res, l, _next) { + if (res->id == nat_id) + break; + } return res; } -static int +static int ipfw_nat_cfg(struct sockopt *sopt) { struct cfg_nat *ptr, *ser_n; @@ -395,22 +365,21 @@ ipfw_nat_cfg(struct sockopt *sopt) struct ip_fw_chain *chain = &V_layer3_chain; buf = malloc(NAT_BUF_LEN, M_IPFW, M_WAITOK | M_ZERO); - sooptcopyin(sopt, buf, NAT_BUF_LEN, - sizeof(struct cfg_nat)); + sooptcopyin(sopt, buf, NAT_BUF_LEN, sizeof(struct cfg_nat)); ser_n = (struct cfg_nat *)buf; /* check valid parameter ser_n->id > 0 ? */ - /* + /* * Find/create nat rule. */ IPFW_WLOCK(chain); - LOOKUP_NAT(&chain->nat, ser_n->id, ptr); + ptr = lookup_nat(&chain->nat, ser_n->id); if (ptr == NULL) { /* New rule: allocate and init new instance. */ - ptr = malloc(sizeof(struct cfg_nat), + ptr = malloc(sizeof(struct cfg_nat), M_IPFW, M_NOWAIT | M_ZERO); if (ptr == NULL) { - IPFW_WUNLOCK(chain); + IPFW_WUNLOCK(chain); free(buf, M_IPFW); return (ENOSPC); } @@ -429,13 +398,13 @@ ipfw_nat_cfg(struct sockopt *sopt) } IPFW_WUNLOCK(chain); - /* + /* * Basic nat configuration. */ ptr->id = ser_n->id; - /* - * XXX - what if this rule doesn't nat any ip and just - * redirect? + /* + * XXX - what if this rule doesn't nat any ip and just + * redirect? * do we set aliasaddress to 0.0.0.0? */ ptr->ip = ser_n->ip; @@ -445,7 +414,7 @@ ipfw_nat_cfg(struct sockopt *sopt) LibAliasSetAddress(ptr->lib, ptr->ip); memcpy(ptr->if_name, ser_n->if_name, IF_NAMESIZE); - /* + /* * Redir and LSNAT configuration. */ /* Delete old cfgs. */ @@ -465,10 +434,11 @@ ipfw_nat_del(struct sockopt *sopt) struct cfg_nat *ptr; struct ip_fw_chain *chain = &V_layer3_chain; int i; - + sooptcopyin(sopt, &i, sizeof i, sizeof i); + /* XXX validate i */ IPFW_WLOCK(chain); - LOOKUP_NAT(&chain->nat, i, ptr); + ptr = lookup_nat(&chain->nat, i); if (ptr == NULL) { IPFW_WUNLOCK(chain); return (EINVAL); @@ -484,13 +454,14 @@ ipfw_nat_del(struct sockopt *sopt) static int ipfw_nat_get_cfg(struct sockopt *sopt) -{ +{ uint8_t *data; struct cfg_nat *n; struct cfg_redir *r; struct cfg_spool *s; int nat_cnt, off; struct ip_fw_chain *chain; + int err = ENOSPC; chain = &V_layer3_chain; nat_cnt = 0; @@ -501,41 +472,35 @@ ipfw_nat_get_cfg(struct sockopt *sopt) /* Serialize all the data. */ LIST_FOREACH(n, &chain->nat, _next) { nat_cnt++; - if (off + SOF_NAT < NAT_BUF_LEN) { - bcopy(n, &data[off], SOF_NAT); - off += SOF_NAT; - LIST_FOREACH(r, &n->redir_chain, _next) { - if (off + SOF_REDIR < NAT_BUF_LEN) { - bcopy(r, &data[off], - SOF_REDIR); - off += SOF_REDIR; - LIST_FOREACH(s, &r->spool_chain, - _next) { - if (off + SOF_SPOOL < - NAT_BUF_LEN) { - bcopy(s, &data[off], - SOF_SPOOL); - off += SOF_SPOOL; - } else - goto nospace; - } - } else + if (off + SOF_NAT >= NAT_BUF_LEN) + goto nospace; + bcopy(n, &data[off], SOF_NAT); + off += SOF_NAT; + LIST_FOREACH(r, &n->redir_chain, _next) { + if (off + SOF_REDIR >= NAT_BUF_LEN) + goto nospace; + bcopy(r, &data[off], SOF_REDIR); + off += SOF_REDIR; + LIST_FOREACH(s, &r->spool_chain, _next) { + if (off + SOF_SPOOL >= NAT_BUF_LEN) goto nospace; + bcopy(s, &data[off], SOF_SPOOL); + off += SOF_SPOOL; } - } else - goto nospace; + } } - bcopy(&nat_cnt, data, sizeof(nat_cnt)); - IPFW_RUNLOCK(chain); - sooptcopyout(sopt, data, NAT_BUF_LEN); - free(data, M_IPFW); - return (0); + err = 0; /* all good */ nospace: IPFW_RUNLOCK(chain); - printf("serialized data buffer not big enough:" - "please increase NAT_BUF_LEN\n"); + if (err == 0) { + bcopy(&nat_cnt, data, sizeof(nat_cnt)); + sooptcopyout(sopt, data, NAT_BUF_LEN); + } else { + printf("serialized data buffer not big enough:" + "please increase NAT_BUF_LEN\n"); + } free(data, M_IPFW); - return (ENOSPC); + return (err); } static int @@ -543,30 +508,33 @@ ipfw_nat_get_log(struct sockopt *sopt) { uint8_t *data; struct cfg_nat *ptr; - int i, size, cnt, sof; + int i, size; struct ip_fw_chain *chain; chain = &V_layer3_chain; - data = NULL; - sof = LIBALIAS_BUF_SIZE; - cnt = 0; IPFW_RLOCK(chain); - size = i = 0; + /* one pass to count, one to copy the data */ + i = 0; LIST_FOREACH(ptr, &chain->nat, _next) { - if (ptr->lib->logDesc == NULL) + if (ptr->lib->logDesc == NULL) + continue; + i++; + } + size = i * (LIBALIAS_BUF_SIZE + sizeof(int)); + data = malloc(size, M_IPFW, M_NOWAIT | M_ZERO); + if (data == NULL) { + IPFW_RUNLOCK(chain); + return (ENOSPC); + } + i = 0; + LIST_FOREACH(ptr, &chain->nat, _next) { + if (ptr->lib->logDesc == NULL) continue; - cnt++; - size = cnt * (sof + sizeof(int)); - data = realloc(data, size, M_IPFW, M_NOWAIT | M_ZERO); - if (data == NULL) { - IPFW_RUNLOCK(chain); - return (ENOSPC); - } bcopy(&ptr->id, &data[i], sizeof(int)); i += sizeof(int); - bcopy(ptr->lib->logDesc, &data[i], sof); - i += sof; + bcopy(ptr->lib->logDesc, &data[i], LIBALIAS_BUF_SIZE); + i += LIBALIAS_BUF_SIZE; } IPFW_RUNLOCK(chain); sooptcopyout(sopt, data, size); @@ -587,7 +555,8 @@ ipfw_nat_init(void) ipfw_nat_get_cfg_ptr = ipfw_nat_get_cfg; ipfw_nat_get_log_ptr = ipfw_nat_get_log; IPFW_WUNLOCK(&V_layer3_chain); - V_ifaddr_event_tag = EVENTHANDLER_REGISTER(ifaddr_event, ifaddr_change, + V_ifaddr_event_tag = EVENTHANDLER_REGISTER( + ifaddr_event, ifaddr_change, NULL, EVENTHANDLER_PRI_ANY); }