> === modified file 'firewall/conntrack.c' > --- firewall/conntrack.c 2010-12-13 19:09:27 +0000 > +++ firewall/conntrack.c 2010-12-13 21:28:35 +0000 > @@ -50,6 +52,7 @@ > #include <openssl/dsa.h> > #include <openssl/rsa.h> > #include <sys/time.h> > +#include <linux/netfilter_ipv4.h> If this makes conntrack.c derived work of netfilter/Linux, we have another case of GPL infection here. Not sure. > @@ -319,23 +352,147 @@ > } > > /** > - * Insert an address into a list of addresses. If same address exists > already, > - * the update_id is replaced with the new value. > - * > - * @param addr_list the address list > - * @param addr the address to be added > - * @param upd_id update id > - * > - * @return the address list > - */ > -static struct slist *update_esp_address(struct slist *addr_list, > - const struct in6_addr *addr, > - const uint32_t *upd_id) > -{ > - struct esp_address *esp_addr = get_esp_address(addr_list, addr); > + * Forward this spi/dest combo via iptables rather than userspace > + * dispatching, or delete the corresponding rules. > + * > + * @param esp_tuple Extract SPI and connection info from this tuple. > + * @param dest The newly added address. > + * @param insert Remove rule if false, add it otherwise. > + * @return 0 on success, -1 otherwise. > + * > + * @todo Test rules using userspace_ipsec, Relay, LSI, sys-opp, midauth, > + * light-update and esp_prot configurations. > + * @todo Test with different byte ordering. > + * > + * @see update_esp_address > + * @see free_esp_tuple > + */ > +static int hip_fw_manage_esp_rule(const struct esp_tuple *esp_tuple, > + const struct in6_addr *dest, bool insert) 'const struct esp_tuple *const esp_tuple' and 'const struct in6_addr *const dest', and 'const bool insert' would be even more const correct. > +{ > + int err = 0; > + const char *flag = insert ? "-I" : "-D"; > + const char *table = NULL; > + > + if (hip_userspace_ipsec || prefer_userspace) { > + return 0; > + } > + > + HIP_ASSERT(esp_tuple); > + HIP_ASSERT(dest); It's good to check esp_tuple and dest for NULL pointers before accessing them, however the result of such a NULL pointer access is roughly the same as that of HIP_ASSERT(): the program terminates. That makes the HIP_ASSERT() kind of useless. > +void hip_fw_manage_esp_tuple(const struct esp_tuple *esp_tuple, bool insert) const correctness? > +{ > + struct slist *lst = esp_tuple->dst_addr_list; it would be good to check esp_tuple for NULL > + while (lst) { > + hip_fw_manage_esp_rule(esp_tuple, lst->data, insert); > + lst = lst->next; > + } > +} > + > +/** > + * Insert a destination address into an esp_tuple. If same address exists > already, > + * the update_id is replaced with the new value instead. > + * > + * @param esp_tuple the esp tuple > + * @param addr the address to be added > + * @param upd_id update id > + */ > +static void update_esp_address(struct esp_tuple *esp_tuple, > + const struct in6_addr *addr, > + const uint32_t *upd_id) 'struct esp_tuple *const esp_tuple', 'const struct in6_addr *const addr', and 'const uint32_t *const upd_id' would be even more const correct. Admittedly, I haven't looked into the rest of the code, but seems odd that upd_id is passed around as a pointer because the const says that nothing is written to it. Why not just a uint32_t instead of a pointer? > @@ -581,13 +743,6 @@ > } > tuple->esp_tuples = NULL; > tuple->connection = NULL; > - > - // tuple was not malloced -> no free here > - free(tuple->src_ip); > - tuple->src_ip = NULL; > - > - free(tuple->dst_ip); > - tuple->dst_ip = NULL; Hm, I couldn't find the code change that would make these free()s unnecessary. So why are they removed? > @@ -1956,3 +2088,273 @@ > HIP_DEBUG("get_tuple_by_hits: no connection found\n"); > return NULL; > } > + > +/** > + * Info about currently set esp rules and their respective packet counts. > + */ > +struct esp_rule_status { > + uint32_t spi; /**< security parameter index */ > + struct in6_addr addr; /**< dest address (may be IPV4-mapped) */ > + unsigned int packet_count; /**< number of packets received */ > +}; It seems that quite a bit of effort went into the periodic clean-up of the iptables rules. Would it be possible to simply reset the iptables counters periodically to 0 (iptables -Z <table name>) and then only remove those rules where the counters are still zero after another period? If that works, it should be a lot simpler than the method used here. > === modified file 'firewall/hslist.c' > --- firewall/hslist.c 2010-12-13 19:09:27 +0000 > +++ firewall/hslist.c 2010-12-13 21:28:35 +0000 > @@ -129,3 +129,21 @@ > > return list; > } > + > +/** > + * find an element in the linked list > + * > + * @param list the linked list > + * @param data the element to find > + * @return the element in the linked list > + */ > +struct slist *find_in_slist(struct slist *list, void *data) Both list and data should be const. Stefan