From nobody@FreeBSD.org Sun Feb 7 19:31:52 2010 Return-Path: Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6C160106566B for ; Sun, 7 Feb 2010 19:31:52 +0000 (UTC) (envelope-from nobody@FreeBSD.org) Received: from www.freebsd.org (www.freebsd.org [IPv6:2001:4f8:fff6::21]) by mx1.freebsd.org (Postfix) with ESMTP id 5BD138FC08 for ; Sun, 7 Feb 2010 19:31:52 +0000 (UTC) Received: from www.freebsd.org (localhost [127.0.0.1]) by www.freebsd.org (8.14.3/8.14.3) with ESMTP id o17JVpbl032574 for ; Sun, 7 Feb 2010 19:31:51 GMT (envelope-from nobody@www.freebsd.org) Received: (from nobody@localhost) by www.freebsd.org (8.14.3/8.14.3/Submit) id o17JVp7R032573; Sun, 7 Feb 2010 19:31:51 GMT (envelope-from nobody) Message-Id: <201002071931.o17JVp7R032573@www.freebsd.org> Date: Sun, 7 Feb 2010 19:31:51 GMT From: Dmitriy Demidov To: freebsd-gnats-submit@FreeBSD.org Subject: ipfw nat redirect_port "buf is too small" error X-Send-Pr-Version: www-3.1 X-GNATS-Notify: >Number: 143653 >Category: kern >Synopsis: [ipfw] [patch] ipfw nat redirect_port "buf is too small" error >Confidential: no >Severity: serious >Priority: medium >Responsible: glebius >State: closed >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Sun Feb 07 19:40:02 UTC 2010 >Closed-Date: Fri Jul 08 14:32:20 UTC 2011 >Last-Modified: Fri Jul 8 14:40:00 UTC 2011 >Originator: Dmitriy Demidov >Release: 7.2-STABLE >Organization: >Environment: FreeBSD hius.local.home 7.2-STABLE FreeBSD 7.2-STABLE #0: Sat Aug 15 13:49:47 EEST 2009 terminus@hius.local.home:/usr/obj/usr/src/sys/STABLE i386 >Description: There is one bug with ipfw nat - it can not handle redirect_port configuration what consist of big number of redirect_port rules. For example if I make a try to apply this configuration: ipfw nat 1 config if em0 log deny_in same_ports reset redirect_port tcp 127.0.0.1:28011 28011 redirect_port udp 127.0.0.1:4444 4444 redirect_port tcp 127.0.0.1:6881 6881 redirect_port udp 127.0.0.1:14400 14400 redirect_port tcp 127.0.0.1:14400 14400 redirect_port tcp 127.0.0.1:14401 14401 redirect_port tcp 127.0.0.1:14402 14402 redirect_port tcp 127.0.0.1:14403 14403 redirect_port tcp 127.0.0.1:14404 14404 redirect_port tcp 127.0.0.1:14405 14405 redirect_port tcp 127.0.0.1:14406 14406 redirect_port tcp 127.0.0.1:14407 14407 redirect_port tcp 127.0.0.1:14408 14408 redirect_port tcp 127.0.0.1:14410 14410 redirect_port tcp 127.0.0.1:14411 14411 redirect_port tcp 127.0.0.1:14412 14412 redirect_port tcp 127.0.0.1:14413 14413 redirect_port tcp 127.0.0.1:14414 14414 redirect_port tcp 127.0.0.1:14415 14415 redirect_port tcp 127.0.0.1:14416 14416 redirect_port tcp 127.0.0.1:14417 14417 redirect_port tcp 127.0.0.1:14418 14418 redirect_port tcp 127.0.0.1:14419 14419 redirect_port tcp 12 7.0.0.1:14420 14420 I always got error message: ipfw: redirect_port: buf is too small This problem is observed on FreeBSD 8.0-RELEASE as well. >How-To-Repeat: Try to configure an instance of ipfw nat with big amount of redirect_port directives. >Fix: It is not my solution (I found it in the Net). ==== 1) edit /usr/src/sys/netinet/ip_fw.h #define NAT_BUF_LEN 1024 change this string to something bigger #define NAT_BUF_LEN 11264 2) cd /usr/src/include make install 3) cd /usr/src make buildworld && make buildkernel >Release-Note: >Audit-Trail: Responsible-Changed-From-To: freebsd-bugs->freebsd-ipfw Responsible-Changed-By: linimon Responsible-Changed-When: Mon Feb 8 03:54:50 UTC 2010 Responsible-Changed-Why: Over to maintainer(s). http://www.freebsd.org/cgi/query-pr.cgi?pr=143653 From: Jeff Kletsky To: bug-followup@FreeBSD.org, dima_bsd@inbox.lv Cc: Subject: Re: kern/143653: [ipfw] [patch] ipfw nat redirect_port "buf is too small" error Date: Sun, 27 Feb 2011 09:08:58 -0800 Confirmed to be a problem on RELEASE-8.2 as well. 17 redirect_port lines are too many, reducing to 10 allowed firewall to load. Machine is a firewall and redirects ssh to several internal hosts, as well as other services for several domains. This is *exactly* the kind of host I would like to be able to maintain solely with freebsd-update without having sources and compilers on the filesystem. From: Jeff Kletsky To: bug-followup@FreeBSD.org, dima_bsd@inbox.lv Cc: Subject: Re: kern/143653: [ipfw] [patch] ipfw nat redirect_port "buf is too small" error Date: Sun, 27 Feb 2011 13:01:12 -0800 Under some situations, can cause *kernel panic* with no automatic reboot (just hangs on screen, HW reset required). Additionally, the firewall script being executed has zero-byte length on reboot. In my case, it was 15 redirect rules entered. RELEASE-8.2 amd64 Copying by hand from the screen: unknown redirect mode: 0 panic: LibAliasRedirect* returned NULL cpuid = 0 The stack backtrace includes: kdb_backtrace+0x5e panic+0x187 ipfw_nat_cfg+0x35a ipfw_ctl+0x211 rip_ctloutput+0x9f sosetopt+0x42 kern_setsockopt+0xc0 setsockopt+0x22 syscallenter+0x1e5 syscall+0x4b Xfast_syscall+0xe2 Kernel dump and configuration files now available in http://wildside.wagsky.com/pr143653/ Should be able to replicate by installing RELEASE-8.2, those config files (adjusting as needed for the two network interfaces) and copying twoport.crashes to the target of the twoport symlink. [root@port7 /var/crash]# uname -a FreeBSD port7.pn.wagsky.com 8.2-RELEASE FreeBSD 8.2-RELEASE #0: Thu Feb 17 02:41:51 UTC 2011 root@mason.cse.buffalo.edu:/usr/obj/usr/src/sys/GENERIC amd64 From: Gleb Smirnoff To: bug-followup@FreeBSD.org Cc: Jeff Kletsky , "Alexander V. Chernikov" Subject: kern/143653 Date: Tue, 19 Apr 2011 18:59:07 +0400 --J/dobhs11T7y2rNN Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Here are patches that eliminate NAT_BUF_LEN and make all memory sizes in these paths dynamic. Testing is appreciated. Patches are against head/, where a big whitespace cleanup had been performed, so before applying to stable/8 you may need to merge r220802,r220804:: http://svn.freebsd.org/viewvc/base/head/sbin/ipfw/nat.c?view=log -- Totus tuus, Glebius. --J/dobhs11T7y2rNN Content-Type: text/x-diff; charset=koi8-r Content-Disposition: attachment; filename="143653.nat.c.diff" Index: nat.c =================================================================== --- nat.c (revision 220834) +++ nat.c (working copy) @@ -281,13 +281,6 @@ /* End of stuff taken from natd.c. */ -#define INC_ARGCV() do { \ - (*_av)++; \ - (*_ac)--; \ - av = *_av; \ - ac = *_ac; \ -} while(0) - /* * The next 3 functions add support for the addr, port and proto redirect and * their logic is loosely based on SetupAddressRedirect(), SetupPortRedirect() @@ -318,121 +311,107 @@ */ static int -setup_redir_addr(char *spool_buf, unsigned int len, - int *_ac, char ***_av) +estimate_redir_addr(int *ac, char ***av) { - char **av, *sep; /* Token separator. */ - /* Temporary buffer used to hold server pool ip's. */ - char tmp_spool_buf[NAT_BUF_LEN]; - int ac, space, lsnat; + size_t space = sizeof(struct cfg_redir); + char *sep; + + if ((sep = strtok(**av, ",")) != NULL) { + space += sizeof(struct cfg_spool); + while ((sep = strtok(NULL, ",")) != NULL) + space += sizeof(struct cfg_spool); + } + + return (space); +} + +static int +setup_redir_addr(char *buf, int *ac, char ***av) +{ struct cfg_redir *r; - struct cfg_spool *tmp; + char *sep; + size_t space; - av = *_av; - ac = *_ac; - space = 0; - lsnat = 0; - if (len >= SOF_REDIR) { - r = (struct cfg_redir *)spool_buf; - /* Skip cfg_redir at beginning of buf. */ - spool_buf = &spool_buf[SOF_REDIR]; - space = SOF_REDIR; - len -= SOF_REDIR; - } else - goto nospace; + r = (struct cfg_redir *)buf; r->mode = REDIR_ADDR; + /* Skip cfg_redir at beginning of buf. */ + buf = &buf[sizeof(struct cfg_redir)]; + space = sizeof(struct cfg_redir); + /* Extract local address. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_addr: missing local address"); - sep = strchr(*av, ','); - if (sep) { /* LSNAT redirection syntax. */ + if ((sep = strtok(**av, ",")) != NULL) { + struct cfg_spool *spool; + + /* Setup LSNAT server pool. */ r->laddr.s_addr = INADDR_NONE; - /* Preserve av, copy spool servers to tmp_spool_buf. */ - strncpy(tmp_spool_buf, *av, strlen(*av)+1); - lsnat = 1; - } else - StrToAddr(*av, &r->laddr); - INC_ARGCV(); - - /* Extract public address. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_addr: missing public address"); - StrToAddr(*av, &r->paddr); - INC_ARGCV(); - - /* Setup LSNAT server pool. */ - if (sep) { - sep = strtok(tmp_spool_buf, ","); while (sep != NULL) { - tmp = (struct cfg_spool *)spool_buf; - if (len < SOF_SPOOL) - goto nospace; - len -= SOF_SPOOL; - space += SOF_SPOOL; - StrToAddr(sep, &tmp->addr); - tmp->port = ~0; + spool = (struct cfg_spool *)buf; + space += sizeof(struct cfg_spool); + StrToAddr(sep, &spool->addr); + spool->port = ~0; r->spool_cnt++; /* Point to the next possible cfg_spool. */ - spool_buf = &spool_buf[SOF_SPOOL]; + buf = &buf[sizeof(struct cfg_spool)]; sep = strtok(NULL, ","); } + } else + StrToAddr(**av, &r->laddr); + (*av)++; (*ac)--; + + /* Extract public address. */ + StrToAddr(**av, &r->paddr); + (*av)++; (*ac)--; + + return (space); +} + +static int +estimate_redir_port(int *ac, char ***av) +{ + size_t space = sizeof(struct cfg_redir); + char *sep; + + if ((sep = strtok(**av, ",")) != NULL) { + space += sizeof(struct cfg_spool); + while ((sep = strtok(NULL, ",")) != NULL) + space += sizeof(struct cfg_spool); } - return(space); -nospace: - errx(EX_DATAERR, "redirect_addr: buf is too small\n"); + + return (space); } static int -setup_redir_port(char *spool_buf, unsigned int len, - int *_ac, char ***_av) +setup_redir_port(char *buf, int *ac, char ***av) { - char **av, *sep, *protoName; - char tmp_spool_buf[NAT_BUF_LEN]; - int ac, space, lsnat; struct cfg_redir *r; - struct cfg_spool *tmp; + char *sep, *protoName, *lsnat = NULL; + size_t space; u_short numLocalPorts; port_range portRange; - av = *_av; - ac = *_ac; - space = 0; - lsnat = 0; numLocalPorts = 0; - if (len >= SOF_REDIR) { - r = (struct cfg_redir *)spool_buf; - /* Skip cfg_redir at beginning of buf. */ - spool_buf = &spool_buf[SOF_REDIR]; - space = SOF_REDIR; - len -= SOF_REDIR; - } else - goto nospace; + r = (struct cfg_redir *)buf; r->mode = REDIR_PORT; + /* Skip cfg_redir at beginning of buf. */ + buf = &buf[sizeof(struct cfg_redir)]; + space = sizeof(struct cfg_redir); + /* * Extract protocol. */ - if (ac == 0) - errx (EX_DATAERR, "redirect_port: missing protocol"); - r->proto = StrToProto(*av); - protoName = *av; - INC_ARGCV(); + r->proto = StrToProto(**av); + protoName = **av; + (*av)++; (*ac)--; /* * Extract local address. */ - if (ac == 0) - errx (EX_DATAERR, "redirect_port: missing local address"); - - sep = strchr(*av, ','); - /* LSNAT redirection syntax. */ - if (sep) { + if ((sep = strchr(**av, ',')) != NULL) { r->laddr.s_addr = INADDR_NONE; r->lport = ~0; numLocalPorts = 1; - /* Preserve av, copy spool servers to tmp_spool_buf. */ - strncpy(tmp_spool_buf, *av, strlen(*av)+1); - lsnat = 1; + lsnat = **av; } else { /* * The sctp nat does not allow the port numbers to be mapped to @@ -440,40 +419,36 @@ * in the target port field. */ if (r->proto == IPPROTO_SCTP) { - if (strchr (*av, ':')) + if (strchr(**av, ':')) errx(EX_DATAERR, "redirect_port:" - "port numbers do not change in sctp, so do not " - "specify them as part of the target"); + "port numbers do not change in sctp, so do " + "not specify them as part of the target"); else - StrToAddr(*av, &r->laddr); + StrToAddr(**av, &r->laddr); } else { - if (StrToAddrAndPortRange (*av, &r->laddr, protoName, - &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + if (StrToAddrAndPortRange(**av, &r->laddr, protoName, + &portRange) != 0) + errx(EX_DATAERR, "redirect_port: " "invalid local port range"); r->lport = GETLOPORT(portRange); numLocalPorts = GETNUMPORTS(portRange); } } - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract public port and optionally address. */ - if (ac == 0) - errx (EX_DATAERR, "redirect_port: missing public port"); - - sep = strchr (*av, ':'); - if (sep) { - if (StrToAddrAndPortRange (*av, &r->paddr, protoName, + if ((sep = strchr(**av, ':')) != NULL) { + if (StrToAddrAndPortRange(**av, &r->paddr, protoName, &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + errx(EX_DATAERR, "redirect_port: " "invalid public port range"); } else { r->paddr.s_addr = INADDR_ANY; - if (StrToPortRange (*av, protoName, &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + if (StrToPortRange(**av, protoName, &portRange) != 0) + errx(EX_DATAERR, "redirect_port: " "invalid public port range"); } @@ -483,7 +458,7 @@ r->lport = r->pport; } r->pport_cnt = GETNUMPORTS(portRange); - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract remote address and optionally port. @@ -492,19 +467,18 @@ * NB: isalpha(**av) => we've to check that next parameter is really an * option for this redirect entry, else stop here processing arg[cv]. */ - if (ac != 0 && !isalpha(**av)) { - sep = strchr (*av, ':'); - if (sep) { - if (StrToAddrAndPortRange (*av, &r->raddr, protoName, + if (*ac != 0 && !isalpha(***av)) { + if ((sep = strchr(**av, ':')) != NULL) { + if (StrToAddrAndPortRange(**av, &r->raddr, protoName, &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + errx(EX_DATAERR, "redirect_port: " "invalid remote port range"); } else { SETLOPORT(portRange, 0); SETNUMPORTS(portRange, 1); - StrToAddr (*av, &r->raddr); + StrToAddr(**av, &r->raddr); } - INC_ARGCV(); + (*av)++; (*ac)--; } else { SETLOPORT(portRange, 0); SETNUMPORTS(portRange, 1); @@ -517,7 +491,7 @@ * Make sure port ranges match up, then add the redirect ports. */ if (numLocalPorts != r->pport_cnt) - errx(EX_DATAERR, "redirect_port:" + errx(EX_DATAERR, "redirect_port: " "port ranges must be equal in size"); /* Remote port range is allowed to be '0' which means all ports. */ @@ -526,20 +500,18 @@ errx(EX_DATAERR, "redirect_port: remote port must" "be 0 or equal to local port range in size"); - /* - * Setup LSNAT server pool. - */ - if (lsnat) { - sep = strtok(tmp_spool_buf, ","); + /* Setup LSNAT server pool. */ + if (lsnat != NULL) { + struct cfg_spool *spool; + + sep = strtok(lsnat, ","); while (sep != NULL) { - tmp = (struct cfg_spool *)spool_buf; - if (len < SOF_SPOOL) - goto nospace; - len -= SOF_SPOOL; - space += SOF_SPOOL; + spool = (struct cfg_spool *)buf; + space += sizeof(struct cfg_spool); /* - * The sctp nat does not allow the port numbers to be mapped to new port numbers - * Therefore, no ports are to be specified in the target port field + * The sctp nat does not allow the port numbers to + * be mapped to new port numbers. Therefore, no ports + * are to be specified in the target port field. */ if (r->proto == IPPROTO_SCTP) { if (strchr (sep, ':')) { @@ -548,11 +520,11 @@ "sctp, so do not specify them as " "part of the target"); } else { - StrToAddr(sep, &tmp->addr); - tmp->port = r->pport; + StrToAddr(sep, &spool->addr); + spool->port = r->pport; } } else { - if (StrToAddrAndPortRange(sep, &tmp->addr, + if (StrToAddrAndPortRange(sep, &spool->addr, protoName, &portRange) != 0) errx(EX_DATAERR, "redirect_port:" "invalid local port range"); @@ -560,88 +532,73 @@ errx(EX_DATAERR, "redirect_port: " "local port must be single in " "this context"); - tmp->port = GETLOPORT(portRange); + spool->port = GETLOPORT(portRange); } r->spool_cnt++; /* Point to the next possible cfg_spool. */ - spool_buf = &spool_buf[SOF_SPOOL]; + buf = &buf[sizeof(struct cfg_spool)]; sep = strtok(NULL, ","); } } + return (space); -nospace: - errx(EX_DATAERR, "redirect_port: buf is too small\n"); } static int -setup_redir_proto(char *spool_buf, unsigned int len, - int *_ac, char ***_av) +setup_redir_proto(char *buf, int *ac, char ***av) { - char **av; - int ac, space; + struct cfg_redir *r; struct protoent *protoent; - struct cfg_redir *r; + size_t space; - av = *_av; - ac = *_ac; - if (len >= SOF_REDIR) { - r = (struct cfg_redir *)spool_buf; - /* Skip cfg_redir at beginning of buf. */ - spool_buf = &spool_buf[SOF_REDIR]; - space = SOF_REDIR; - len -= SOF_REDIR; - } else - goto nospace; + r = (struct cfg_redir *)buf; r->mode = REDIR_PROTO; + /* Skip cfg_redir at beginning of buf. */ + buf = &buf[sizeof(struct cfg_redir)]; + space = sizeof(struct cfg_redir); + /* * Extract protocol. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_proto: missing protocol"); - - protoent = getprotobyname(*av); + protoent = getprotobyname(**av); if (protoent == NULL) - errx(EX_DATAERR, "redirect_proto: unknown protocol %s", *av); + errx(EX_DATAERR, "redirect_proto: unknown protocol %s", **av); else r->proto = protoent->p_proto; - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract local address. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_proto: missing local address"); - else - StrToAddr(*av, &r->laddr); + StrToAddr(**av, &r->laddr); - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract optional public address. */ - if (ac == 0) { + if (*ac == 0) { r->paddr.s_addr = INADDR_ANY; r->raddr.s_addr = INADDR_ANY; } else { /* see above in setup_redir_port() */ - if (!isalpha(**av)) { - StrToAddr(*av, &r->paddr); - INC_ARGCV(); + if (!isalpha(***av)) { + StrToAddr(**av, &r->paddr); + (*av)++; (*ac)--; /* * Extract optional remote address. */ /* see above in setup_redir_port() */ - if (ac!=0 && !isalpha(**av)) { - StrToAddr(*av, &r->raddr); - INC_ARGCV(); + if (*ac != 0 && !isalpha(***av)) { + StrToAddr(**av, &r->raddr); + (*av)++; (*ac)--; } } } + return (space); -nospace: - errx(EX_DATAERR, "redirect_proto: buf is too small\n"); } static void @@ -763,27 +720,76 @@ ipfw_config_nat(int ac, char **av) { struct cfg_nat *n; /* Nat instance configuration. */ - int i, len, off, tok; - char *id, buf[NAT_BUF_LEN]; /* Buffer for serialized data. */ + int i, off, tok, ac1; + char *id, *buf, **av1; + size_t len; - len = NAT_BUF_LEN; - /* Offset in buf: save space for n at the beginning. */ - off = sizeof(*n); - memset(buf, 0, sizeof(buf)); - n = (struct cfg_nat *)buf; - av++; ac--; /* Nat id. */ if (ac && isdigit(**av)) { id = *av; - i = atoi(*av); ac--; av++; - n->id = i; } else errx(EX_DATAERR, "missing nat id"); if (ac == 0) errx(EX_DATAERR, "missing option"); + len = sizeof(struct cfg_nat); + ac1 = ac; + av1 = av; + while (ac1 > 0) { + tok = match_token(nat_params, *av1); + ac1--; av1++; + switch (tok) { + case TOK_IP: + case TOK_IF: + ac1--; av1++; + break; + case TOK_ALOG: + case TOK_DENY_INC: + case TOK_SAME_PORTS: + case TOK_UNREG_ONLY: + case TOK_RESET_ADDR: + case TOK_ALIAS_REV: + case TOK_PROXY_ONLY: + break; + case TOK_REDIR_ADDR: + if (ac1 < 2) + errx(EX_DATAERR, "redirect_addr: " + "not enough arguments"); + len += estimate_redir_addr(&ac1, &av1); + av1 += 2; ac1 -= 2; + break; + case TOK_REDIR_PORT: + if (ac1 < 3) + errx(EX_DATAERR, "redirect_port: " + "not enough arguments"); + av1++; ac1--; + len += estimate_redir_port(&ac1, &av1); + av1 += 2; ac1 -= 2; + break; + case TOK_REDIR_PROTO: + if (ac1 < 2) + errx(EX_DATAERR, "redirect_proto: " + "not enough arguments"); + len += sizeof(struct cfg_redir); + av1 += 2; ac1 -= 2; + break; + default: + errx(EX_DATAERR, "unrecognised option ``%s''", av1[-1]); + } + } + + if ((buf = malloc(len)) == NULL) + errx(EX_OSERR, "malloc failed"); + + /* Offset in buf: save space for n at the beginning. */ + off = sizeof(*n); + memset(buf, 0, len); + n = (struct cfg_nat *)buf; + i = atoi(id); + n->id = i; + while (ac > 0) { tok = match_token(nat_params, *av); ac--; av++; @@ -832,21 +838,18 @@ case TOK_REDIR_PROTO: switch (tok) { case TOK_REDIR_ADDR: - i = setup_redir_addr(&buf[off], len, &ac, &av); + i = setup_redir_addr(&buf[off], &ac, &av); break; case TOK_REDIR_PORT: - i = setup_redir_port(&buf[off], len, &ac, &av); + i = setup_redir_port(&buf[off], &ac, &av); break; case TOK_REDIR_PROTO: - i = setup_redir_proto(&buf[off], len, &ac, &av); + i = setup_redir_proto(&buf[off], &ac, &av); break; } n->redir_cnt++; off += i; - len -= i; break; - default: - errx(EX_DATAERR, "unrecognised option ``%s''", av[-1]); } } --J/dobhs11T7y2rNN Content-Type: text/x-diff; charset=koi8-r Content-Disposition: attachment; filename="143653.netinet.diff" Index: ip_fw.h =================================================================== --- ip_fw.h (revision 220826) +++ ip_fw.h (working copy) @@ -383,8 +383,6 @@ }; #endif -#define NAT_BUF_LEN 1024 - #ifdef IPFW_INTERNAL /* Nat configuration data struct. */ struct cfg_nat { Index: ipfw/ip_fw_private.h =================================================================== --- ipfw/ip_fw_private.h (revision 220826) +++ ipfw/ip_fw_private.h (working copy) @@ -225,6 +225,7 @@ struct rwlock uh_lock; /* lock for upper half */ #endif uint32_t id; /* ruleset id */ + uint32_t gencnt; /* generation count */ }; struct sockopt; /* used by tcp_var.h */ Index: ipfw/ip_fw_nat.c =================================================================== --- ipfw/ip_fw_nat.c (revision 220826) +++ ipfw/ip_fw_nat.c (working copy) @@ -138,7 +138,7 @@ } } -static int +static void add_redir_spool_cfg(char *buf, struct cfg_nat *ptr) { struct cfg_redir *r, *ser_r; @@ -199,7 +199,6 @@ /* And finally hook this redir entry. */ LIST_INSERT_HEAD(&ptr->redir_chain, r, _next); } - return (1); } static int @@ -344,20 +343,28 @@ static int ipfw_nat_cfg(struct sockopt *sopt) { - struct cfg_nat *ptr, *ser_n; + struct cfg_nat *cfg, *ptr; char *buf; struct ip_fw_chain *chain = &V_layer3_chain; + int gencnt, len, error = 0; - buf = malloc(NAT_BUF_LEN, M_IPFW, M_WAITOK | M_ZERO); - sooptcopyin(sopt, buf, NAT_BUF_LEN, sizeof(struct cfg_nat)); - ser_n = (struct cfg_nat *)buf; + len = sopt->sopt_valsize; + buf = malloc(len, M_TEMP, M_WAITOK | M_ZERO); + if ((error = sooptcopyin(sopt, buf, len, sizeof(struct cfg_nat))) != 0) + goto out; - /* check valid parameter ser_n->id > 0 ? */ + cfg = (struct cfg_nat *)buf; + if (cfg->id < 0) { + error = EINVAL; + goto out; + } + /* * Find/create nat rule. */ IPFW_WLOCK(chain); - ptr = lookup_nat(&chain->nat, ser_n->id); + gencnt = chain->gencnt; + ptr = lookup_nat(&chain->nat, cfg->id); if (ptr == NULL) { IPFW_WUNLOCK(chain); /* New rule: allocate and init new instance. */ @@ -365,27 +372,27 @@ ptr->lib = LibAliasInit(NULL); LIST_INIT(&ptr->redir_chain); } else { - /* Entry already present: temporarly unhook it. */ + /* Entry already present: temporarily unhook it. */ LIST_REMOVE(ptr, _next); - flush_nat_ptrs(chain, ser_n->id); + flush_nat_ptrs(chain, cfg->id); IPFW_WUNLOCK(chain); } /* * Basic nat configuration. */ - ptr->id = ser_n->id; + ptr->id = cfg->id; /* * 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; - ptr->redir_cnt = ser_n->redir_cnt; - ptr->mode = ser_n->mode; - LibAliasSetMode(ptr->lib, ser_n->mode, ser_n->mode); + ptr->ip = cfg->ip; + ptr->redir_cnt = cfg->redir_cnt; + ptr->mode = cfg->mode; + LibAliasSetMode(ptr->lib, cfg->mode, cfg->mode); LibAliasSetAddress(ptr->lib, ptr->ip); - memcpy(ptr->if_name, ser_n->if_name, IF_NAMESIZE); + memcpy(ptr->if_name, cfg->if_name, IF_NAMESIZE); /* * Redir and LSNAT configuration. @@ -394,15 +401,19 @@ del_redir_spool_cfg(ptr, &ptr->redir_chain); /* Add new entries. */ add_redir_spool_cfg(&buf[(sizeof(struct cfg_nat))], ptr); - free(buf, M_IPFW); + IPFW_WLOCK(chain); - /* - * XXXGL race here: another ipfw_nat_cfg() may already inserted - * entry with the same ser_n->id. - */ + /* Extra check to avoid race with another ipfw_nat_cfg() */ + if (gencnt != chain->gencnt && + ((cfg = lookup_nat(&chain->nat, ptr->id)) != NULL)) + LIST_REMOVE(cfg, _next); LIST_INSERT_HEAD(&chain->nat, ptr, _next); + chain->gencnt++; IPFW_WUNLOCK(chain); - return (0); + +out: + free(buf, M_TEMP); + return (error); } static int @@ -432,52 +443,61 @@ static int ipfw_nat_get_cfg(struct sockopt *sopt) { - uint8_t *data; + struct ip_fw_chain *chain = &V_layer3_chain; struct cfg_nat *n; struct cfg_redir *r; struct cfg_spool *s; - int nat_cnt, off; - struct ip_fw_chain *chain; - int err = ENOSPC; + char *data; + int gencnt, nat_cnt, len, error; - chain = &V_layer3_chain; nat_cnt = 0; - off = sizeof(nat_cnt); + len = sizeof(nat_cnt); - data = malloc(NAT_BUF_LEN, M_IPFW, M_WAITOK | M_ZERO); IPFW_RLOCK(chain); - /* Serialize all the data. */ +retry: + gencnt = chain->gencnt; + /* Estimate memory amount */ LIST_FOREACH(n, &chain->nat, _next) { nat_cnt++; - if (off + SOF_NAT >= NAT_BUF_LEN) - goto nospace; - bcopy(n, &data[off], SOF_NAT); - off += SOF_NAT; + len += sizeof(struct cfg_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; + len += sizeof(struct cfg_redir); + LIST_FOREACH(s, &r->spool_chain, _next) + len += sizeof(struct cfg_spool); + } + } + IPFW_RUNLOCK(chain); + + data = malloc(len, M_TEMP, M_WAITOK | M_ZERO); + bcopy(&nat_cnt, data, sizeof(nat_cnt)); + + nat_cnt = 0; + len = sizeof(nat_cnt); + + IPFW_RLOCK(chain); + if (gencnt != chain->gencnt) { + free(data, M_TEMP); + goto retry; + } + /* Serialize all the data. */ + LIST_FOREACH(n, &chain->nat, _next) { + bcopy(n, &data[len], sizeof(struct cfg_nat)); + len += sizeof(struct cfg_nat); + LIST_FOREACH(r, &n->redir_chain, _next) { + bcopy(r, &data[len], sizeof(struct cfg_redir)); + len += sizeof(struct cfg_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; + bcopy(s, &data[len], sizeof(struct cfg_spool)); + len += sizeof(struct cfg_spool); } } } - err = 0; /* all good */ -nospace: IPFW_RUNLOCK(chain); - 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 (err); + + error = sooptcopyout(sopt, data, len); + free(data, M_TEMP); + + return (error); } static int --J/dobhs11T7y2rNN-- State-Changed-From-To: open->patched State-Changed-By: glebius State-Changed-When: Tue Apr 19 15:09:27 UTC 2011 State-Changed-Why: Fixed in head/. Responsible-Changed-From-To: freebsd-ipfw->glebius Responsible-Changed-By: glebius Responsible-Changed-When: Tue Apr 19 15:09:27 UTC 2011 Responsible-Changed-Why: Fixed in head/. http://www.freebsd.org/cgi/query-pr.cgi?pr=143653 From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: kern/143653: commit references a PR Date: Tue, 19 Apr 2011 15:03:26 +0000 (UTC) Author: glebius Date: Tue Apr 19 15:03:12 2011 New Revision: 220835 URL: http://svn.freebsd.org/changeset/base/220835 Log: Rewrite NAT configuration parser, so that memory allocation size is calculated dynamically. PR: kern/143653 Modified: head/sbin/ipfw/nat.c Modified: head/sbin/ipfw/nat.c ============================================================================== --- head/sbin/ipfw/nat.c Tue Apr 19 13:54:51 2011 (r220834) +++ head/sbin/ipfw/nat.c Tue Apr 19 15:03:12 2011 (r220835) @@ -281,13 +281,6 @@ StrToAddrAndPortRange (const char* str, /* End of stuff taken from natd.c. */ -#define INC_ARGCV() do { \ - (*_av)++; \ - (*_ac)--; \ - av = *_av; \ - ac = *_ac; \ -} while(0) - /* * The next 3 functions add support for the addr, port and proto redirect and * their logic is loosely based on SetupAddressRedirect(), SetupPortRedirect() @@ -318,121 +311,107 @@ StrToAddrAndPortRange (const char* str, */ static int -setup_redir_addr(char *spool_buf, unsigned int len, - int *_ac, char ***_av) +estimate_redir_addr(int *ac, char ***av) +{ + size_t space = sizeof(struct cfg_redir); + char *sep; + + if ((sep = strtok(**av, ",")) != NULL) { + space += sizeof(struct cfg_spool); + while ((sep = strtok(NULL, ",")) != NULL) + space += sizeof(struct cfg_spool); + } + + return (space); +} + +static int +setup_redir_addr(char *buf, int *ac, char ***av) { - char **av, *sep; /* Token separator. */ - /* Temporary buffer used to hold server pool ip's. */ - char tmp_spool_buf[NAT_BUF_LEN]; - int ac, space, lsnat; struct cfg_redir *r; - struct cfg_spool *tmp; + char *sep; + size_t space; - av = *_av; - ac = *_ac; - space = 0; - lsnat = 0; - if (len >= SOF_REDIR) { - r = (struct cfg_redir *)spool_buf; - /* Skip cfg_redir at beginning of buf. */ - spool_buf = &spool_buf[SOF_REDIR]; - space = SOF_REDIR; - len -= SOF_REDIR; - } else - goto nospace; + r = (struct cfg_redir *)buf; r->mode = REDIR_ADDR; - /* Extract local address. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_addr: missing local address"); - sep = strchr(*av, ','); - if (sep) { /* LSNAT redirection syntax. */ - r->laddr.s_addr = INADDR_NONE; - /* Preserve av, copy spool servers to tmp_spool_buf. */ - strncpy(tmp_spool_buf, *av, strlen(*av)+1); - lsnat = 1; - } else - StrToAddr(*av, &r->laddr); - INC_ARGCV(); + /* Skip cfg_redir at beginning of buf. */ + buf = &buf[sizeof(struct cfg_redir)]; + space = sizeof(struct cfg_redir); - /* Extract public address. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_addr: missing public address"); - StrToAddr(*av, &r->paddr); - INC_ARGCV(); + /* Extract local address. */ + if ((sep = strtok(**av, ",")) != NULL) { + struct cfg_spool *spool; - /* Setup LSNAT server pool. */ - if (sep) { - sep = strtok(tmp_spool_buf, ","); + /* Setup LSNAT server pool. */ + r->laddr.s_addr = INADDR_NONE; while (sep != NULL) { - tmp = (struct cfg_spool *)spool_buf; - if (len < SOF_SPOOL) - goto nospace; - len -= SOF_SPOOL; - space += SOF_SPOOL; - StrToAddr(sep, &tmp->addr); - tmp->port = ~0; + spool = (struct cfg_spool *)buf; + space += sizeof(struct cfg_spool); + StrToAddr(sep, &spool->addr); + spool->port = ~0; r->spool_cnt++; /* Point to the next possible cfg_spool. */ - spool_buf = &spool_buf[SOF_SPOOL]; + buf = &buf[sizeof(struct cfg_spool)]; sep = strtok(NULL, ","); } + } else + StrToAddr(**av, &r->laddr); + (*av)++; (*ac)--; + + /* Extract public address. */ + StrToAddr(**av, &r->paddr); + (*av)++; (*ac)--; + + return (space); +} + +static int +estimate_redir_port(int *ac, char ***av) +{ + size_t space = sizeof(struct cfg_redir); + char *sep; + + if ((sep = strtok(**av, ",")) != NULL) { + space += sizeof(struct cfg_spool); + while ((sep = strtok(NULL, ",")) != NULL) + space += sizeof(struct cfg_spool); } - return(space); -nospace: - errx(EX_DATAERR, "redirect_addr: buf is too small\n"); + + return (space); } static int -setup_redir_port(char *spool_buf, unsigned int len, - int *_ac, char ***_av) +setup_redir_port(char *buf, int *ac, char ***av) { - char **av, *sep, *protoName; - char tmp_spool_buf[NAT_BUF_LEN]; - int ac, space, lsnat; struct cfg_redir *r; - struct cfg_spool *tmp; + char *sep, *protoName, *lsnat = NULL; + size_t space; u_short numLocalPorts; port_range portRange; - av = *_av; - ac = *_ac; - space = 0; - lsnat = 0; numLocalPorts = 0; - if (len >= SOF_REDIR) { - r = (struct cfg_redir *)spool_buf; - /* Skip cfg_redir at beginning of buf. */ - spool_buf = &spool_buf[SOF_REDIR]; - space = SOF_REDIR; - len -= SOF_REDIR; - } else - goto nospace; + r = (struct cfg_redir *)buf; r->mode = REDIR_PORT; + /* Skip cfg_redir at beginning of buf. */ + buf = &buf[sizeof(struct cfg_redir)]; + space = sizeof(struct cfg_redir); + /* * Extract protocol. */ - if (ac == 0) - errx (EX_DATAERR, "redirect_port: missing protocol"); - r->proto = StrToProto(*av); - protoName = *av; - INC_ARGCV(); + r->proto = StrToProto(**av); + protoName = **av; + (*av)++; (*ac)--; /* * Extract local address. */ - if (ac == 0) - errx (EX_DATAERR, "redirect_port: missing local address"); - - sep = strchr(*av, ','); - /* LSNAT redirection syntax. */ - if (sep) { + if ((sep = strchr(**av, ',')) != NULL) { r->laddr.s_addr = INADDR_NONE; r->lport = ~0; numLocalPorts = 1; - /* Preserve av, copy spool servers to tmp_spool_buf. */ - strncpy(tmp_spool_buf, *av, strlen(*av)+1); - lsnat = 1; + lsnat = **av; } else { /* * The sctp nat does not allow the port numbers to be mapped to @@ -440,40 +419,36 @@ setup_redir_port(char *spool_buf, unsign * in the target port field. */ if (r->proto == IPPROTO_SCTP) { - if (strchr (*av, ':')) + if (strchr(**av, ':')) errx(EX_DATAERR, "redirect_port:" - "port numbers do not change in sctp, so do not " - "specify them as part of the target"); + "port numbers do not change in sctp, so do " + "not specify them as part of the target"); else - StrToAddr(*av, &r->laddr); + StrToAddr(**av, &r->laddr); } else { - if (StrToAddrAndPortRange (*av, &r->laddr, protoName, - &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + if (StrToAddrAndPortRange(**av, &r->laddr, protoName, + &portRange) != 0) + errx(EX_DATAERR, "redirect_port: " "invalid local port range"); r->lport = GETLOPORT(portRange); numLocalPorts = GETNUMPORTS(portRange); } } - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract public port and optionally address. */ - if (ac == 0) - errx (EX_DATAERR, "redirect_port: missing public port"); - - sep = strchr (*av, ':'); - if (sep) { - if (StrToAddrAndPortRange (*av, &r->paddr, protoName, + if ((sep = strchr(**av, ':')) != NULL) { + if (StrToAddrAndPortRange(**av, &r->paddr, protoName, &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + errx(EX_DATAERR, "redirect_port: " "invalid public port range"); } else { r->paddr.s_addr = INADDR_ANY; - if (StrToPortRange (*av, protoName, &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + if (StrToPortRange(**av, protoName, &portRange) != 0) + errx(EX_DATAERR, "redirect_port: " "invalid public port range"); } @@ -483,7 +458,7 @@ setup_redir_port(char *spool_buf, unsign r->lport = r->pport; } r->pport_cnt = GETNUMPORTS(portRange); - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract remote address and optionally port. @@ -492,19 +467,18 @@ setup_redir_port(char *spool_buf, unsign * NB: isalpha(**av) => we've to check that next parameter is really an * option for this redirect entry, else stop here processing arg[cv]. */ - if (ac != 0 && !isalpha(**av)) { - sep = strchr (*av, ':'); - if (sep) { - if (StrToAddrAndPortRange (*av, &r->raddr, protoName, + if (*ac != 0 && !isalpha(***av)) { + if ((sep = strchr(**av, ':')) != NULL) { + if (StrToAddrAndPortRange(**av, &r->raddr, protoName, &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + errx(EX_DATAERR, "redirect_port: " "invalid remote port range"); } else { SETLOPORT(portRange, 0); SETNUMPORTS(portRange, 1); - StrToAddr (*av, &r->raddr); + StrToAddr(**av, &r->raddr); } - INC_ARGCV(); + (*av)++; (*ac)--; } else { SETLOPORT(portRange, 0); SETNUMPORTS(portRange, 1); @@ -517,7 +491,7 @@ setup_redir_port(char *spool_buf, unsign * Make sure port ranges match up, then add the redirect ports. */ if (numLocalPorts != r->pport_cnt) - errx(EX_DATAERR, "redirect_port:" + errx(EX_DATAERR, "redirect_port: " "port ranges must be equal in size"); /* Remote port range is allowed to be '0' which means all ports. */ @@ -526,20 +500,18 @@ setup_redir_port(char *spool_buf, unsign errx(EX_DATAERR, "redirect_port: remote port must" "be 0 or equal to local port range in size"); - /* - * Setup LSNAT server pool. - */ - if (lsnat) { - sep = strtok(tmp_spool_buf, ","); + /* Setup LSNAT server pool. */ + if (lsnat != NULL) { + struct cfg_spool *spool; + + sep = strtok(lsnat, ","); while (sep != NULL) { - tmp = (struct cfg_spool *)spool_buf; - if (len < SOF_SPOOL) - goto nospace; - len -= SOF_SPOOL; - space += SOF_SPOOL; + spool = (struct cfg_spool *)buf; + space += sizeof(struct cfg_spool); /* - * The sctp nat does not allow the port numbers to be mapped to new port numbers - * Therefore, no ports are to be specified in the target port field + * The sctp nat does not allow the port numbers to + * be mapped to new port numbers. Therefore, no ports + * are to be specified in the target port field. */ if (r->proto == IPPROTO_SCTP) { if (strchr (sep, ':')) { @@ -548,11 +520,11 @@ setup_redir_port(char *spool_buf, unsign "sctp, so do not specify them as " "part of the target"); } else { - StrToAddr(sep, &tmp->addr); - tmp->port = r->pport; + StrToAddr(sep, &spool->addr); + spool->port = r->pport; } } else { - if (StrToAddrAndPortRange(sep, &tmp->addr, + if (StrToAddrAndPortRange(sep, &spool->addr, protoName, &portRange) != 0) errx(EX_DATAERR, "redirect_port:" "invalid local port range"); @@ -560,88 +532,73 @@ setup_redir_port(char *spool_buf, unsign errx(EX_DATAERR, "redirect_port: " "local port must be single in " "this context"); - tmp->port = GETLOPORT(portRange); + spool->port = GETLOPORT(portRange); } r->spool_cnt++; /* Point to the next possible cfg_spool. */ - spool_buf = &spool_buf[SOF_SPOOL]; + buf = &buf[sizeof(struct cfg_spool)]; sep = strtok(NULL, ","); } } + return (space); -nospace: - errx(EX_DATAERR, "redirect_port: buf is too small\n"); } static int -setup_redir_proto(char *spool_buf, unsigned int len, - int *_ac, char ***_av) +setup_redir_proto(char *buf, int *ac, char ***av) { - char **av; - int ac, space; - struct protoent *protoent; struct cfg_redir *r; + struct protoent *protoent; + size_t space; - av = *_av; - ac = *_ac; - if (len >= SOF_REDIR) { - r = (struct cfg_redir *)spool_buf; - /* Skip cfg_redir at beginning of buf. */ - spool_buf = &spool_buf[SOF_REDIR]; - space = SOF_REDIR; - len -= SOF_REDIR; - } else - goto nospace; + r = (struct cfg_redir *)buf; r->mode = REDIR_PROTO; + /* Skip cfg_redir at beginning of buf. */ + buf = &buf[sizeof(struct cfg_redir)]; + space = sizeof(struct cfg_redir); + /* * Extract protocol. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_proto: missing protocol"); - - protoent = getprotobyname(*av); + protoent = getprotobyname(**av); if (protoent == NULL) - errx(EX_DATAERR, "redirect_proto: unknown protocol %s", *av); + errx(EX_DATAERR, "redirect_proto: unknown protocol %s", **av); else r->proto = protoent->p_proto; - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract local address. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_proto: missing local address"); - else - StrToAddr(*av, &r->laddr); + StrToAddr(**av, &r->laddr); - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract optional public address. */ - if (ac == 0) { + if (*ac == 0) { r->paddr.s_addr = INADDR_ANY; r->raddr.s_addr = INADDR_ANY; } else { /* see above in setup_redir_port() */ - if (!isalpha(**av)) { - StrToAddr(*av, &r->paddr); - INC_ARGCV(); + if (!isalpha(***av)) { + StrToAddr(**av, &r->paddr); + (*av)++; (*ac)--; /* * Extract optional remote address. */ /* see above in setup_redir_port() */ - if (ac!=0 && !isalpha(**av)) { - StrToAddr(*av, &r->raddr); - INC_ARGCV(); + if (*ac != 0 && !isalpha(***av)) { + StrToAddr(**av, &r->raddr); + (*av)++; (*ac)--; } } } + return (space); -nospace: - errx(EX_DATAERR, "redirect_proto: buf is too small\n"); } static void @@ -763,27 +720,76 @@ void ipfw_config_nat(int ac, char **av) { struct cfg_nat *n; /* Nat instance configuration. */ - int i, len, off, tok; - char *id, buf[NAT_BUF_LEN]; /* Buffer for serialized data. */ - - len = NAT_BUF_LEN; - /* Offset in buf: save space for n at the beginning. */ - off = sizeof(*n); - memset(buf, 0, sizeof(buf)); - n = (struct cfg_nat *)buf; + int i, off, tok, ac1; + char *id, *buf, **av1; + size_t len; av++; ac--; /* Nat id. */ if (ac && isdigit(**av)) { id = *av; - i = atoi(*av); ac--; av++; - n->id = i; } else errx(EX_DATAERR, "missing nat id"); if (ac == 0) errx(EX_DATAERR, "missing option"); + len = sizeof(struct cfg_nat); + ac1 = ac; + av1 = av; + while (ac1 > 0) { + tok = match_token(nat_params, *av1); + ac1--; av1++; + switch (tok) { + case TOK_IP: + case TOK_IF: + ac1--; av1++; + break; + case TOK_ALOG: + case TOK_DENY_INC: + case TOK_SAME_PORTS: + case TOK_UNREG_ONLY: + case TOK_RESET_ADDR: + case TOK_ALIAS_REV: + case TOK_PROXY_ONLY: + break; + case TOK_REDIR_ADDR: + if (ac1 < 2) + errx(EX_DATAERR, "redirect_addr: " + "not enough arguments"); + len += estimate_redir_addr(&ac1, &av1); + av1 += 2; ac1 -= 2; + break; + case TOK_REDIR_PORT: + if (ac1 < 3) + errx(EX_DATAERR, "redirect_port: " + "not enough arguments"); + av1++; ac1--; + len += estimate_redir_port(&ac1, &av1); + av1 += 2; ac1 -= 2; + break; + case TOK_REDIR_PROTO: + if (ac1 < 2) + errx(EX_DATAERR, "redirect_proto: " + "not enough arguments"); + len += sizeof(struct cfg_redir); + av1 += 2; ac1 -= 2; + break; + default: + errx(EX_DATAERR, "unrecognised option ``%s''", av1[-1]); + } + } + + if ((buf = malloc(len)) == NULL) + errx(EX_OSERR, "malloc failed"); + + /* Offset in buf: save space for n at the beginning. */ + off = sizeof(*n); + memset(buf, 0, len); + n = (struct cfg_nat *)buf; + i = atoi(id); + n->id = i; + while (ac > 0) { tok = match_token(nat_params, *av); ac--; av++; @@ -832,21 +838,18 @@ ipfw_config_nat(int ac, char **av) case TOK_REDIR_PROTO: switch (tok) { case TOK_REDIR_ADDR: - i = setup_redir_addr(&buf[off], len, &ac, &av); + i = setup_redir_addr(&buf[off], &ac, &av); break; case TOK_REDIR_PORT: - i = setup_redir_port(&buf[off], len, &ac, &av); + i = setup_redir_port(&buf[off], &ac, &av); break; case TOK_REDIR_PROTO: - i = setup_redir_proto(&buf[off], len, &ac, &av); + i = setup_redir_proto(&buf[off], &ac, &av); break; } n->redir_cnt++; off += i; - len -= i; break; - default: - errx(EX_DATAERR, "unrecognised option ``%s''", av[-1]); } } _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: kern/143653: commit references a PR Date: Tue, 19 Apr 2011 15:06:42 +0000 (UTC) Author: glebius Date: Tue Apr 19 15:06:33 2011 New Revision: 220837 URL: http://svn.freebsd.org/changeset/base/220837 Log: - Rewrite functions that copyin/out NAT configuration, so that they calculate required memory size dynamically. - Fix races on chain re-lock. - Introduce new field to ip_fw_chain - generation count. Now utilized only in the NAT configuration, but can be utilized wider in ipfw. - Get rid of NAT_BUF_LEN in ip_fw.h PR: kern/143653 Modified: head/sys/netinet/ip_fw.h head/sys/netinet/ipfw/ip_fw_nat.c head/sys/netinet/ipfw/ip_fw_private.h Modified: head/sys/netinet/ip_fw.h ============================================================================== --- head/sys/netinet/ip_fw.h Tue Apr 19 15:05:12 2011 (r220836) +++ head/sys/netinet/ip_fw.h Tue Apr 19 15:06:33 2011 (r220837) @@ -383,8 +383,6 @@ struct cfg_redir { }; #endif -#define NAT_BUF_LEN 1024 - #ifdef IPFW_INTERNAL /* Nat configuration data struct. */ struct cfg_nat { Modified: head/sys/netinet/ipfw/ip_fw_nat.c ============================================================================== --- head/sys/netinet/ipfw/ip_fw_nat.c Tue Apr 19 15:05:12 2011 (r220836) +++ head/sys/netinet/ipfw/ip_fw_nat.c Tue Apr 19 15:06:33 2011 (r220837) @@ -138,7 +138,7 @@ del_redir_spool_cfg(struct cfg_nat *n, s } } -static int +static void add_redir_spool_cfg(char *buf, struct cfg_nat *ptr) { struct cfg_redir *r, *ser_r; @@ -199,7 +199,6 @@ add_redir_spool_cfg(char *buf, struct cf /* And finally hook this redir entry. */ LIST_INSERT_HEAD(&ptr->redir_chain, r, _next); } - return (1); } static int @@ -344,20 +343,28 @@ lookup_nat(struct nat_list *l, int nat_i static int ipfw_nat_cfg(struct sockopt *sopt) { - struct cfg_nat *ptr, *ser_n; + struct cfg_nat *cfg, *ptr; char *buf; struct ip_fw_chain *chain = &V_layer3_chain; + int gencnt, len, error = 0; - buf = malloc(NAT_BUF_LEN, M_IPFW, M_WAITOK | M_ZERO); - sooptcopyin(sopt, buf, NAT_BUF_LEN, sizeof(struct cfg_nat)); - ser_n = (struct cfg_nat *)buf; + len = sopt->sopt_valsize; + buf = malloc(len, M_TEMP, M_WAITOK | M_ZERO); + if ((error = sooptcopyin(sopt, buf, len, sizeof(struct cfg_nat))) != 0) + goto out; + + cfg = (struct cfg_nat *)buf; + if (cfg->id < 0) { + error = EINVAL; + goto out; + } - /* check valid parameter ser_n->id > 0 ? */ /* * Find/create nat rule. */ IPFW_WLOCK(chain); - ptr = lookup_nat(&chain->nat, ser_n->id); + gencnt = chain->gencnt; + ptr = lookup_nat(&chain->nat, cfg->id); if (ptr == NULL) { IPFW_WUNLOCK(chain); /* New rule: allocate and init new instance. */ @@ -365,27 +372,27 @@ ipfw_nat_cfg(struct sockopt *sopt) ptr->lib = LibAliasInit(NULL); LIST_INIT(&ptr->redir_chain); } else { - /* Entry already present: temporarly unhook it. */ + /* Entry already present: temporarily unhook it. */ LIST_REMOVE(ptr, _next); - flush_nat_ptrs(chain, ser_n->id); + flush_nat_ptrs(chain, cfg->id); IPFW_WUNLOCK(chain); } /* * Basic nat configuration. */ - ptr->id = ser_n->id; + ptr->id = cfg->id; /* * 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; - ptr->redir_cnt = ser_n->redir_cnt; - ptr->mode = ser_n->mode; - LibAliasSetMode(ptr->lib, ser_n->mode, ser_n->mode); + ptr->ip = cfg->ip; + ptr->redir_cnt = cfg->redir_cnt; + ptr->mode = cfg->mode; + LibAliasSetMode(ptr->lib, cfg->mode, cfg->mode); LibAliasSetAddress(ptr->lib, ptr->ip); - memcpy(ptr->if_name, ser_n->if_name, IF_NAMESIZE); + memcpy(ptr->if_name, cfg->if_name, IF_NAMESIZE); /* * Redir and LSNAT configuration. @@ -394,15 +401,19 @@ ipfw_nat_cfg(struct sockopt *sopt) del_redir_spool_cfg(ptr, &ptr->redir_chain); /* Add new entries. */ add_redir_spool_cfg(&buf[(sizeof(struct cfg_nat))], ptr); - free(buf, M_IPFW); + IPFW_WLOCK(chain); - /* - * XXXGL race here: another ipfw_nat_cfg() may already inserted - * entry with the same ser_n->id. - */ + /* Extra check to avoid race with another ipfw_nat_cfg() */ + if (gencnt != chain->gencnt && + ((cfg = lookup_nat(&chain->nat, ptr->id)) != NULL)) + LIST_REMOVE(cfg, _next); LIST_INSERT_HEAD(&chain->nat, ptr, _next); + chain->gencnt++; IPFW_WUNLOCK(chain); - return (0); + +out: + free(buf, M_TEMP); + return (error); } static int @@ -432,52 +443,61 @@ ipfw_nat_del(struct sockopt *sopt) static int ipfw_nat_get_cfg(struct sockopt *sopt) { - uint8_t *data; + struct ip_fw_chain *chain = &V_layer3_chain; struct cfg_nat *n; struct cfg_redir *r; struct cfg_spool *s; - int nat_cnt, off; - struct ip_fw_chain *chain; - int err = ENOSPC; + char *data; + int gencnt, nat_cnt, len, error; - chain = &V_layer3_chain; nat_cnt = 0; - off = sizeof(nat_cnt); + len = sizeof(nat_cnt); - data = malloc(NAT_BUF_LEN, M_IPFW, M_WAITOK | M_ZERO); IPFW_RLOCK(chain); - /* Serialize all the data. */ +retry: + gencnt = chain->gencnt; + /* Estimate memory amount */ LIST_FOREACH(n, &chain->nat, _next) { nat_cnt++; - if (off + SOF_NAT >= NAT_BUF_LEN) - goto nospace; - bcopy(n, &data[off], SOF_NAT); - off += SOF_NAT; + len += sizeof(struct cfg_nat); + LIST_FOREACH(r, &n->redir_chain, _next) { + len += sizeof(struct cfg_redir); + LIST_FOREACH(s, &r->spool_chain, _next) + len += sizeof(struct cfg_spool); + } + } + IPFW_RUNLOCK(chain); + + data = malloc(len, M_TEMP, M_WAITOK | M_ZERO); + bcopy(&nat_cnt, data, sizeof(nat_cnt)); + + nat_cnt = 0; + len = sizeof(nat_cnt); + + IPFW_RLOCK(chain); + if (gencnt != chain->gencnt) { + free(data, M_TEMP); + goto retry; + } + /* Serialize all the data. */ + LIST_FOREACH(n, &chain->nat, _next) { + bcopy(n, &data[len], sizeof(struct cfg_nat)); + len += sizeof(struct cfg_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; + bcopy(r, &data[len], sizeof(struct cfg_redir)); + len += sizeof(struct cfg_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; + bcopy(s, &data[len], sizeof(struct cfg_spool)); + len += sizeof(struct cfg_spool); } } } - err = 0; /* all good */ -nospace: IPFW_RUNLOCK(chain); - 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 (err); + + error = sooptcopyout(sopt, data, len); + free(data, M_TEMP); + + return (error); } static int Modified: head/sys/netinet/ipfw/ip_fw_private.h ============================================================================== --- head/sys/netinet/ipfw/ip_fw_private.h Tue Apr 19 15:05:12 2011 (r220836) +++ head/sys/netinet/ipfw/ip_fw_private.h Tue Apr 19 15:06:33 2011 (r220837) @@ -225,6 +225,7 @@ struct ip_fw_chain { struct rwlock uh_lock; /* lock for upper half */ #endif uint32_t id; /* ruleset id */ + uint32_t gencnt; /* generation count */ }; struct sockopt; /* used by tcp_var.h */ _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: kern/143653: commit references a PR Date: Fri, 17 Jun 2011 12:13:05 +0000 (UTC) Author: glebius Date: Fri Jun 17 12:12:52 2011 New Revision: 223185 URL: http://svn.freebsd.org/changeset/base/223185 Log: - Fix my braino in the 220835, when I used strtok(). It isn't applicable here, since modifies the string. Switch to strchr(). - Restore support for undocumented optional parameters of redir_port and redir_proto, that were disabled in 220835. - While here, change !isalpha() checks on optinal parameters for isdigit(). Submitted by: Alexander V. Chernikov PR: kern/143653 Modified: head/sbin/ipfw/nat.c Modified: head/sbin/ipfw/nat.c ============================================================================== --- head/sbin/ipfw/nat.c Fri Jun 17 11:13:37 2011 (r223184) +++ head/sbin/ipfw/nat.c Fri Jun 17 12:12:52 2011 (r223185) @@ -315,14 +315,19 @@ static int estimate_redir_addr(int *ac, char ***av) { size_t space = sizeof(struct cfg_redir); - char *sep; + char *sep = **av; + u_int c = 0; - if ((sep = strtok(**av, ",")) != NULL) { - space += sizeof(struct cfg_spool); - while ((sep = strtok(NULL, ",")) != NULL) - space += sizeof(struct cfg_spool); + while ((sep = strchr(sep, ',')) != NULL) { + c++; + sep++; } + if (c > 0) + c++; + + space += c * sizeof(struct cfg_spool); + return (space); } @@ -370,14 +375,19 @@ static int estimate_redir_port(int *ac, char ***av) { size_t space = sizeof(struct cfg_redir); - char *sep; + char *sep = **av; + u_int c = 0; - if ((sep = strtok(**av, ",")) != NULL) { - space += sizeof(struct cfg_spool); - while ((sep = strtok(NULL, ",")) != NULL) - space += sizeof(struct cfg_spool); + while ((sep = strchr(sep, ',')) != NULL) { + c++; + sep++; } + if (c > 0) + c++; + + space += c * sizeof(struct cfg_spool); + return (space); } @@ -465,10 +475,10 @@ setup_redir_port(char *buf, int *ac, cha * Extract remote address and optionally port. */ /* - * NB: isalpha(**av) => we've to check that next parameter is really an + * NB: isdigit(**av) => we've to check that next parameter is really an * option for this redirect entry, else stop here processing arg[cv]. */ - if (*ac != 0 && !isalpha(***av)) { + if (*ac != 0 && isdigit(***av)) { if ((sep = strchr(**av, ':')) != NULL) { if (StrToAddrAndPortRange(**av, &r->raddr, protoName, &portRange) != 0) @@ -584,7 +594,7 @@ setup_redir_proto(char *buf, int *ac, ch r->raddr.s_addr = INADDR_ANY; } else { /* see above in setup_redir_port() */ - if (!isalpha(***av)) { + if (isdigit(***av)) { StrToAddr(**av, &r->paddr); (*av)++; (*ac)--; @@ -592,7 +602,7 @@ setup_redir_proto(char *buf, int *ac, ch * Extract optional remote address. */ /* see above in setup_redir_port() */ - if (*ac != 0 && !isalpha(***av)) { + if (*ac != 0 && isdigit(***av)) { StrToAddr(**av, &r->raddr); (*av)++; (*ac)--; } @@ -774,6 +784,9 @@ ipfw_config_nat(int ac, char **av) av1++; ac1--; len += estimate_redir_port(&ac1, &av1); av1 += 2; ac1 -= 2; + /* Skip optional remoteIP/port */ + if (ac1 != 0 && isdigit(**av1)) + av1++; ac1--; break; case TOK_REDIR_PROTO: if (ac1 < 2) @@ -781,6 +794,11 @@ ipfw_config_nat(int ac, char **av) "not enough arguments"); len += sizeof(struct cfg_redir); av1 += 2; ac1 -= 2; + /* Skip optional remoteIP/port */ + if (ac1 != 0 && isdigit(**av1)) + av1++; ac1--; + if (ac1 != 0 && isdigit(**av1)) + av1++; ac1--; break; default: errx(EX_DATAERR, "unrecognised option ``%s''", av1[-1]); _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: kern/143653: commit references a PR Date: Fri, 8 Jul 2011 14:26:51 +0000 (UTC) Author: glebius Date: Fri Jul 8 14:26:42 2011 New Revision: 223871 URL: http://svn.freebsd.org/changeset/base/223871 Log: Merge from head/ 220835,220914,223079,223185,223416,223499: Rewrite NAT configuration parser, so that memory allocation size is calculated dynamically. PR: kern/143653 Modified: stable/8/sbin/ipfw/nat.c Directory Properties: stable/8/sbin/ipfw/ (props changed) Modified: stable/8/sbin/ipfw/nat.c ============================================================================== --- stable/8/sbin/ipfw/nat.c Fri Jul 8 13:45:53 2011 (r223870) +++ stable/8/sbin/ipfw/nat.c Fri Jul 8 14:26:42 2011 (r223871) @@ -281,13 +281,6 @@ StrToAddrAndPortRange (const char* str, /* End of stuff taken from natd.c. */ -#define INC_ARGCV() do { \ - (*_av)++; \ - (*_ac)--; \ - av = *_av; \ - ac = *_ac; \ -} while(0) - /* * The next 3 functions add support for the addr, port and proto redirect and * their logic is loosely based on SetupAddressRedirect(), SetupPortRedirect() @@ -318,121 +311,117 @@ StrToAddrAndPortRange (const char* str, */ static int -setup_redir_addr(char *spool_buf, unsigned int len, - int *_ac, char ***_av) +estimate_redir_addr(int *ac, char ***av) +{ + size_t space = sizeof(struct cfg_redir); + char *sep = **av; + u_int c = 0; + + while ((sep = strchr(sep, ',')) != NULL) { + c++; + sep++; + } + + if (c > 0) + c++; + + space += c * sizeof(struct cfg_spool); + + return (space); +} + +static int +setup_redir_addr(char *buf, int *ac, char ***av) { - char **av, *sep; /* Token separator. */ - /* Temporary buffer used to hold server pool ip's. */ - char tmp_spool_buf[NAT_BUF_LEN]; - int ac, space, lsnat; struct cfg_redir *r; - struct cfg_spool *tmp; + char *sep; + size_t space; - av = *_av; - ac = *_ac; - space = 0; - lsnat = 0; - if (len >= SOF_REDIR) { - r = (struct cfg_redir *)spool_buf; - /* Skip cfg_redir at beginning of buf. */ - spool_buf = &spool_buf[SOF_REDIR]; - space = SOF_REDIR; - len -= SOF_REDIR; - } else - goto nospace; + r = (struct cfg_redir *)buf; r->mode = REDIR_ADDR; - /* Extract local address. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_addr: missing local address"); - sep = strchr(*av, ','); - if (sep) { /* LSNAT redirection syntax. */ - r->laddr.s_addr = INADDR_NONE; - /* Preserve av, copy spool servers to tmp_spool_buf. */ - strncpy(tmp_spool_buf, *av, strlen(*av)+1); - lsnat = 1; - } else - StrToAddr(*av, &r->laddr); - INC_ARGCV(); + /* Skip cfg_redir at beginning of buf. */ + buf = &buf[sizeof(struct cfg_redir)]; + space = sizeof(struct cfg_redir); - /* Extract public address. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_addr: missing public address"); - StrToAddr(*av, &r->paddr); - INC_ARGCV(); + /* Extract local address. */ + if ((sep = strtok(**av, ",")) != NULL) { + struct cfg_spool *spool; - /* Setup LSNAT server pool. */ - if (sep) { - sep = strtok(tmp_spool_buf, ","); + /* Setup LSNAT server pool. */ + r->laddr.s_addr = INADDR_NONE; while (sep != NULL) { - tmp = (struct cfg_spool *)spool_buf; - if (len < SOF_SPOOL) - goto nospace; - len -= SOF_SPOOL; - space += SOF_SPOOL; - StrToAddr(sep, &tmp->addr); - tmp->port = ~0; + spool = (struct cfg_spool *)buf; + space += sizeof(struct cfg_spool); + StrToAddr(sep, &spool->addr); + spool->port = ~0; r->spool_cnt++; /* Point to the next possible cfg_spool. */ - spool_buf = &spool_buf[SOF_SPOOL]; + buf = &buf[sizeof(struct cfg_spool)]; sep = strtok(NULL, ","); } + } else + StrToAddr(**av, &r->laddr); + (*av)++; (*ac)--; + + /* Extract public address. */ + StrToAddr(**av, &r->paddr); + (*av)++; (*ac)--; + + return (space); +} + +static int +estimate_redir_port(int *ac, char ***av) +{ + size_t space = sizeof(struct cfg_redir); + char *sep = **av; + u_int c = 0; + + while ((sep = strchr(sep, ',')) != NULL) { + c++; + sep++; } - return(space); -nospace: - errx(EX_DATAERR, "redirect_addr: buf is too small\n"); + + if (c > 0) + c++; + + space += c * sizeof(struct cfg_spool); + + return (space); } static int -setup_redir_port(char *spool_buf, unsigned int len, - int *_ac, char ***_av) +setup_redir_port(char *buf, int *ac, char ***av) { - char **av, *sep, *protoName; - char tmp_spool_buf[NAT_BUF_LEN]; - int ac, space, lsnat; struct cfg_redir *r; - struct cfg_spool *tmp; + char *sep, *protoName, *lsnat = NULL; + size_t space; u_short numLocalPorts; port_range portRange; - av = *_av; - ac = *_ac; - space = 0; - lsnat = 0; numLocalPorts = 0; - if (len >= SOF_REDIR) { - r = (struct cfg_redir *)spool_buf; - /* Skip cfg_redir at beginning of buf. */ - spool_buf = &spool_buf[SOF_REDIR]; - space = SOF_REDIR; - len -= SOF_REDIR; - } else - goto nospace; + r = (struct cfg_redir *)buf; r->mode = REDIR_PORT; + /* Skip cfg_redir at beginning of buf. */ + buf = &buf[sizeof(struct cfg_redir)]; + space = sizeof(struct cfg_redir); + /* * Extract protocol. */ - if (ac == 0) - errx (EX_DATAERR, "redirect_port: missing protocol"); - r->proto = StrToProto(*av); - protoName = *av; - INC_ARGCV(); + r->proto = StrToProto(**av); + protoName = **av; + (*av)++; (*ac)--; /* * Extract local address. */ - if (ac == 0) - errx (EX_DATAERR, "redirect_port: missing local address"); - - sep = strchr(*av, ','); - /* LSNAT redirection syntax. */ - if (sep) { + if ((sep = strchr(**av, ',')) != NULL) { r->laddr.s_addr = INADDR_NONE; r->lport = ~0; numLocalPorts = 1; - /* Preserve av, copy spool servers to tmp_spool_buf. */ - strncpy(tmp_spool_buf, *av, strlen(*av)+1); - lsnat = 1; + lsnat = **av; } else { /* * The sctp nat does not allow the port numbers to be mapped to @@ -440,40 +429,36 @@ setup_redir_port(char *spool_buf, unsign * in the target port field. */ if (r->proto == IPPROTO_SCTP) { - if (strchr (*av, ':')) + if (strchr(**av, ':')) errx(EX_DATAERR, "redirect_port:" - "port numbers do not change in sctp, so do not " - "specify them as part of the target"); + "port numbers do not change in sctp, so do " + "not specify them as part of the target"); else - StrToAddr(*av, &r->laddr); + StrToAddr(**av, &r->laddr); } else { - if (StrToAddrAndPortRange (*av, &r->laddr, protoName, - &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + if (StrToAddrAndPortRange(**av, &r->laddr, protoName, + &portRange) != 0) + errx(EX_DATAERR, "redirect_port: " "invalid local port range"); r->lport = GETLOPORT(portRange); numLocalPorts = GETNUMPORTS(portRange); } } - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract public port and optionally address. */ - if (ac == 0) - errx (EX_DATAERR, "redirect_port: missing public port"); - - sep = strchr (*av, ':'); - if (sep) { - if (StrToAddrAndPortRange (*av, &r->paddr, protoName, + if ((sep = strchr(**av, ':')) != NULL) { + if (StrToAddrAndPortRange(**av, &r->paddr, protoName, &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + errx(EX_DATAERR, "redirect_port: " "invalid public port range"); } else { r->paddr.s_addr = INADDR_ANY; - if (StrToPortRange (*av, protoName, &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + if (StrToPortRange(**av, protoName, &portRange) != 0) + errx(EX_DATAERR, "redirect_port: " "invalid public port range"); } @@ -483,28 +468,27 @@ setup_redir_port(char *spool_buf, unsign r->lport = r->pport; } r->pport_cnt = GETNUMPORTS(portRange); - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract remote address and optionally port. */ /* - * NB: isalpha(**av) => we've to check that next parameter is really an + * NB: isdigit(**av) => we've to check that next parameter is really an * option for this redirect entry, else stop here processing arg[cv]. */ - if (ac != 0 && !isalpha(**av)) { - sep = strchr (*av, ':'); - if (sep) { - if (StrToAddrAndPortRange (*av, &r->raddr, protoName, + if (*ac != 0 && isdigit(***av)) { + if ((sep = strchr(**av, ':')) != NULL) { + if (StrToAddrAndPortRange(**av, &r->raddr, protoName, &portRange) != 0) - errx(EX_DATAERR, "redirect_port:" + errx(EX_DATAERR, "redirect_port: " "invalid remote port range"); } else { SETLOPORT(portRange, 0); SETNUMPORTS(portRange, 1); - StrToAddr (*av, &r->raddr); + StrToAddr(**av, &r->raddr); } - INC_ARGCV(); + (*av)++; (*ac)--; } else { SETLOPORT(portRange, 0); SETNUMPORTS(portRange, 1); @@ -517,7 +501,7 @@ setup_redir_port(char *spool_buf, unsign * Make sure port ranges match up, then add the redirect ports. */ if (numLocalPorts != r->pport_cnt) - errx(EX_DATAERR, "redirect_port:" + errx(EX_DATAERR, "redirect_port: " "port ranges must be equal in size"); /* Remote port range is allowed to be '0' which means all ports. */ @@ -526,20 +510,18 @@ setup_redir_port(char *spool_buf, unsign errx(EX_DATAERR, "redirect_port: remote port must" "be 0 or equal to local port range in size"); - /* - * Setup LSNAT server pool. - */ - if (lsnat) { - sep = strtok(tmp_spool_buf, ","); + /* Setup LSNAT server pool. */ + if (lsnat != NULL) { + struct cfg_spool *spool; + + sep = strtok(lsnat, ","); while (sep != NULL) { - tmp = (struct cfg_spool *)spool_buf; - if (len < SOF_SPOOL) - goto nospace; - len -= SOF_SPOOL; - space += SOF_SPOOL; + spool = (struct cfg_spool *)buf; + space += sizeof(struct cfg_spool); /* - * The sctp nat does not allow the port numbers to be mapped to new port numbers - * Therefore, no ports are to be specified in the target port field + * The sctp nat does not allow the port numbers to + * be mapped to new port numbers. Therefore, no ports + * are to be specified in the target port field. */ if (r->proto == IPPROTO_SCTP) { if (strchr (sep, ':')) { @@ -548,11 +530,11 @@ setup_redir_port(char *spool_buf, unsign "sctp, so do not specify them as " "part of the target"); } else { - StrToAddr(sep, &tmp->addr); - tmp->port = r->pport; + StrToAddr(sep, &spool->addr); + spool->port = r->pport; } } else { - if (StrToAddrAndPortRange(sep, &tmp->addr, + if (StrToAddrAndPortRange(sep, &spool->addr, protoName, &portRange) != 0) errx(EX_DATAERR, "redirect_port:" "invalid local port range"); @@ -560,88 +542,73 @@ setup_redir_port(char *spool_buf, unsign errx(EX_DATAERR, "redirect_port: " "local port must be single in " "this context"); - tmp->port = GETLOPORT(portRange); + spool->port = GETLOPORT(portRange); } r->spool_cnt++; /* Point to the next possible cfg_spool. */ - spool_buf = &spool_buf[SOF_SPOOL]; + buf = &buf[sizeof(struct cfg_spool)]; sep = strtok(NULL, ","); } } + return (space); -nospace: - errx(EX_DATAERR, "redirect_port: buf is too small\n"); } static int -setup_redir_proto(char *spool_buf, unsigned int len, - int *_ac, char ***_av) +setup_redir_proto(char *buf, int *ac, char ***av) { - char **av; - int ac, space; - struct protoent *protoent; struct cfg_redir *r; + struct protoent *protoent; + size_t space; - av = *_av; - ac = *_ac; - if (len >= SOF_REDIR) { - r = (struct cfg_redir *)spool_buf; - /* Skip cfg_redir at beginning of buf. */ - spool_buf = &spool_buf[SOF_REDIR]; - space = SOF_REDIR; - len -= SOF_REDIR; - } else - goto nospace; + r = (struct cfg_redir *)buf; r->mode = REDIR_PROTO; + /* Skip cfg_redir at beginning of buf. */ + buf = &buf[sizeof(struct cfg_redir)]; + space = sizeof(struct cfg_redir); + /* * Extract protocol. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_proto: missing protocol"); - - protoent = getprotobyname(*av); + protoent = getprotobyname(**av); if (protoent == NULL) - errx(EX_DATAERR, "redirect_proto: unknown protocol %s", *av); + errx(EX_DATAERR, "redirect_proto: unknown protocol %s", **av); else r->proto = protoent->p_proto; - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract local address. */ - if (ac == 0) - errx(EX_DATAERR, "redirect_proto: missing local address"); - else - StrToAddr(*av, &r->laddr); + StrToAddr(**av, &r->laddr); - INC_ARGCV(); + (*av)++; (*ac)--; /* * Extract optional public address. */ - if (ac == 0) { + if (*ac == 0) { r->paddr.s_addr = INADDR_ANY; r->raddr.s_addr = INADDR_ANY; } else { /* see above in setup_redir_port() */ - if (!isalpha(**av)) { - StrToAddr(*av, &r->paddr); - INC_ARGCV(); + if (isdigit(***av)) { + StrToAddr(**av, &r->paddr); + (*av)++; (*ac)--; /* * Extract optional remote address. */ /* see above in setup_redir_port() */ - if (ac!=0 && !isalpha(**av)) { - StrToAddr(*av, &r->raddr); - INC_ARGCV(); + if (*ac != 0 && isdigit(***av)) { + StrToAddr(**av, &r->raddr); + (*av)++; (*ac)--; } } } + return (space); -nospace: - errx(EX_DATAERR, "redirect_proto: buf is too small\n"); } static void @@ -763,30 +730,103 @@ void ipfw_config_nat(int ac, char **av) { struct cfg_nat *n; /* Nat instance configuration. */ - int i, len, off, tok; - char *id, buf[NAT_BUF_LEN]; /* Buffer for serialized data. */ - - len = NAT_BUF_LEN; - /* Offset in buf: save space for n at the beginning. */ - off = sizeof(*n); - memset(buf, 0, sizeof(buf)); - n = (struct cfg_nat *)buf; + int i, off, tok, ac1; + char *id, *buf, **av1, *end; + size_t len; - av++; ac--; + av++; + ac--; /* Nat id. */ - if (ac && isdigit(**av)) { - id = *av; - i = atoi(*av); - ac--; av++; - n->id = i; - } else + if (ac == 0) errx(EX_DATAERR, "missing nat id"); + id = *av; + i = (int)strtol(id, &end, 0); + if (i <= 0 || *end != '\0') + errx(EX_DATAERR, "illegal nat id: %s", id); + av++; + ac--; if (ac == 0) errx(EX_DATAERR, "missing option"); + len = sizeof(struct cfg_nat); + ac1 = ac; + av1 = av; + while (ac1 > 0) { + tok = match_token(nat_params, *av1); + ac1--; + av1++; + switch (tok) { + case TOK_IP: + case TOK_IF: + ac1--; + av1++; + break; + case TOK_ALOG: + case TOK_DENY_INC: + case TOK_SAME_PORTS: + case TOK_UNREG_ONLY: + case TOK_RESET_ADDR: + case TOK_ALIAS_REV: + case TOK_PROXY_ONLY: + break; + case TOK_REDIR_ADDR: + if (ac1 < 2) + errx(EX_DATAERR, "redirect_addr: " + "not enough arguments"); + len += estimate_redir_addr(&ac1, &av1); + av1 += 2; + ac1 -= 2; + break; + case TOK_REDIR_PORT: + if (ac1 < 3) + errx(EX_DATAERR, "redirect_port: " + "not enough arguments"); + av1++; + ac1--; + len += estimate_redir_port(&ac1, &av1); + av1 += 2; + ac1 -= 2; + /* Skip optional remoteIP/port */ + if (ac1 != 0 && isdigit(**av1)) { + av1++; + ac1--; + } + break; + case TOK_REDIR_PROTO: + if (ac1 < 2) + errx(EX_DATAERR, "redirect_proto: " + "not enough arguments"); + len += sizeof(struct cfg_redir); + av1 += 2; + ac1 -= 2; + /* Skip optional remoteIP/port */ + if (ac1 != 0 && isdigit(**av1)) { + av1++; + ac1--; + } + if (ac1 != 0 && isdigit(**av1)) { + av1++; + ac1--; + } + break; + default: + errx(EX_DATAERR, "unrecognised option ``%s''", av1[-1]); + } + } + + if ((buf = malloc(len)) == NULL) + errx(EX_OSERR, "malloc failed"); + + /* Offset in buf: save space for n at the beginning. */ + off = sizeof(*n); + memset(buf, 0, len); + n = (struct cfg_nat *)buf; + n->id = i; + while (ac > 0) { tok = match_token(nat_params, *av); - ac--; av++; + ac--; + av++; switch (tok) { case TOK_IP: if (ac == 0) @@ -794,13 +834,15 @@ ipfw_config_nat(int ac, char **av) if (!inet_aton(av[0], &(n->ip))) errx(EX_DATAERR, "bad ip address ``%s''", av[0]); - ac--; av++; + ac--; + av++; break; case TOK_IF: if (ac == 0) errx(EX_DATAERR, "missing option"); set_addr_dynamic(av[0], n); - ac--; av++; + ac--; + av++; break; case TOK_ALOG: n->mode |= PKT_ALIAS_LOG; @@ -832,21 +874,18 @@ ipfw_config_nat(int ac, char **av) case TOK_REDIR_PROTO: switch (tok) { case TOK_REDIR_ADDR: - i = setup_redir_addr(&buf[off], len, &ac, &av); + i = setup_redir_addr(&buf[off], &ac, &av); break; case TOK_REDIR_PORT: - i = setup_redir_port(&buf[off], len, &ac, &av); + i = setup_redir_port(&buf[off], &ac, &av); break; case TOK_REDIR_PROTO: - i = setup_redir_proto(&buf[off], len, &ac, &av); + i = setup_redir_proto(&buf[off], &ac, &av); break; } n->redir_cnt++; off += i; - len -= i; break; - default: - errx(EX_DATAERR, "unrecognised option ``%s''", av[-1]); } } @@ -879,7 +918,8 @@ ipfw_show_nat(int ac, char **av) data = NULL; frule = 0; lrule = IPFW_DEFAULT_RULE; /* max ipfw rule number */ - ac--; av++; + ac--; + av++; if (co.test_only) return; _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" State-Changed-From-To: patched->closed State-Changed-By: glebius State-Changed-When: Fri Jul 8 14:31:57 UTC 2011 State-Changed-Why: Merged to stable/8. http://www.freebsd.org/cgi/query-pr.cgi?pr=143653 From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: kern/143653: commit references a PR Date: Fri, 8 Jul 2011 14:30:18 +0000 (UTC) Author: glebius Date: Fri Jul 8 14:30:06 2011 New Revision: 223872 URL: http://svn.freebsd.org/changeset/base/223872 Log: Merge from head/ 220800,220837,220914: Log: LibAliasInit() should allocate memory with M_WAITOK flag. Modify it and its callers. Log: - Rewrite functions that copyin/out NAT configuration, so that they calculate required memory size dynamically. - Fix races on chain re-lock. - Introduce new field to ip_fw_chain - generation count. Now utilized only in the NAT configuration, but can be utilized wider in ipfw. - Get rid of NAT_BUF_LEN in ip_fw.h PR: kern/143653 Modified: stable/8/sys/netgraph/ng_nat.c stable/8/sys/netinet/ip_fw.h stable/8/sys/netinet/ipfw/ip_fw_nat.c stable/8/sys/netinet/ipfw/ip_fw_private.h stable/8/sys/netinet/libalias/alias_db.c Directory Properties: stable/8/sys/ (props changed) stable/8/sys/amd64/include/xen/ (props changed) stable/8/sys/cddl/contrib/opensolaris/ (props changed) stable/8/sys/contrib/dev/acpica/ (props changed) stable/8/sys/contrib/pf/ (props changed) Modified: stable/8/sys/netgraph/ng_nat.c ============================================================================== --- stable/8/sys/netgraph/ng_nat.c Fri Jul 8 14:26:42 2011 (r223871) +++ stable/8/sys/netgraph/ng_nat.c Fri Jul 8 14:30:06 2011 (r223872) @@ -280,10 +280,6 @@ ng_nat_constructor(node_p node) /* Init aliasing engine. */ priv->lib = LibAliasInit(NULL); - if (priv->lib == NULL) { - free(priv, M_NETGRAPH); - return (ENOMEM); - } /* Set same ports on. */ (void )LibAliasSetMode(priv->lib, PKT_ALIAS_SAME_PORTS, Modified: stable/8/sys/netinet/ip_fw.h ============================================================================== --- stable/8/sys/netinet/ip_fw.h Fri Jul 8 14:26:42 2011 (r223871) +++ stable/8/sys/netinet/ip_fw.h Fri Jul 8 14:30:06 2011 (r223872) @@ -380,8 +380,6 @@ struct cfg_redir { }; #endif -#define NAT_BUF_LEN 1024 - #ifdef IPFW_INTERNAL /* Nat configuration data struct. */ struct cfg_nat { Modified: stable/8/sys/netinet/ipfw/ip_fw_nat.c ============================================================================== --- stable/8/sys/netinet/ipfw/ip_fw_nat.c Fri Jul 8 14:26:42 2011 (r223871) +++ stable/8/sys/netinet/ipfw/ip_fw_nat.c Fri Jul 8 14:30:06 2011 (r223872) @@ -138,7 +138,7 @@ del_redir_spool_cfg(struct cfg_nat *n, s } } -static int +static void add_redir_spool_cfg(char *buf, struct cfg_nat *ptr) { struct cfg_redir *r, *ser_r; @@ -199,7 +199,6 @@ add_redir_spool_cfg(char *buf, struct cf /* And finally hook this redir entry. */ LIST_INSERT_HEAD(&ptr->redir_chain, r, _next); } - return (1); } static int @@ -354,59 +353,57 @@ lookup_nat(struct nat_list *l, int nat_i static int ipfw_nat_cfg(struct sockopt *sopt) { - struct cfg_nat *ptr, *ser_n; + struct cfg_nat *cfg, *ptr; char *buf; struct ip_fw_chain *chain = &V_layer3_chain; + size_t len; + int gencnt, error = 0; - buf = malloc(NAT_BUF_LEN, M_IPFW, M_WAITOK | M_ZERO); - sooptcopyin(sopt, buf, NAT_BUF_LEN, sizeof(struct cfg_nat)); - ser_n = (struct cfg_nat *)buf; + len = sopt->sopt_valsize; + buf = malloc(len, M_TEMP, M_WAITOK | M_ZERO); + if ((error = sooptcopyin(sopt, buf, len, sizeof(struct cfg_nat))) != 0) + goto out; + + cfg = (struct cfg_nat *)buf; + if (cfg->id < 0) { + error = EINVAL; + goto out; + } - /* check valid parameter ser_n->id > 0 ? */ /* * Find/create nat rule. */ IPFW_WLOCK(chain); - ptr = lookup_nat(&chain->nat, ser_n->id); + gencnt = chain->gencnt; + ptr = lookup_nat(&chain->nat, cfg->id); if (ptr == NULL) { + IPFW_WUNLOCK(chain); /* New rule: allocate and init new instance. */ - ptr = malloc(sizeof(struct cfg_nat), - M_IPFW, M_NOWAIT | M_ZERO); - if (ptr == NULL) { - IPFW_WUNLOCK(chain); - free(buf, M_IPFW); - return (ENOSPC); - } + ptr = malloc(sizeof(struct cfg_nat), M_IPFW, M_WAITOK | M_ZERO); ptr->lib = LibAliasInit(NULL); - if (ptr->lib == NULL) { - IPFW_WUNLOCK(chain); - free(ptr, M_IPFW); - free(buf, M_IPFW); - return (EINVAL); - } LIST_INIT(&ptr->redir_chain); } else { - /* Entry already present: temporarly unhook it. */ + /* Entry already present: temporarily unhook it. */ LIST_REMOVE(ptr, _next); - flush_nat_ptrs(chain, ser_n->id); + flush_nat_ptrs(chain, cfg->id); + IPFW_WUNLOCK(chain); } - IPFW_WUNLOCK(chain); /* * Basic nat configuration. */ - ptr->id = ser_n->id; + ptr->id = cfg->id; /* * 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; - ptr->redir_cnt = ser_n->redir_cnt; - ptr->mode = ser_n->mode; - LibAliasSetMode(ptr->lib, ser_n->mode, ser_n->mode); + ptr->ip = cfg->ip; + ptr->redir_cnt = cfg->redir_cnt; + ptr->mode = cfg->mode; + LibAliasSetMode(ptr->lib, cfg->mode, cfg->mode); LibAliasSetAddress(ptr->lib, ptr->ip); - memcpy(ptr->if_name, ser_n->if_name, IF_NAMESIZE); + memcpy(ptr->if_name, cfg->if_name, IF_NAMESIZE); /* * Redir and LSNAT configuration. @@ -415,11 +412,19 @@ ipfw_nat_cfg(struct sockopt *sopt) del_redir_spool_cfg(ptr, &ptr->redir_chain); /* Add new entries. */ add_redir_spool_cfg(&buf[(sizeof(struct cfg_nat))], ptr); - free(buf, M_IPFW); + IPFW_WLOCK(chain); + /* Extra check to avoid race with another ipfw_nat_cfg() */ + if (gencnt != chain->gencnt && + ((cfg = lookup_nat(&chain->nat, ptr->id)) != NULL)) + LIST_REMOVE(cfg, _next); LIST_INSERT_HEAD(&chain->nat, ptr, _next); + chain->gencnt++; IPFW_WUNLOCK(chain); - return (0); + +out: + free(buf, M_TEMP); + return (error); } static int @@ -449,52 +454,61 @@ ipfw_nat_del(struct sockopt *sopt) static int ipfw_nat_get_cfg(struct sockopt *sopt) { - uint8_t *data; + struct ip_fw_chain *chain = &V_layer3_chain; struct cfg_nat *n; struct cfg_redir *r; struct cfg_spool *s; - int nat_cnt, off; - struct ip_fw_chain *chain; - int err = ENOSPC; + char *data; + int gencnt, nat_cnt, len, error; - chain = &V_layer3_chain; nat_cnt = 0; - off = sizeof(nat_cnt); + len = sizeof(nat_cnt); - data = malloc(NAT_BUF_LEN, M_IPFW, M_WAITOK | M_ZERO); IPFW_RLOCK(chain); - /* Serialize all the data. */ +retry: + gencnt = chain->gencnt; + /* Estimate memory amount */ LIST_FOREACH(n, &chain->nat, _next) { nat_cnt++; - if (off + SOF_NAT >= NAT_BUF_LEN) - goto nospace; - bcopy(n, &data[off], SOF_NAT); - off += SOF_NAT; + len += sizeof(struct cfg_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; + len += sizeof(struct cfg_redir); + LIST_FOREACH(s, &r->spool_chain, _next) + len += sizeof(struct cfg_spool); + } + } + IPFW_RUNLOCK(chain); + + data = malloc(len, M_TEMP, M_WAITOK | M_ZERO); + bcopy(&nat_cnt, data, sizeof(nat_cnt)); + + nat_cnt = 0; + len = sizeof(nat_cnt); + + IPFW_RLOCK(chain); + if (gencnt != chain->gencnt) { + free(data, M_TEMP); + goto retry; + } + /* Serialize all the data. */ + LIST_FOREACH(n, &chain->nat, _next) { + bcopy(n, &data[len], sizeof(struct cfg_nat)); + len += sizeof(struct cfg_nat); + LIST_FOREACH(r, &n->redir_chain, _next) { + bcopy(r, &data[len], sizeof(struct cfg_redir)); + len += sizeof(struct cfg_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; + bcopy(s, &data[len], sizeof(struct cfg_spool)); + len += sizeof(struct cfg_spool); } } } - err = 0; /* all good */ -nospace: IPFW_RUNLOCK(chain); - 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 (err); + + error = sooptcopyout(sopt, data, len); + free(data, M_TEMP); + + return (error); } static int Modified: stable/8/sys/netinet/ipfw/ip_fw_private.h ============================================================================== --- stable/8/sys/netinet/ipfw/ip_fw_private.h Fri Jul 8 14:26:42 2011 (r223871) +++ stable/8/sys/netinet/ipfw/ip_fw_private.h Fri Jul 8 14:30:06 2011 (r223872) @@ -225,6 +225,7 @@ struct ip_fw_chain { struct rwlock uh_lock; /* lock for upper half */ #endif uint32_t id; /* ruleset id */ + uint32_t gencnt; /* generation count */ }; struct sockopt; /* used by tcp_var.h */ Modified: stable/8/sys/netinet/libalias/alias_db.c ============================================================================== --- stable/8/sys/netinet/libalias/alias_db.c Fri Jul 8 14:26:42 2011 (r223871) +++ stable/8/sys/netinet/libalias/alias_db.c Fri Jul 8 14:30:06 2011 (r223872) @@ -2492,9 +2492,14 @@ LibAliasInit(struct libalias *la) #endif if (la == NULL) { +#ifdef _KERNEL +#undef malloc /* XXX: ugly */ + la = malloc(sizeof *la, M_ALIAS, M_WAITOK | M_ZERO); +#else la = calloc(sizeof *la, 1); if (la == NULL) return (la); +#endif #ifndef _KERNEL /* kernel cleans up on module unload */ if (LIST_EMPTY(&instancehead)) _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" >Unformatted: