[hipl-dev] Re: [Merge] lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl
- From: Stefan Götz <stefan.goetz@xxxxxxxxxxxxxxxxx>
- To: mp+43568@xxxxxxxxxxxxxxxxxx
- Date: Mon, 10 Jan 2011 22:39:58 -0000
Review: Needs Fixing
General remarks
---------------
To clarify the review, I'll add an <importance> flag to each comment.
<importance> is either [H]igh, [M]edium, or [L]ow or something else that will
be self explaining. Everything with importance High and Medium is sufficiently
severe in my opinion to block the merge (so I won't give an 'approve' rating)
so these things need to be fixed or addressed in my opinion (things that break
HIPL get a High, things that do not conform to project policy or I find
unacceptable for some other reason get a Medium). Importance Low is something
which I would personally like to see addressed but which are ok, though not
great, to merge as is.
If [H] or [M] is followed by a question, that's a point where I need more
information to make a vote on the merge. No real need to change the code right
away.
I know that this may sound overly formal and a bit full of myself but I hope
that it simply helps the branch author to more easily identify important
comments, what blocks a merge in my opinion and what is just a remark.
Review
------
> === 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>
>
> #include "lib/core/builder.h"
> #include "lib/core/debug.h"
[H] license: does the inclusion introduce a GPL infection?
> @@ -73,9 +76,40 @@
> #include "config.h"
> #include "reinject.h"
>
> -
> -struct dlist *hip_list = NULL;
> -struct dlist *esp_list = NULL;
> +#ifdef CONFIG_HIP_DEBUG
> +// this improves our chances of finding bugs in the timeout code
> +#define DEFAULT_CONNECTION_TIMEOUT 20; // 20 seconds
> +#define DEFAULT_CLEANUP_INTERVAL 10; // 10 seconds
> +#else
> +#define DEFAULT_CONNECTION_TIMEOUT (60 * 5); // 5 minutes
> +#define DEFAULT_CLEANUP_INTERVAL (60 * 60); // 1 minute
> +#endif
[M] style & policy: use constants instead of macros
> +/**
> + * Interval between sweeps in hip_fw_conntrack_periodic_cleanup(),
> + * in Seconds.
[L] docs: Seconds -> seconds
> @@ -116,7 +149,7 @@
> *
> * @param hiptuple HIP tuple
> */
> -static void print_tuple(const struct hip_tuple *hiptuple)
> +static void print_tuple(DBG const struct hip_tuple *hiptuple)
[L] style: const correctness for pointer arguments. May apply to other
functions. Please check.
> @@ -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.
[L] I don't like TODOs in trunk. Especially if they effectively say that the
code has poor quality due to lazyness ;-)
> + *
> + * @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)
> +{
> + 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);
> +
> + if (esp_tuple->esp_prot_tfm > ESP_PROT_TFM_UNUSED) {
> + HIP_DEBUG("ESP Transforms requested; not handled via iptables "
> + "since we need to inspect packets\n");
> + return 0;
> + }
> +
> + if (esp_tuple->tuple->esp_relay) {
> + HIP_DEBUG("ESP Relay requested; not handled via iptables "
> + "since we need packet rewriting\n");
> + return 0;
> + }
[L] design/style: It seems strange to me that 'success' is returned when the
function didn't actually complete successfully. It might make sense to have a
return value that says 'you called this function under conditions with which a
call does not make sense'.
> + err = system_printf("ip6tables %s %s -p 50 "
> + "-d %s -m esp --espspi 0x%08X -j ACCEPT",
> + flag, table, daddr, esp_tuple->spi);
> + }
> +
> +out_err:
> + if (err == EXIT_SUCCESS) {
> + total_esp_rules_count += (insert ? 1 : -1);
> + HIP_DEBUG("total_esp_rules_count = %d\n", total_esp_rules_count);
> + }
> + return err;
> +}
[L] style: it's counter-intuitive to have non-error code after the label
out_err.
> +/**
> + * Forward all spi/dest combos associated with @a esp_tuple via iptables,
> + * or delete the corresponding rules.
[L] docs: wouldn't it be more correct to say something like 'Insert or remove
all iptables rules matching a given combination of SPIs and destination
[somethings]'?
> + *
> + * @param esp_tuple Extract SPI and connection info from this tuple.
[L] docs: Conveys an implementation detail, not a useful hint on the effect of
this parameter on the service this function provides.
> + * @param insert remove all rules if zero, add otherwise.
> + *
> + * @see hip_fw_manage_esp_rule
> + */
> +void hip_fw_manage_esp_tuple(const struct esp_tuple *esp_tuple, bool insert)
[M] const correctness
> +{
> + struct slist *lst = esp_tuple->dst_addr_list;
> + while (lst) {
> + hip_fw_manage_esp_rule(esp_tuple, lst->data, insert);
> + lst = lst->next;
> + }
> +}
[L] style: personally, I would write this as a for instead of a while loop
because it perfectly matches the for loop pattern
> +
> +/**
> + * Insert a destination address into an esp_tuple. If same address exists
> already,
> + * the update_id is replaced with the new value instead.
[M] docs: does this sentence really reflect the service provided by the
function? I think the function actually changes iptables rules, right?
[L] docs: rewrite to reflect actual functionality
> @@ -425,9 +584,11 @@
> * initialize and store a new HIP/ESP connnection into the connection table
> *
> * @param data the connection-related data to be inserted
> + * @param ctx the context
[M] docs: please make the documentation more helpful for a caller
> @@ -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 */
> +};
[M] Hm, I think we've discussed the topic of keeping track of packet counts in
HIPL before. I think we came to the conclusion that it would be less
error-prone and more efficient to use iptable's internal counters instead.
Above all, it would significantly reduce code complexity. If that is correct, I
do not want the current code to be merged into trunk.
> +/**
> + * Do some necessary bookkeeping concerning connection tracking.
> + * Currently, this only makes sure that stale locations will be removed.
> + * The actual tasks will be run at most once per @c connection_timeout
> + * seconds, no matter how often you call the function.
> + *
> + * @note Don't call this from a thread or timer, since most of hipfw is not
> + * reentrant (and so this function isn't either).
> + */
> +void hip_fw_conntrack_periodic_cleanup(void)
> +{
> + int err = 0;
> + struct slist *iter_conn = NULL;
> + struct esp_rule_status *snapshot = NULL;
> + static time_t last_check = 0; // timestamp of last call
> + const time_t now = time(NULL);
> +
> + if (connection_timeout == 0) {
> + // timeout disabled
> + return;
> + }
> +
> + HIP_ASSERT(now >= last_check);
> + if (last_check != 0 && now - last_check < cleanup_interval) {
> + // don't sweep yet
> + return;
> + }
> +
> + HIP_DEBUG("Commencing periodic cleanup\n");
> +
> + last_check = now;
> +
> + // If connections are covered by iptables rules, we rely on packet
> + // counters to update timestamps indirectly.
> + // By taking care of this now, the affected connections won't be
> + // examined again in the conn_list loop later on.
> +
> + if (total_esp_rules_count > 0) {
> + const size_t sz = sizeof(*snapshot) * total_esp_rules_count;
> + size_t n = 0;
> +
> + HIP_IFEL(!(snapshot = malloc(sz)), 1, "Insufficient memory\n");
> +
[M] style: Could a local array variable be used instead of malloc()?
[L] style: if possible, use a local array variable instead of heap memory to
simplify memory management
> + n += populate_snapshot("iptables -nvL", &snapshot[n],
> total_esp_rules_count - n);
> + HIP_ASSERT(n <= total_esp_rules_count);
> +
> + n += populate_snapshot("ip6tables -nvL", &snapshot[n],
> total_esp_rules_count - n);
> + HIP_ASSERT(n <= total_esp_rules_count);
> +
> + struct dlist *iter_esp = esp_list;
> + while (iter_esp) {
> + update_esp_tuple_timestamp(now, snapshot, n, iter_esp->data);
> + iter_esp = iter_esp->next;
> + }
> + free(snapshot);
> + snapshot = NULL;
> + }
> +
> + // do the actual cleaning now
> +
> + iter_conn = conn_list;
> + while (iter_conn) {
> + struct connection *conn = (struct connection*) iter_conn->data;
[M] style: could the cast be avoided?
[L] style: remove cast if possible
> === modified file 'firewall/firewall_defines.h'
> --- firewall/firewall_defines.h 2010-12-13 19:09:27 +0000
> +++ firewall/firewall_defines.h 2010-12-13 21:28:35 +0000
> @@ -78,11 +79,18 @@
> // null update_id can be removed.
> };
>
> +/**
> + * Stores information about one ESP connection endpoint (i.e., one SPI).
> + *
> + * @todo Write allocator/deallocator for automatic synching with ::esp_list.
[M] docs: specify why this not necessary right now but why it could be
necessary at some stage. Developers need to be able to understand the basic
implications of this TODO without having to read through hundreds of lines of
potentially affected code.
> @@ -119,6 +127,11 @@
> struct tuple *tuple;
> };
>
> +/**
> + * Connection endpoint.
> + *
> + * @todo Write allocator/deallocator for automatic syncing with ::hip_list.
[M] docs: see above.
> @@ -129,18 +142,26 @@
> int direction;
> struct connection *connection;
> int state;
> + int hook;
> uint32_t lupdate_seq;
> int esp_relay;
> struct in6_addr esp_relay_daddr;
> in_port_t esp_relay_dport;
> };
>
> +/**
> + * A connection (i.e., two tuples).
[M] docs: not helpful. A connection between what? What tuples? Why do we need
this data structure?
> + * alloc/dealloc using insert_new_connection() or
> + * remove_connection(), respectively.
> + */
> struct connection {
> === 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
[L] docs: Grammatically correct English sentences are very desirable in doxygen
documentation.
> + *
> + * @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)
[M] const correctness
> === modified file 'hipd/hiprelay.c'
> --- hipd/hiprelay.c 2010-11-30 14:50:30 +0000
> +++ hipd/hiprelay.c 2010-12-13 21:28:35 +0000
> @@ -1015,16 +1015,16 @@
> *
> * @param r the HIP control message to be relayed
> * @param type_hdr message type
> - * @param r_saddr the original source address
> - * @param r_daddr the original destination address
> + * @param r_saddr (unused) the original source address
> + * @param r_daddr (unused) the original destination address
[M] what is the point of adding unused function arguments to an already crowded
signature?
> === modified file 'lib/core/debug.h'
> --- lib/core/debug.h 2010-12-06 19:58:46 +0000
> +++ lib/core/debug.h 2010-12-13 21:28:35 +0000
> @@ -29,6 +29,7 @@
> #include <stdarg.h>
> #include <stdint.h>
>
> +#include "config.h"
> #include "protodefs.h"
>
> /* includes filename, line number and max(debug_prefix[]) */
> @@ -295,10 +296,17 @@
> #define HIP_INFO_LSI(str, lsi) hip_print_lsi(DEBUG_LEVEL_INFO, __FILE__,
> __LINE__, __FUNCTION__, str, lsi)
> #define HIP_INFO_INADDR(str, in) hip_print_lsi(DEBUG_LEVEL_INFO, __FILE__,
> __LINE__, __FUNCTION__, str, in)
>
> +#ifdef CONFIG_HIP_DEBUG
> #define HIP_DEBUG_HIT(str, hit) hip_print_hit(DEBUG_LEVEL_DEBUG, __FILE__,
> __LINE__, __FUNCTION__, str, hit)
> #define HIP_DEBUG_IN6ADDR(str, in6) hip_print_hit(DEBUG_LEVEL_DEBUG,
> __FILE__, __LINE__, __FUNCTION__, str, in6)
> #define HIP_DEBUG_LSI(str, lsi) hip_print_lsi(DEBUG_LEVEL_DEBUG, __FILE__,
> __LINE__, __FUNCTION__, str, lsi)
> #define HIP_DEBUG_INADDR(str, in) hip_print_lsi(DEBUG_LEVEL_DEBUG,
> __FILE__, __LINE__, __FUNCTION__, str, in)
> +#else
> +#define HIP_DEBUG_HIT(str, hit) do {} while (0)
> +#define HIP_DEBUG_IN6ADDR(str, in6) do {} while (0)
> +#define HIP_DEBUG_LSI(str, lsi) do {} while (0)
> +#define HIP_DEBUG_INADDR(str, in) do {} while (0)
> +#endif
[M] Is this modification relevant to this branch? I think it should be
committed separately.
--
https://code.launchpad.net/~diego-biurrun/hipl/hipfw-performance/+merge/43568
Your team HIPL core team is requested to review the proposed merge of
lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl.
Other related posts:
