From b1be6dab1b6ffe015d7aeb8a6d08021ebdf00fcf Mon Sep 17 00:00:00 2001 From: luigi Date: Tue, 22 Dec 2009 13:53:34 +0000 Subject: some mostly cosmetic changes in preparation for upcoming work: + in many places, replace &V_layer3_chain with a local variable chain; + bring the counter of rules and static_len within ip_fw_chain replacing static variables; + remove some spurious comments and extern declaration; + document which lock protects certain data structures --- sys/netinet/ipfw/ip_fw2.c | 93 ++++++++++++++++++++-------------------- sys/netinet/ipfw/ip_fw_private.h | 8 +++- sys/netinet/ipfw/ip_fw_sockopt.c | 77 ++++++++++++++------------------- sys/netinet/ipfw/ip_fw_table.c | 3 ++ 4 files changed, 89 insertions(+), 92 deletions(-) diff --git a/sys/netinet/ipfw/ip_fw2.c b/sys/netinet/ipfw/ip_fw2.c index 32cd548..ca998c1 100644 --- a/sys/netinet/ipfw/ip_fw2.c +++ b/sys/netinet/ipfw/ip_fw2.c @@ -1,5 +1,5 @@ /*- - * Copyright (c) 2002 Luigi Rizzo, Universita` di Pisa + * Copyright (c) 2002-2009 Luigi Rizzo, Universita` di Pisa * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -131,12 +131,10 @@ VNET_DEFINE(u_int32_t, set_disable); #define V_set_disable VNET(set_disable) VNET_DEFINE(int, fw_verbose); -//#define V_verbose_limit VNET(verbose_limit) /* counter for ipfw_log(NULL...) */ VNET_DEFINE(u_int64_t, norule_counter); VNET_DEFINE(int, verbose_limit); - /* layer3_chain contains the list of rules for layer 3 */ VNET_DEFINE(struct ip_fw_chain, layer3_chain); @@ -147,8 +145,6 @@ ipfw_nat_cfg_t *ipfw_nat_del_ptr; ipfw_nat_cfg_t *ipfw_nat_get_cfg_ptr; ipfw_nat_cfg_t *ipfw_nat_get_log_ptr; -extern int ipfw_chg_hook(SYSCTL_HANDLER_ARGS); - #ifdef SYSCTL_NODE SYSCTL_NODE(_net_inet_ip, OID_AUTO, fw, CTLFLAG_RW, 0, "Firewall"); SYSCTL_VNET_INT(_net_inet_ip_fw, OID_AUTO, one_pass, @@ -173,6 +169,9 @@ SYSCTL_INT(_net_inet_ip_fw, OID_AUTO, default_to_accept, CTLFLAG_RDTUN, &default_to_accept, 0, "Make the default rule accept all packets."); TUNABLE_INT("net.inet.ip.fw.default_to_accept", &default_to_accept); +SYSCTL_VNET_INT(_net_inet_ip_fw, OID_AUTO, static_count, + CTLFLAG_RD, &VNET_NAME(layer3_chain.n_rules), 0, + "Number of static rules"); #ifdef INET6 SYSCTL_DECL(_net_inet6_ip6); @@ -739,6 +738,17 @@ check_uidgid(ipfw_insn_u32 *insn, int proto, struct ifnet *oif, } /* + * Helper function to write the matching rule into args + */ +static inline void +set_match(struct ip_fw_args *args, struct ip_fw *f, struct ip_fw_chain *chain) +{ + args->rule = f; + args->rule_id = f->id; + args->chain_id = chain->id; +} + +/* * The main check routine for the firewall. * * All arguments are in args so we can modify them and return them @@ -1942,13 +1952,9 @@ do { \ case O_PIPE: case O_QUEUE: - args->rule = f; /* report matching rule */ - args->rule_id = f->id; - args->chain_id = chain->id; - if (cmd->arg1 == IP_FW_TABLEARG) - args->cookie = tablearg; - else - args->cookie = cmd->arg1; + set_match(args, f, chain); + args->cookie = (cmd->arg1 == IP_FW_TABLEARG) ? + tablearg : cmd->arg1; retval = IP_FW_DUMMYNET; l = 0; /* exit inner loop */ done = 1; /* exit outer loop */ @@ -2077,13 +2083,9 @@ do { \ case O_NETGRAPH: case O_NGTEE: - args->rule = f; /* report matching rule */ - args->rule_id = f->id; - args->chain_id = chain->id; - if (cmd->arg1 == IP_FW_TABLEARG) - args->cookie = tablearg; - else - args->cookie = cmd->arg1; + set_match(args, f, chain); + args->cookie = (cmd->arg1 == IP_FW_TABLEARG) ? + tablearg : cmd->arg1; retval = (cmd->opcode == O_NETGRAPH) ? IP_FW_NETGRAPH : IP_FW_NGTEE; l = 0; /* exit inner loop */ @@ -2106,14 +2108,12 @@ do { \ struct cfg_nat *t; int nat_id; - args->rule = f; /* Report matching rule. */ - args->rule_id = f->id; - args->chain_id = chain->id; + set_match(args, f, chain); t = ((ipfw_insn_nat *)cmd)->nat; if (t == NULL) { nat_id = (cmd->arg1 == IP_FW_TABLEARG) ? tablearg : cmd->arg1; - t = (*lookup_nat_ptr)(&V_layer3_chain.nat, nat_id); + t = (*lookup_nat_ptr)(&chain->nat, nat_id); if (t == NULL) { retval = IP_FW_DENY; @@ -2175,9 +2175,7 @@ do { \ else ip->ip_sum = in_cksum(m, hlen); retval = IP_FW_REASS; - args->rule = f; - args->rule_id = f->id; - args->chain_id = chain->id; + set_match(args, f, chain); } done = 1; /* exit outer loop */ break; @@ -2311,8 +2309,13 @@ vnet_ipfw_init(const void *unused) { int error; struct ip_fw default_rule; + struct ip_fw_chain *chain; + + chain = &V_layer3_chain; /* First set up some values that are compile time options */ + V_autoinc_step = 100; /* bounded to 1..1000 in add_rule() */ + V_fw_deny_unknown_exthdrs = 1; #ifdef IPFIREWALL_VERBOSE V_fw_verbose = 1; #endif @@ -2320,20 +2323,17 @@ vnet_ipfw_init(const void *unused) V_verbose_limit = IPFIREWALL_VERBOSE_LIMIT; #endif - error = ipfw_init_tables(&V_layer3_chain); + error = ipfw_init_tables(chain); if (error) { panic("init_tables"); /* XXX Marko fix this ! */ } #ifdef IPFIREWALL_NAT - LIST_INIT(&V_layer3_chain.nat); + LIST_INIT(&chain->nat); #endif - V_autoinc_step = 100; /* bounded to 1..1000 in add_rule() */ - - V_fw_deny_unknown_exthdrs = 1; - V_layer3_chain.rules = NULL; - IPFW_LOCK_INIT(&V_layer3_chain); + chain->rules = NULL; + IPFW_LOCK_INIT(chain); bzero(&default_rule, sizeof default_rule); default_rule.act_ofs = 0; @@ -2342,17 +2342,17 @@ vnet_ipfw_init(const void *unused) default_rule.set = RESVD_SET; default_rule.cmd[0].len = 1; default_rule.cmd[0].opcode = default_to_accept ? O_ACCEPT : O_DENY; - error = ipfw_add_rule(&V_layer3_chain, &default_rule); + error = ipfw_add_rule(chain, &default_rule); if (error != 0) { printf("ipfw2: error %u initializing default rule " "(support disabled)\n", error); - IPFW_LOCK_DESTROY(&V_layer3_chain); + IPFW_LOCK_DESTROY(chain); printf("leaving ipfw_iattach (1) with error %d\n", error); return (error); } - V_layer3_chain.default_rule = V_layer3_chain.rules; + chain->default_rule = chain->rules; ipfw_dyn_init(); @@ -2386,6 +2386,7 @@ static int vnet_ipfw_uninit(const void *unused) { struct ip_fw *reap; + struct ip_fw_chain *chain = &V_layer3_chain; V_ipfw_vnet_ready = 0; /* tell new callers to go away */ /* @@ -2400,21 +2401,21 @@ vnet_ipfw_uninit(const void *unused) V_ip_fw_chk_ptr = NULL; V_ip_fw_ctl_ptr = NULL; - IPFW_WLOCK(&V_layer3_chain); + IPFW_WLOCK(chain); /* We wait on the wlock here until the last user leaves */ - IPFW_WUNLOCK(&V_layer3_chain); - IPFW_WLOCK(&V_layer3_chain); + IPFW_WUNLOCK(chain); + IPFW_WLOCK(chain); ipfw_dyn_uninit(0); /* run the callout_drain */ - ipfw_flush_tables(&V_layer3_chain); - V_layer3_chain.reap = NULL; - ipfw_free_chain(&V_layer3_chain, 1 /* kill default rule */); - reap = V_layer3_chain.reap; - V_layer3_chain.reap = NULL; - IPFW_WUNLOCK(&V_layer3_chain); + ipfw_flush_tables(chain); + chain->reap = NULL; + ipfw_free_chain(chain, 1 /* kill default rule */); + reap = chain->reap; + chain->reap = NULL; + IPFW_WUNLOCK(chain); if (reap != NULL) ipfw_reap_rules(reap); - IPFW_LOCK_DESTROY(&V_layer3_chain); + IPFW_LOCK_DESTROY(chain); ipfw_dyn_uninit(1); /* free the remaining parts */ return 0; } diff --git a/sys/netinet/ipfw/ip_fw_private.h b/sys/netinet/ipfw/ip_fw_private.h index 095de66..9c67d20 100644 --- a/sys/netinet/ipfw/ip_fw_private.h +++ b/sys/netinet/ipfw/ip_fw_private.h @@ -78,9 +78,11 @@ struct ip_fw_args { struct mbuf *m; /* the mbuf chain */ struct ifnet *oif; /* output interface */ struct sockaddr_in *next_hop; /* forward address */ + struct ip_fw *rule; /* matching rule */ - uint32_t rule_id; /* matching rule id */ - uint32_t chain_id; /* ruleset id */ + uint32_t rule_id; /* matching rule id */ + uint32_t chain_id; /* ruleset id */ + struct ether_header *eh; /* for bridged packets */ struct ipfw_flow_id f_id; /* grabbed from IP header */ @@ -174,6 +176,8 @@ struct ip_fw_chain { struct ip_fw *rules; /* list of rules */ struct ip_fw *reap; /* list of rules to reap */ struct ip_fw *default_rule; + int n_rules; /* number of static rules */ + int static_len; /* total len of static rules */ LIST_HEAD(nat_list, cfg_nat) nat; /* list of nat entries */ struct radix_node_head *tables[IPFW_TABLES_MAX]; struct rwlock rwmtx; diff --git a/sys/netinet/ipfw/ip_fw_sockopt.c b/sys/netinet/ipfw/ip_fw_sockopt.c index d418881..49618da 100644 --- a/sys/netinet/ipfw/ip_fw_sockopt.c +++ b/sys/netinet/ipfw/ip_fw_sockopt.c @@ -77,23 +77,9 @@ __FBSDID("$FreeBSD$"); MALLOC_DEFINE(M_IPFW, "IpFw/IpAcct", "IpFw/IpAcct chain's"); /* - * static variables followed by global ones + * static variables followed by global ones (none in this file) */ -static VNET_DEFINE(u_int32_t, static_count); /* # of static rules */ -#define V_static_count VNET(static_count) - -static VNET_DEFINE(u_int32_t, static_len); /* bytes of static rules */ -#define V_static_len VNET(static_len) - -#ifdef SYSCTL_NODE -SYSCTL_DECL(_net_inet_ip_fw); -SYSCTL_VNET_INT(_net_inet_ip_fw, OID_AUTO, static_count, - CTLFLAG_RD, &VNET_NAME(static_count), 0, - "Number of static rules"); - -#endif /* SYSCTL_NODE */ - /* * When a rule is added/deleted, clear the next_rule pointers in all rules. * These will be reconstructed on the fly as packets are matched. @@ -187,11 +173,9 @@ ipfw_add_rule(struct ip_fw_chain *chain, struct ip_fw *input_rule) /* chain->id incremented inside flush_rule_ptrs() */ rule->id = chain->id; done: - V_static_count++; - V_static_len += l; + chain->n_rules++; + chain->static_len += l; IPFW_WUNLOCK(chain); - DEB(printf("ipfw: installed rule %d, static count now %d\n", - rule->rulenum, V_static_count);) return (0); } @@ -218,8 +202,8 @@ remove_rule(struct ip_fw_chain *chain, struct ip_fw *rule, chain->rules = n; else prev->next = n; - V_static_count--; - V_static_len -= l; + chain->n_rules--; + chain->static_len -= l; rule->next = chain->reap; chain->reap = rule; @@ -839,6 +823,7 @@ ipfw_ctl(struct sockopt *sopt) int error; size_t size; struct ip_fw *buf, *rule; + struct ip_fw_chain *chain; u_int32_t rulenum[2]; error = priv_check(sopt->sopt_td, PRIV_NETINET_IPFW); @@ -856,6 +841,7 @@ ipfw_ctl(struct sockopt *sopt) return (error); } + chain = &V_layer3_chain; error = 0; switch (sopt->sopt_name) { @@ -871,7 +857,7 @@ ipfw_ctl(struct sockopt *sopt) * change between calculating the size and returning the * data in which case we'll just return what fits. */ - size = V_static_len; /* size of static rules */ + size = chain->static_len; /* size of static rules */ size += ipfw_dyn_len(); if (size >= sopt->sopt_valsize) @@ -883,7 +869,7 @@ ipfw_ctl(struct sockopt *sopt) */ buf = malloc(size, M_TEMP, M_WAITOK); error = sooptcopyout(sopt, buf, - ipfw_getrules(&V_layer3_chain, buf, size)); + ipfw_getrules(chain, buf, size)); free(buf, M_TEMP); break; @@ -901,10 +887,10 @@ ipfw_ctl(struct sockopt *sopt) * the old list without the need for a lock. */ - IPFW_WLOCK(&V_layer3_chain); - ipfw_free_chain(&V_layer3_chain, 0 /* keep default rule */); - rule = V_layer3_chain.reap; - IPFW_WUNLOCK(&V_layer3_chain); + IPFW_WLOCK(chain); + ipfw_free_chain(chain, 0 /* keep default rule */); + rule = chain->reap; + IPFW_WUNLOCK(chain); ipfw_reap_rules(rule); break; @@ -915,7 +901,7 @@ ipfw_ctl(struct sockopt *sopt) if (error == 0) error = check_ipfw_struct(rule, sopt->sopt_valsize); if (error == 0) { - error = ipfw_add_rule(&V_layer3_chain, rule); + error = ipfw_add_rule(chain, rule); size = RULESIZE(rule); if (!error && sopt->sopt_dir == SOPT_GET) error = sooptcopyout(sopt, rule, size); @@ -941,13 +927,14 @@ ipfw_ctl(struct sockopt *sopt) if (error) break; size = sopt->sopt_valsize; - if (size == sizeof(u_int32_t)) /* delete or reassign */ - error = del_entry(&V_layer3_chain, rulenum[0]); - else if (size == 2*sizeof(u_int32_t)) /* set enable/disable */ + if (size == sizeof(u_int32_t) && rulenum[0] != 0) { + /* delete or reassign, locking done in del_entry() */ + error = del_entry(chain, rulenum[0]); + } else if (size == 2*sizeof(u_int32_t)) { /* set enable/disable */ V_set_disable = (V_set_disable | rulenum[0]) & ~rulenum[1] & ~(1<sopt_name == IP_FW_RESETLOG); break; + /*--- TABLE manipulations are protected by the IPFW_LOCK ---*/ case IP_FW_TABLE_ADD: { ipfw_table_entry ent; @@ -972,7 +960,7 @@ ipfw_ctl(struct sockopt *sopt) sizeof(ent), sizeof(ent)); if (error) break; - error = ipfw_add_table_entry(&V_layer3_chain, ent.tbl, + error = ipfw_add_table_entry(chain, ent.tbl, ent.addr, ent.masklen, ent.value); } break; @@ -985,7 +973,7 @@ ipfw_ctl(struct sockopt *sopt) sizeof(ent), sizeof(ent)); if (error) break; - error = ipfw_del_table_entry(&V_layer3_chain, ent.tbl, + error = ipfw_del_table_entry(chain, ent.tbl, ent.addr, ent.masklen); } break; @@ -998,9 +986,9 @@ ipfw_ctl(struct sockopt *sopt) sizeof(tbl), sizeof(tbl)); if (error) break; - IPFW_WLOCK(&V_layer3_chain); - error = ipfw_flush_table(&V_layer3_chain, tbl); - IPFW_WUNLOCK(&V_layer3_chain); + IPFW_WLOCK(chain); + error = ipfw_flush_table(chain, tbl); + IPFW_WUNLOCK(chain); } break; @@ -1011,9 +999,9 @@ ipfw_ctl(struct sockopt *sopt) if ((error = sooptcopyin(sopt, &tbl, sizeof(tbl), sizeof(tbl)))) break; - IPFW_RLOCK(&V_layer3_chain); - error = ipfw_count_table(&V_layer3_chain, tbl, &cnt); - IPFW_RUNLOCK(&V_layer3_chain); + IPFW_RLOCK(chain); + error = ipfw_count_table(chain, tbl, &cnt); + IPFW_RUNLOCK(chain); if (error) break; error = sooptcopyout(sopt, &cnt, sizeof(cnt)); @@ -1037,9 +1025,9 @@ ipfw_ctl(struct sockopt *sopt) } tbl->size = (size - sizeof(*tbl)) / sizeof(ipfw_table_entry); - IPFW_RLOCK(&V_layer3_chain); - error = ipfw_dump_table(&V_layer3_chain, tbl); - IPFW_RUNLOCK(&V_layer3_chain); + IPFW_RLOCK(chain); + error = ipfw_dump_table(chain, tbl); + IPFW_RUNLOCK(chain); if (error) { free(tbl, M_TEMP); break; @@ -1049,6 +1037,7 @@ ipfw_ctl(struct sockopt *sopt) } break; + /*--- NAT operations are protected by the IPFW_LOCK ---*/ case IP_FW_NAT_CFG: if (IPFW_NAT_LOADED) error = ipfw_nat_cfg_ptr(sopt); diff --git a/sys/netinet/ipfw/ip_fw_table.c b/sys/netinet/ipfw/ip_fw_table.c index 0dab070..342085b 100644 --- a/sys/netinet/ipfw/ip_fw_table.c +++ b/sys/netinet/ipfw/ip_fw_table.c @@ -34,6 +34,9 @@ __FBSDID("$FreeBSD$"); * keys are network prefixes (addr/masklen), and values are integers. * As a degenerate case we can interpret keys as 32-bit integers * (with a /32 mask). + * + * The table is protected by the IPFW lock even for manipulation coming + * from userland, because operations are typically fast. */ #if !defined(KLD_MODULE) -- cgit v1.1