[hipl-dev] Re: [Merge] lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl

On Mon, 13 Dec 2010 23:40:41 +0100, Stefan Götz <stefan.goetz@xxxxxxxxxxxxxxxxx> wrote:


=== 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.

IIRC this was pulled in for consistent magic-numbering of the iptables queues (IN, FORWARD, OUT) only, but because actual communication with iptables doesn't use these they can easily be replaced by a suitable HIPL-specific enum. This may become relevant again if the new netfilter interface is adopted in the future, though.

+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.

OK

+{
+    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.

True for plain old assert(), but HIP_ASSERT() also logs the exact line and condition, which is helpful if no debugger was present at the time of the crash. Or do you mean I should consider it a runtime error and bail out gracefully?

+{
+    struct slist *lst = esp_tuple->dst_addr_list;

it would be good to check esp_tuple for NULL

OK

+/**
+ * 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?

Yes, I chose to keep it until I understand the update code better. I'll at least add a TODO next to it.

@@ -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?

Good catch: I removed these fields [1], but seems like it wasn't merged [2]. If it's not too urgent, I was looking forward to continue working on the branch next week anyway (and address some other earlier mails concerning it too).

[1] http://bazaar.launchpad.net/~christof-mroz/hipl/hipfw-performance/revision/4951 [2] http://bazaar.launchpad.net/~diego-biurrun/hipl/hipfw-performance/annotate/head%3A/firewall/firewall_defines.h#L137

Generally, IPs and HITs in some structs were changed to be stored as-is rather than using a pointless malloc().

@@ -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.

I remember discussing zeroing with René either on the list or in real-life and it was considered too confusing (clobbering "globally visible" data, relying on "iptables -nLv" output seems to be common). Maybe we weren't aware that zeroing individual chains (rather than all) is possible back then. It would indeed do away with the packet counter, and I'm in favor of that.

Other related posts: