[hipl-dev] Re: [Merge] lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl
- From: René Hummen <rene.hummen@xxxxxxxxxxxxxxxxx>
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Tue, 14 Dec 2010 11:28:33 +0100
On 14.12.2010, at 02:50, Christof Mroz wrote:
> 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.
Yes, we didn't consider that. So, I'm fine with zeroing.
Ciao,
René
--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/
Other related posts:
