[hipl-dev] Re: [Merge] lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl
- From: Stefan Götz <stefan.goetz@xxxxxxxxxxxxxxxxx>
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Mon, 13 Dec 2010 23:40:41 +0100
> === 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
Other related posts:
