From 94dea9059f0ff01c8960a0b84dfc86d2f92fcabe Mon Sep 17 00:00:00 2001 From: glebius Date: Tue, 19 Apr 2011 15:06:33 +0000 Subject: [PATCH] - 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 --- sys/netinet/ip_fw.h | 2 - sys/netinet/ipfw/ip_fw_nat.c | 126 ++++++++++++++++++------------- sys/netinet/ipfw/ip_fw_private.h | 1 + 3 files changed, 74 insertions(+), 55 deletions(-) diff --git a/sys/netinet/ip_fw.h b/sys/netinet/ip_fw.h index fdcc5fd4c1df..06e107c2de61 100644 --- a/sys/netinet/ip_fw.h +++ b/sys/netinet/ip_fw.h @@ -383,8 +383,6 @@ struct cfg_redir { }; #endif -#define NAT_BUF_LEN 1024 - #ifdef IPFW_INTERNAL /* Nat configuration data struct. */ struct cfg_nat { diff --git a/sys/netinet/ipfw/ip_fw_nat.c b/sys/netinet/ipfw/ip_fw_nat.c index f5f1374bd310..596975d80333 100644 --- a/sys/netinet/ipfw/ip_fw_nat.c +++ b/sys/netinet/ipfw/ip_fw_nat.c @@ -138,7 +138,7 @@ del_redir_spool_cfg(struct cfg_nat *n, struct redir_chain *head) } } -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 cfg_nat *ptr) /* 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_id) 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) { - 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 diff --git a/sys/netinet/ipfw/ip_fw_private.h b/sys/netinet/ipfw/ip_fw_private.h index 13df9c93ee8c..16ef46d574e1 100644 --- a/sys/netinet/ipfw/ip_fw_private.h +++ b/sys/netinet/ipfw/ip_fw_private.h @@ -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 */