Merge lp:~christof-mroz/hipl/hipfw-esp-speedup into lp:hipl

[Christof Mroz]

Addressed the review comments.

[Miika Komu (miika-iki)]

How much is the speed up?

[Christof Mroz (christof-mroz)]

> How much is the speed up?

See the branch description for iperf results (using VMs so far):
https://code.launchpad.net/~christof-mroz/hipl/hipfw-esp-speedup

[Diego Biurrun (diego-biurrun)]

 review needs-fixing

On Tue, Mar 22, 2011 at 04:37:59PM +0000, Christof Mroz wrote:
> Christof Mroz has proposed merging lp:~christof-mroz/hipl/hipfw-esp-speedup into lp:hipl with lp:~christof-mroz/hipl/hipfw-timeout as a prerequisite.
>
> --- firewall/conntrack.c 2011-03-22 16:37:54 +0000
> +++ firewall/conntrack.c 2011-03-22 16:37:54 +0000
> @@ -340,23 +346,154 @@
>
> + * Set up or remove iptables rules to bypass userspace processing of the
> + * SPI/destination pairs as specified by @a esp_tuple and @a dest.
> + * This can greatly improve firewall throughput.
> + *
> + * @param esp_tuple Determines the SPI.
> + * @param dest The corresponding destination address to bypass. May be
> + * a IPv6-mapped IPv4 address.
> + * @param insert Insert new rule if true, remove existing if false.
> + * @return 0 if rules were modified, non-zero otherwise.

The function returns -1, not non-zero - positive numbers are non-zero as
well, so I suggest being doubly clear here...

> + * @note This interferes, in one way or another, with userspace_ipsec,
> + * Relay, LSI, midauth, lightweight-update and esp_prot. Care was
> + * taken to not break these features though.

nit: relay

> +static int hip_fw_manage_esp_rule(const struct esp_tuple *const esp_tuple,
> + const struct in6_addr *const dest, bool insert)
> +{
> +
> + if (IN6_IS_ADDR_V4MAPPED(dest)) {
> + char daddr[INET_ADDRSTRLEN];
> + struct in_addr dest4;
> +
> + IPV6_TO_IPV4_MAP(dest, &dest4);
> + HIP_IFEL(!inet_ntop(AF_INET, &dest4, daddr, sizeof(daddr)), -1,
> + "inet_ntop: %s\n", strerror(errno));
> + } else {
> + char daddr[INET6_ADDRSTRLEN];
> + HIP_IFEL(!inet_ntop(AF_INET6, dest, daddr, sizeof(daddr)), -1,
> + "inet_ntop: %s\n", strerror(errno));
> +
> + }
> +
> +out_err:
> + return err == EXIT_SUCCESS ? 0 : -1;
> +}

I'd solve this without goto/HIP_IFEL and just return directly.

> +/**
> + * Set up or remove iptables rules to bypass userspace processing of all
> + * SPI/destination pairs associated with @a esp_tuple.
> + *
> + * @param esp_tuple Determines the SPI and all destination addresses.
> + * @param insert Insert rules if true, remove existing if false.
> + *
> + * @see hip_fw_manage_esp_rule

hip_fw_manage_esp_rule() ?

I'm not sure, just asking...

> +void hip_fw_manage_esp_tuple(const struct esp_tuple *const esp_tuple, const bool insert)

nit: break such long lines

> @@ -448,7 +588,7 @@
> * @param data the connection-related data to be inserted
> * @see remove_connection
> */
> -static void insert_new_connection(const struct hip_data *data)
> +static void insert_new_connection(const struct hip_data *data, struct hip_fw_context *ctx)

ditto

> @@ -1983,6 +2108,173 @@
>
> /**
> + * Parse one line of iptables -nvL formatted output, and extract
> + * packet count, SPI and destination IP if successful.
> + * This takes into account all kinds of rules that can are created by
> + * hip_fw_manage_esp_rule().

can are?

> + * @param spi Out: receives the SPI.
> + * @param spi Out: rece...

review: Needs Fixing

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christof!

Thanks for
- splitting the original merge proposal into smaller chunks
- providing hard numbers to show that we really want theses changes

I don't have a test setup, so I couldn't test this feature. Code comments only.

> === modified file 'firewall/conntrack.c'
> --- firewall/conntrack.c 2011-03-22 16:37:54 +0000
> +++ firewall/conntrack.c 2011-03-22 16:37:54 +0000
> @@ -340,23 +346,154 @@
> }
>
> /**
> - * 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);
> + * Set up or remove iptables rules to bypass userspace processing of the
> + * SPI/destination pairs as specified by @a esp_tuple and @a dest.
> + * This can greatly improve firewall throughput.
> + *
> + * @param esp_tuple Determines the SPI.
> + * @param dest The corresponding destination address to bypass. May be
> + * a IPv6-mapped IPv4 address.
> + * @param insert Insert new rule if true, remove existing if false.
> + * @return 0 if rules were modified, non-zero otherwise.
> + *
> + * @note This feature may be turned off completely by the -u command line option.
> + * It is also automatically deactivated for connections that demand
> + * more advanced connection tracking.
> + * In these cases, -1 is returned even though there was not even an
> + * attempt to modify rules.
> + *
> + * @note This interferes, in one way or another, with userspace_ipsec,
> + * Relay, LSI, midauth, lightweight-update and esp_prot. Care was
> + * taken to not break these features though.
> + *
> + * @see update_esp_address()
> + * @see free_esp_tuple()
> + * @see ::esp_speedup
> + */
> +static int hip_fw_manage_esp_rule(const struct esp_tuple *const esp_tuple,
> + const struct in6_addr *const dest, bool insert)

[M] 'const bool insert' for const correctness

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

[M] const correctness

> @@ -367,7 +504,7 @@
> *esp_addr->update_id = *upd_id;
> }
> HIP_DEBUG("update_esp_address: found and updated\n");
> - return addr_list;
> + return;
> }
> esp_addr = malloc(sizeof(struct esp_address));
> memcpy(&esp_addr->dst_addr, addr, sizeof(struct in6_addr));

[H] not you...

review: Needs Fixing

[Diego Biurrun (diego-biurrun)]

On Sun, Mar 27, 2011 at 09:54:13PM +0000, Stefan Götz wrote:
>
> > === modified file 'firewall/conntrack.c'
> > --- firewall/conntrack.c 2011-03-22 16:37:54 +0000
> > +++ firewall/conntrack.c 2011-03-22 16:37:54 +0000
> > @@ -340,23 +346,154 @@
> >
> > +static int hip_fw_manage_esp_rule(const struct esp_tuple *const esp_tuple,
> > + const struct in6_addr *const dest, bool insert)
>
> [M] 'const bool insert' for const correctness

What's the point of adding const to a non-pointer value passed
to a function?

> > @@ -448,7 +588,7 @@
> > * @param data the connection-related data to be inserted
>
> [M] the 'ctx' parameter is not documented

Christof, does your code pass 'make alltests'? This should likely
be caught by 'make doxygen', which is a part of 'make alltests'.

Apparently we should create some sort of checklist to run through
before submitting merge proposals...

Diego

[Christof Mroz (christof-mroz)]

On Sun, 27 Mar 2011 21:54:13 +0000
Stefan Götz <email address hidden> wrote:

> > + * @return true if rule was valid and output was
> > written,
> > + * false otherwise
>
> [L] Again, when is a rule 'valid'?

Explaining the input format in detail would be reciting the
implementation, I think. It already states that rules set up by
hip_fw_manage_esp_rule() are covered, which is the important info.

> > + * @param now We consider this the current time.
> > + * @return Number of rules that were identified with an
> > esp tuple.
> > + * Not necessarily the number of tuples updated.
> > + *
> > + * @note This function doesn't clear the packet counters itself.
>
> [M] Reading and zeroing the packet counters should be done atomically
> with a single command line invocation. Otherwise, a packet might
> arrive in between and the ESP rule would be deleted even though it
> was still active.

Good point. Didn't find a way to zero out individual rules though.

> > + *
> > + * @todo De-uglify this. You may be tempted to statically link in
> > libiptc,
> > + * and I'd generally approve of it because while it was
> > never meant
> > + * to be used publicly, quite some projects have relied on
> > it without
> > + * burning their fingers too badly for a long time now. But
> > on the
> > + * other hand, there's the impending nftables release that
> > will render
> > + * all your hard work obsolete anyway. I'd rather suggest
> > waiting for
> > + * a post-alpha release of libnl_nft before wasting your
> > time...
> > + * --cmroz, oct 2010
>
> [M] I find this confusing. First, the @todo says that this
> functionality should not be implemented as is but via libiptc
> instead. Then it says the implementation should stay the way it is.
> So is this really a @todo? Or is the @todo the update to libnl_nft?

The TODO applies to "de-uglify this", everything else is just a hint. Would you degrade it to a @note?

Alternatively, we could also discuss the issue right now and get rid of that paragraph altogether. Controlling iptables via shell may not be pretty, but using libiptc instead would blow up the code considerably in most cases, without real benefit. The only counterexample (that I know of), where libiptc could atually simplify matters, would be the ESP speedups introduced in this branch.

But pulling in a new lib because of that? Sounds like overkill.
Also, I bet there are many scripts out there that depend on the current output format, so the netfilter team will be careful not to change it unnecessarily.

Some months later, I'm not so sure anymore that iptables will be obsoleted in the near future.

> > + */
> > +static unsigned int detect_esp_rule_activity(const char *const cmd,
> > + const time_t now)
> > +{
> > +
> > + unsigned int ret = 0;
> > + bool chain_ok = false;
> > + char bfr[256];
> > + FILE *p;
> > +
> > + if (!(p = popen(cmd, "r"))) {
> > + HIP_ERROR("popen(\"%s\"): %s\n", cmd, strerror(errno));
> > + return 0;
> > + }
> > +
> > + ...

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Diego!

>> > +static int hip_fw_manage_esp_rule(const struct esp_tuple *const esp_tuple,
>> > +                                  const struct in6_addr *const dest, bool insert)
>>
>> [M] 'const bool insert' for const correctness
>
> What's the point of adding const to a non-pointer value passed
> to a function?

doc/HACKING encourages const correctness, including the case of
function parameters (regardless of whether they are pointers or not).
The given code violates this policy. That's why I remarked on it.
Maybe I misunderstood your question?

Stefan

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christof!

On Mon, Mar 28, 2011 at 3:46 PM, Christof Mroz <email address hidden> wrote:
> On Sun, 27 Mar 2011 21:54:13 +0000
> Stefan Götz <email address hidden> wrote:
>
>> > + * @return true if rule was valid and output was
>> > written,
>> > + * false otherwise
>>
>> [L] Again, when is a rule 'valid'?
>
> Explaining the input format in detail would be reciting the
> implementation, I think. It already states that rules set up by
> hip_fw_manage_esp_rule() are covered, which is the important info.

Yes, I agree. What I meant was: it is not obvious, that 'rule' == 'input'. Consequently, you can give this function perfectly valid output from 'iptables -nvL' and that function would still return false - simply because not all output lines from 'iptables -nvL' are rules. That, however does not become clear from the documentation. Why I'm such a PITA about documentation here is that it's fairly difficult to correctly interpret the documentation without having read the source code in this case. Something like '@return true if @a input could be parsed as an ESP rule and the output parameters were set to the according values, false otherwise' is all I need to understand this better.

>> [M] Reading and zeroing the packet counters should be done atomically
>> with a single command line invocation. Otherwise, a packet might
>> arrive in between and the ESP rule would be deleted even though it
>> was still active.
>
> Good point. Didn't find a way to zero out individual rules though.

Yepp, it's not possible for individual rules, but you can combine the '-L' option for iptables with the '-Z' option for a particular chain to read and reset the counters atomically.

>> [M] I find this confusing. First, the @todo says that this
>> functionality should not be implemented as is but via libiptc
>> instead. Then it says the implementation should stay the way it is.
>> So is this really a @todo? Or is the @todo the update to libnl_nft?
>
> The TODO applies to "de-uglify this", everything else is just a hint. Would you degrade it to a @note?
>
> Alternatively, we could also discuss the issue right now and get rid of that paragraph altogether. Controlling iptables via shell may not be pretty, but using libiptc instead would blow up the code considerably in most cases, without real benefit. The only counterexample (that I know of), where libiptc could atually simplify matters, would be the ESP speedups introduced in this branch.
>
> But pulling in a new lib because of that? Sounds like overkill.
> Also, I bet there are many scripts out there that depend on the current output format, so the netfilter team will be careful not to change it unnecessarily.
>
> Some months later, I'm not so sure anymore that iptables will be obsoleted in the near future.

Okay, now I have all the information :-) This should go into a @note in order to justify your decision for parsing iptables output so every other developer doesn't have to scratch their heads as to why this route was chosen. I don't think we need 'to do' anything about this code at all.

>> > + if (strncmp(bfr, "Chain", 5) == 0) {
>> > + chain_ok =...

review: Needs Fixing

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Christof!

> === modified file 'firewall/conntrack.c'
> +static void update_esp_address(struct esp_tuple *esp_tuple,

[M] would 'struct esp_tuple *const esp_tuple' work for better const correctness?

> /**
> + * Parse one line of iptables -nvL formatted output, and extract
> + * packet count, SPI and destination IP if it is valid and packet count is
> + * not zero.

[L] What is the acceptable/supported format? 'iptables -nvL' output might differ based on version or translation. Examples would be sufficient.

[M] The functions hip_fw_manage_esp_tuple() and system_printf() are not covered by unit tests.

review: Needs Fixing

[Diego Biurrun (diego-biurrun)]

 review abstain

I'm fine with the merge proposal now, some minor comments below.
All you need to pass now is Stefan's review :)

On Mon, Mar 28, 2011 at 02:06:47PM +0000, Christof Mroz wrote:
> Christof Mroz has proposed merging lp:~christof-mroz/hipl/hipfw-esp-speedup into lp:hipl with lp:~christof-mroz/hipl/hipfw-timeout as a prerequisite.
>
> --- firewall/conntrack.c 2011-03-28 14:06:39 +0000
> +++ firewall/conntrack.c 2011-03-28 14:06:40 +0000
> @@ -1984,6 +2114,174 @@
>
> + if ((str_spi = strstr(input, "spi:"))) {
> + // non-UDP
> + if (sscanf(str_spi, "spi:%u", spi) < 1) {
> + HIP_ERROR("Unexpected iptables output (spi): '%s'\n", input);
> + return false;
> + }
> + } else if ((str_spi = strstr(input, u32_prefix))) {
> + // UDP
> + // spi follows u32_prefix string as a hex number
> + // (always host byte order)
> + if (sscanf(&str_spi[sizeof(u32_prefix) - 1], "%x", spi) < 1) {
> + HIP_ERROR("Unexpected iptables output (u32 match): '%s'\n", input);
> + return false;
> + }
> + } else {
> + // no SPI specified, so it's no ESP rule
> + return false;
> + }
> +
> + // grab packet count and destination IP.
> + if (sscanf(input, formats[0], packet_count, ip) < 2) {
> + // retry with alternative format before we give up
> + if (sscanf(input, formats[1], packet_count, ip) < 2) {
> + HIP_ERROR("Unexpected iptables output (number of colums): '%s'\n", input);
> + return false;
> + }
> + }

All the inner if-conditions could be folded into the outer if-conditions.

> + while (fgets(bfr, sizeof(bfr), p)) {
> + if (strncmp(bfr, "Chain", 5) == 0) {
> + chain_ok = (strncmp(bfr, "Chain HIPFW-INPUT", 17) == 0) ||
> + (strncmp(bfr, "Chain HIPFW-OUTPUT", 18) == 0) ||
> + (strncmp(bfr, "Chain HIPFW-FORWARD", 19) == 0);

nit: The () are unnecessary.

> --- firewall/pisa.c 2011-01-14 15:53:02 +0000
> +++ firewall/pisa.c 2011-03-28 14:06:40 +0000
> @@ -311,6 +311,12 @@
> if (t) {
> t->connection->pisa_state = PISA_STATE_ALLOW;
> HIP_INFO("PISA accepted the connection.\n");
> +
> + struct slist *lst = t->esp_tuples;
> + while (lst) {
> + hip_fw_manage_esp_tuple(lst->data, true);
> + lst = lst->next;
> + }
> } else {
> HIP_ERROR("Connection not found.\n");
> }
> @@ -329,6 +335,15 @@
> if (t) {
> t->connection->pisa_state = PISA_STATE_DISALLOW;
> + HIP_INFO("PISA removed the connection.\n");
> +
> + struct slist *lst = t->esp_tuples;
> + while (lst) {
> + hip_fw_manage_esp_tuple(lst->data, false);
> + lst = lst->next;
> + }
> + } else {
> + HIP_ERROR("Connection not found.\n");
> }
> }

This might be worth moving into its own function to avoid the
code duplication.

Diego

review: Abstain

[Diego Biurrun (diego-biurrun)]

On Mon, Mar 28, 2011 at 03:50:51PM +0000, Stefan Götz wrote:
>
> >> > +static int hip_fw_manage_esp_rule(const struct esp_tuple *const esp_tuple,
> >> > +                                  const struct in6_addr *const dest, bool insert)
> >>
> >> [M] 'const bool insert' for const correctness
> >
> > What's the point of adding const to a non-pointer value passed
> > to a function?
>
> doc/HACKING encourages const correctness, including the case of
> function parameters (regardless of whether they are pointers or not).
> The given code violates this policy. That's why I remarked on it.

Yes, of course.

> Maybe I misunderstood your question?

I'm wondering aloud if it makes sense to enforce const correctness for
values passed to a function as ints. Modifying such values will not
have side effects since they are only used in the function.

So I'm wondering if it makes sense to demand const correctness for such
values or if it's just a pointless bother for the programmer.

Note that I don't have an answer, I'm wondering and I'm interested in
hearing opinions.

Diego

[Stefan Götz (stefan.goetz-deactivatedaccount)]

> I'm wondering aloud if it makes sense to enforce const correctness for
> values passed to a function as ints.  Modifying such values will not
> have side effects since they are only used in the function.

1) True, a modified function argument has no side effect outside the function.

2) This is not about external side effects. This is about keeping a
variable value constant within its scope if it is *intended* not to
change. By extension of your argument (and to annoy you a bit), we
don't need to mark global constants as 'const' anymore because these
'const's are a bother to everyone as long as they either painstakingly
check that these intended constants are never modified during runtime.
Or because such a modification has no side effects outside the running
process anyway.

> Note that I don't have an answer, I'm wondering and I'm interested in
> hearing opinions.

My opinion, I guess, is clear :) When an L-Value (variable/argument)
can be const, it should be const. I think it is good programming
practice and a good policy to let the compiler help where it can.

Stefan

[Christof Mroz (christof-mroz)]

On Mon, 28 Mar 2011 18:11:00 +0200, Stefan Götz
<email address hidden> wrote:

>>> [L] Again, when is a rule 'valid'?
>>
>> Explaining the input format in detail would be reciting the
>> implementation, I think. It already states that rules set up by
>> hip_fw_manage_esp_rule() are covered, which is the important info.
>
> Yes, I agree. What I meant was: it is not obvious, that 'rule' ==
> 'input'.

Ah I see what you meant in the last comment now.

> Something like '@return true if @a input could be parsed as an ESP
> rule and the output parameters were set to the according values, false
> otherwise'

Thanks, I'll plagiarize that.

[Christof Mroz (christof-mroz)]

On Mon, 28 Mar 2011 19:03:34 +0200, Diego Biurrun <email address hidden> wrote:

> I'm fine with the merge proposal now, some minor comments below.
> All you need to pass now is Stefan's review :)

Hooray! On to the final boss.

>> + // grab packet count and destination IP.
>> + if (sscanf(input, formats[0], packet_count, ip) < 2) {
>> + // retry with alternative format before we give up
>> + if (sscanf(input, formats[1], packet_count, ip) < 2) {
>> + HIP_ERROR("Unexpected iptables output (number of colums):
>> '%s'\n", input);
>> + return false;
>> + }
>> + }
>
> All the inner if-conditions could be folded into the outer if-conditions.

It's more readable this way IMO.

>> --- firewall/pisa.c 2011-01-14 15:53:02 +0000
>> +++ firewall/pisa.c 2011-03-28 14:06:40 +0000
>> @@ -311,6 +311,12 @@
>> if (t) {
>> t->connection->pisa_state = PISA_STATE_ALLOW;
>> HIP_INFO("PISA accepted the connection.\n");
>> +
>> + struct slist *lst = t->esp_tuples;
>> + while (lst) {
>> + hip_fw_manage_esp_tuple(lst->data, true);
>> + lst = lst->next;
>> + }
>> } else {
>> HIP_ERROR("Connection not found.\n");
>> }
>> @@ -329,6 +335,15 @@
>> if (t) {
>> t->connection->pisa_state = PISA_STATE_DISALLOW;
>> + HIP_INFO("PISA removed the connection.\n");
>> +
>> + struct slist *lst = t->esp_tuples;
>> + while (lst) {
>> + hip_fw_manage_esp_tuple(lst->data, false);
>> + lst = lst->next;
>> + }
>> + } else {
>> + HIP_ERROR("Connection not found.\n");
>> }
>> }
>
> This might be worth moving into its own function to avoid the
> code duplication.

OK, Good idea.

[Diego Biurrun (diego-biurrun)]

On Tue, Mar 29, 2011 at 07:30:58PM +0000, Christof Mroz wrote:
> On Mon, 28 Mar 2011 19:03:34 +0200, Diego Biurrun <email address hidden> wrote:
>
> > I'm fine with the merge proposal now, some minor comments below.
> > All you need to pass now is Stefan's review :)
>
> Hooray! On to the final boss.

:)

> >> + // grab packet count and destination IP.
> >> + if (sscanf(input, formats[0], packet_count, ip) < 2) {
> >> + // retry with alternative format before we give up
> >> + if (sscanf(input, formats[1], packet_count, ip) < 2) {
> >> + HIP_ERROR("Unexpected iptables output (number of colums):
> >> '%s'\n", input);
> >> + return false;
> >> + }
> >> + }
> >
> > All the inner if-conditions could be folded into the outer if-conditions.
>
> It's more readable this way IMO.

I disagree, you also get rid of one level of indentation and semantically
it's one big condition, not two where something might also happen if the
second is not fulfilled. Use your own judgement...

Diego

[lp:~christof-mroz/hipl/hipfw-esp-speedup]

5794. By Christof Mroz on 2011-03-29

Merged hipfw-timeout branch.

5795. By Christof Mroz on 2011-03-29

Clarify documentation of parse_iptables_esp_rule().

5796. By Christof Mroz on 2011-03-29

Atomically read and zero iptables packet counters.

5797. By Christof Mroz on 2011-03-29

Degrade comment about parser approach from a @todo to a @note.

The approach seems to work just fine, and it is unlikely to break in the near future.

5798. By Christof Mroz on 2011-03-29

Const correctness.

5799. By Christof Mroz on 2011-03-29

Merge duplicated code for enabling/disabling all ESP rules associated with one connection tuple.

5800. By Christof Mroz on 2011-03-29

Cosmetic: Whitespace and alignment cleanup.

5801. By Christof Mroz on 2011-03-29

Use correct chain name for outgoing packets.

5802. By Christof Mroz on 2011-03-29

Use true/false rather than 1/0 for boolean.

5803. By Christof Mroz on 2011-03-29

Unit tests for hip_fw_manage_esp_rule.

5804. By Christof Mroz on 2011-04-03

Fix unsigned <-> signed cast direction.

A signed value that's known to be positive may be safely cast into an unsigned
value, but cast unsigned to signed carries the risk of overflow (in C).

5805. By Christof Mroz on 2011-04-03

Export the maximum command line length as a global preprocessor #define.

5806. By Christof Mroz on 2011-04-03

Merged with hipfw-timeout branch.

5807. By Christof Mroz on 2011-04-04

Merged lp:~christof-mroz/hipl/hipfw-timeout rev 5810.

5808. By Christof Mroz on 2011-04-04

Fixed system_printf() length check.

5809. By Christof Mroz on 2011-04-04

Unit test for system_printf().

5810. By Christof Mroz on 2011-04-04

Declare tuple variable at tighter scope.

5811. By Christof Mroz on 2011-04-04

Document static global total_esp_rules_count.

Unmerged revisions

=== modified file 'Makefile.am'
--- Makefile.am	2011-04-04 16:38:28 +0000
+++ Makefile.am	2011-04-04 16:38:28 +0000
@@ -201,6 +201,7 @@
                               test/mocks.c                   \
                               test/firewall/conntrack.c      \
                               test/firewall/file_buffer.c    \
+                              test/firewall/helpers.c        \
                               test/firewall/line_parser.c    \
                               test/firewall/port_bindings.c  \
                               $(firewall_hipfw_sources)

=== modified file 'firewall/conntrack.c'
--- firewall/conntrack.c	2011-04-04 16:38:28 +0000
+++ firewall/conntrack.c	2011-04-04 16:38:28 +0000
@@ -40,7 +40,9 @@
 #define _BSD_SOURCE
 
 #include <errno.h>
+#include <limits.h>
 #include <stdint.h>
+#include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
 #include <arpa/inet.h>
@@ -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"
@@ -62,6 +65,7 @@
 #include "modules/update/hipd/update.h"
 #include "common_types.h"
 #include "dlist.h"
+#include "hslist.h"
 #include "esp_prot_conntrack.h"
 #include "firewall_defines.h"
 #include "firewall.h"
@@ -106,6 +110,8 @@
     STATE_CLOSING
 };
 
+static unsigned int total_esp_rules_count = 0;
+
 /*------------print functions-------------*/
 /**
  * prints out the list of addresses of esp_addr_list
@@ -341,6 +347,161 @@
 }
 
 /**
+ * Set up or remove iptables rules to bypass userspace processing of the
+ * SPI/destination pairs as specified by @a esp_tuple and @a dest.
+ * This can greatly improve firewall throughput.
+ *
+ * @param esp_tuple Determines the SPI.
+ * @param dest      The corresponding destination address to bypass. May be
+ *                  a IPv6-mapped IPv4 address.
+ * @param insert    Insert new rule if true, remove existing if false.
+ * @return          0 if rules were modified, -1 otherwise.
+ *
+ * @note This feature may be turned off completely by the -u command line option.
+ *       It is also automatically deactivated for connections that demand
+ *       more advanced connection tracking.
+ *       In these cases, -1 is returned even though there was not even an
+ *       attempt to modify rules.
+ *
+ * @note This interferes, in one way or another, with userspace_ipsec,
+ *       relay, LSI, midauth, lightweight-update and esp_prot. Care was
+ *       taken to not break these features though.
+ *
+ * @see update_esp_address()
+ * @see free_esp_tuple()
+ * @see ::esp_speedup
+ */
+static int hip_fw_manage_esp_rule(const struct esp_tuple *const esp_tuple,
+                                  const struct in6_addr *const dest,
+                                  const bool insert)
+{
+    int         err   = 0;
+    const char *flag  = insert ? "-I" : "-D";
+    const char *table = NULL;
+
+    if (!esp_speedup || hip_userspace_ipsec) {
+        return -1;
+    }
+
+    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 -1;
+    }
+
+    if (esp_tuple->tuple->esp_relay) {
+        HIP_DEBUG("ESP Relay requested; not handled via iptables "
+                  "since we need packet rewriting\n");
+        return -1;
+    }
+
+    switch (esp_tuple->tuple->hook) {
+    case NF_IP_LOCAL_IN:
+        table = "HIPFW-INPUT";
+        break;
+    case NF_IP_FORWARD:
+        table = "HIPFW-FORWARD";
+        break;
+    case NF_IP_LOCAL_OUT:
+        table = "HIPFW-OUTPUT";
+        break;
+    default:
+        HIP_ERROR("Packet was received via unsupported netfilter hook %d\n",
+                  esp_tuple->tuple->hook);
+        return -1;
+    }
+
+    HIP_DEBUG("insert         = %d\n", insert);
+    HIP_DEBUG("table          = %s\n", table);
+    HIP_DEBUG("esp_tuple->spi = 0x%08X\n", esp_tuple->spi);
+    HIP_DEBUG_IN6ADDR("src  ip", esp_tuple->tuple->src_ip);
+    HIP_DEBUG_IN6ADDR("dest ip", dest);
+
+    if (IN6_IS_ADDR_V4MAPPED(dest)) {
+        char           daddr[INET_ADDRSTRLEN];
+        struct in_addr dest4;
+
+        IPV6_TO_IPV4_MAP(dest, &dest4);
+        HIP_IFEL(!inet_ntop(AF_INET, &dest4, daddr, sizeof(daddr)), -1,
+                 "inet_ntop: %s\n", strerror(errno));
+
+        if (esp_tuple->tuple->connection->udp_encap) {
+            /* SPI is the first 32bit value in encapsulating UDP payload, so
+             * we may use a simple u32 Pattern. Here, '4&0x1FFF=0' ensures
+             * we're not processing a fragmented packet.
+             */
+            err = system_printf("iptables %s %s -p UDP "
+                                "--dport 10500 --sport 10500 -d %s -m u32 "
+                                "--u32 '4&0x1FFF=0 && 0>>22&0x3C@8=0x%08X' -j ACCEPT",
+                                flag, table, daddr, esp_tuple->spi);
+        } else {
+            err = system_printf("iptables %s %s -p 50 "
+                                "-d %s -m esp --espspi 0x%08X -j ACCEPT",
+                                flag, table, daddr, esp_tuple->spi);
+        }
+    } else {
+        char daddr[INET6_ADDRSTRLEN];
+        HIP_IFEL(!inet_ntop(AF_INET6, dest, daddr, sizeof(daddr)), -1,
+                 "inet_ntop: %s\n", strerror(errno));
+
+        HIP_ASSERT(!esp_tuple->tuple->connection->udp_encap);
+        err = system_printf("ip6tables %s %s -p 50 "
+                            "-d %s -m esp --espspi 0x%08X -j ACCEPT",
+                            flag, table, daddr, esp_tuple->spi);
+    }
+
+    if (err == EXIT_SUCCESS) {
+        total_esp_rules_count += (insert ? 1 : -1);
+        HIP_DEBUG("total_esp_rules_count = %d\n", total_esp_rules_count);
+    }
+
+out_err:
+    return err == EXIT_SUCCESS ? 0 : -1;
+}
+
+/**
+ * Set up or remove iptables rules to bypass userspace processing of all
+ * SPI/destination pairs associated with @a esp_tuple.
+ *
+ * @param esp_tuple Determines the SPI and all destination addresses.
+ * @param insert    Insert rules if true, remove existing if false.
+ *
+ * @see hip_fw_manage_esp_rule()
+ */
+static void hip_fw_manage_esp_tuple(const struct esp_tuple *const esp_tuple,
+                                    const bool insert)
+{
+    const struct hip_ll_node *node = esp_tuple->dst_addresses.head;
+    while (node) {
+        hip_fw_manage_esp_rule(esp_tuple, node->ptr, insert);
+        node = node->next;
+    }
+}
+
+/**
+ * Set up or remove iptables rules to bypass userspace processing of all
+ * ESP SPI/destination pairs associated with @a tuple.
+ *
+ * @param esp_tuple Determines all SPI/destination pairs.
+ * @param insert    Insert rules if true, remove existing if false.
+ *
+ * @see hip_fw_manage_esp_rule()
+ * @see hip_fw_manage_esp_tuple()
+ */
+void hip_fw_manage_all_esp_tuples(const struct tuple *const tuple,
+                                  const bool insert)
+{
+    const struct slist *lst = tuple->esp_tuples;
+    while (lst) {
+        hip_fw_manage_esp_tuple(lst->data, insert);
+        lst = lst->next;
+    }
+}
+
+/**
  * Insert or update a destination address associated with an ESP tuple.
  * If the address is already known, its update_id is replaced with the new
  * value.
@@ -383,6 +544,7 @@
         *esp_addr->update_id = *upd_id;
     }
 
+    hip_fw_manage_esp_rule(esp_tuple, addr, true);
     return true;
 
 out_err:
@@ -458,12 +620,17 @@
 }
 
 /**
- * initialize and store a new HIP/ESP connnection into the connection table
- *
- * @param data the connection-related data to be inserted
- * @see remove_connection
+ * Initialize and store a new HIP/ESP connnection into the connection
+ * table.
+ *
+ * @param data The connection-related data to be inserted.
+ * @param ctx  The packet context. Note that source and destination HITs
+ *             are always taken from @a data rather than @a ctx.
+ *
+ * @see remove_connection()
  */
-static void insert_new_connection(const struct hip_data *data)
+static void insert_new_connection(const struct hip_data *const data,
+                                  const struct hip_fw_context *const ctx)
 {
     struct connection *connection = NULL;
 
@@ -472,6 +639,7 @@
     connection = calloc(1, sizeof(struct connection));
 
     connection->state     = STATE_ESTABLISHED;
+    connection->udp_encap = ctx->udp_encap_hdr ? true : false;
     connection->timestamp = time(NULL);
 #ifdef HIP_CONFIG_MIDAUTH
     connection->pisa_state = PISA_STATE_DISALLOW;
@@ -568,6 +736,7 @@
 
         // remove all associated addresses
         while ((addr = hip_ll_del_first(&esp_tuple->dst_addresses, NULL))) {
+            hip_fw_manage_esp_rule(esp_tuple, &addr->dst_addr, false);
             free(addr->update_id);
             free(addr);
         }
@@ -1102,6 +1271,7 @@
 
         other_dir->esp_tuples = append_to_slist(other_dir->esp_tuples, esp_tuple);
 
+        update_esp_address(esp_tuple, ip6_src, NULL);
         insert_esp_tuple(esp_tuple);
     }
 
@@ -1171,6 +1341,7 @@
                  -1, "adding or updating ESP destination address failed");
         esp_tuple->tuple = other_dir;
 
+        update_esp_address(esp_tuple, ip6_src, NULL);
         insert_esp_tuple(esp_tuple);
 
         HIP_DEBUG("ESP tuple inserted\n");
@@ -1663,7 +1834,7 @@
             }
 #endif
 
-            insert_new_connection(data);
+            insert_new_connection(data, ctx);
 
             // TODO call free for all pointer members of data - comment by Rene
             free(data);
@@ -1720,6 +1891,8 @@
         } else {
             HIP_DEBUG("Tuple connection NULL, could not timestamp\n");
         }
+
+        tuple->hook = ctx->ipq_packet->hook;
     }
 
     HIP_DEBUG("udp_encap_hdr=%p tuple=%p err=%d\n", ctx->udp_encap_hdr, tuple, err);
@@ -2022,6 +2195,178 @@
 }
 
 /**
+ * Parse one line of `iptables -nvL` formatted output and extract packet count,
+ * SPI and destination IP if the line corresponds to a previously set up ESP
+ * rule.
+ * This takes into account specifically the kinds of rules that can be created
+ * by hip_fw_manage_esp_rule().
+ *
+ * @param input        The line to be parsed.
+ * @param packet_count Out: receives the packet count.
+ * @param spi          Out: receives the SPI, unless the packet count was zero.
+ * @param dest         Out: receives the destination IP, unless the packet count
+ *                          was zero.
+ * @return             true if @a input could be parsed as an ESP
+ *                     rule (and at least @a packet_count was set), false
+ *                     otherwise.
+ *
+ * @note Short-circuiting behaviour for @a spi and @a dest (see description).
+ *
+ * @see detect_esp_rule_activity()
+ * @see hip_fw_manage_esp_rule()
+ */
+static bool parse_iptables_esp_rule(const char *const input,
+                                    unsigned int *const packet_count,
+                                    uint32_t *const spi,
+                                    struct in6_addr *const dest)
+{
+    static const char u32_prefix[] = "u32 0x4&0x1fff=0x0&&0x0>>0x16&0x3c@0x8=0x";
+
+    /*
+     * In iptables output, one column is optional. So we try the long
+     * format first and fall back to the shorter one (see sscanf call
+     * below).
+     * The %45s format is used here because 45 is the maximum IPv6 address
+     * length, considering all variations (i.e. INET6_ADDRSTRLEN - 1).
+     */
+    static const char *formats[] = { "%u %*u %*s %*s %*2[!f-] %*s %*s %*s %45s",
+                                     "%u %*u %*s %*s %*s %*s %*s %45s" };
+
+    char        ip[INET6_ADDRSTRLEN];
+    const char *str_spi;
+
+    // there's two ways of specifying SPIs in a rule
+    // (see hip_fw_manage_esp_rule)
+
+    if ((str_spi = strstr(input, "spi:"))) {
+        // non-UDP
+        if (sscanf(str_spi, "spi:%u", spi) < 1) {
+            HIP_ERROR("Unexpected iptables output (spi): '%s'\n", input);
+            return false;
+        }
+    } else if ((str_spi = strstr(input, u32_prefix))) {
+        // UDP
+        // spi follows u32_prefix string as a hex number
+        // (always host byte order)
+        if (sscanf(&str_spi[sizeof(u32_prefix) - 1], "%x", spi) < 1) {
+            HIP_ERROR("Unexpected iptables output (u32 match): '%s'\n", input);
+            return false;
+        }
+    } else {
+        // no SPI specified, so it's no ESP rule
+        return false;
+    }
+
+    // grab packet count and destination IP.
+    if (sscanf(input, formats[0], packet_count, ip) < 2) {
+        // retry with alternative format before we give up
+        if (sscanf(input, formats[1], packet_count, ip) < 2) {
+            HIP_ERROR("Unexpected iptables output (number of colums): '%s'\n", input);
+            return false;
+        }
+    }
+
+    // IP not needed, unless there was activity
+    if (*packet_count > 0) {
+        char *slash;
+
+        // IP may be in /128 format, strip the suffix
+        if ((slash = strchr(ip, '/'))) {
+            *slash = '\0';
+        }
+
+        // parse destination IP; try IPv6 first, then IPv4
+        if (!inet_pton(AF_INET6, ip, dest)) {
+            struct in_addr addr4;
+            if (!inet_pton(AF_INET, ip, &addr4)) {
+                HIP_ERROR("Unexpected iptables output: '%s'\n", input);
+                HIP_ERROR("Can't parse destination IP: %s\n", ip);
+                return false;
+            }
+
+            IPV4_TO_IPV6_MAP(&addr4, dest);
+        }
+    }
+
+    return true;
+}
+
+/**
+ * Update timestamps of all ESP tuples where corresponding iptables rules'
+ * packet counters are non-zero.
+ * Currently, this works by parsing the output of iptables and ip6tables
+ * to extract and zero the packet counters.
+ *
+ * @param now We consider this the current time.
+ * @return    Number of rules that were identified with an esp tuple
+ *            (not necessarily the number of tuples updated), or -1 if
+ *            communication with iptables failed.
+ *
+ * @note Ugly approach, yes. You may be tempted to statically link in
+ *       libiptc, and I'd generally approve of it because while it was never
+ *       meant to be used publicly, quite some projects have relied on it
+ *       without burning their fingers too badly for a long time now.  On the
+ *       other hand, libiptc would blow up iptables communication code at most
+ *       other places, and the output format is unlikely to change.
+ *       Furthermore, a successor to iptables (namely: nftables) with an actual
+ *       API is under consideration by the netfilter team.
+ */
+static int detect_esp_rule_activity(const time_t now)
+{
+    static const char *const bins[]   = { "iptables", "ip6tables" };
+    static const char *const chains[] = { "HIPFW-INPUT", "HIPFW-OUTPUT",
+                                          "HIPFW-FORWARD" };
+
+    unsigned int chain, bin, ret = 0;
+
+    for (bin = 0; bin < ARRAY_SIZE(bins); ++bin) {
+        for (chain = 0; chain < ARRAY_SIZE(chains); ++chain) {
+            char  bfr[256];
+            FILE *p;
+
+            snprintf(bfr, sizeof(bfr), "%s -nvL -Z %s", bins[bin], chains[chain]);
+            if (!(p = popen(bfr, "r"))) {
+                HIP_ERROR("popen(\"%s\"): %s\n", bfr, strerror(errno));
+                return -1;
+            }
+
+            while (fgets(bfr, sizeof(bfr), p)) {
+                unsigned int    packet_count;
+                uint32_t        spi;
+                struct in6_addr dest;
+                struct tuple   *tuple;
+
+                if (parse_iptables_esp_rule(bfr, &packet_count, &spi, &dest)) {
+                    ret += 1;
+                    if (packet_count > 0) {
+                        tuple = get_tuple_by_esp(&dest, spi);
+                        if (!tuple) {
+                            HIP_ERROR("Stray ESP rule: SPI = %u\n", spi);
+                            continue;
+                        }
+
+                        tuple->connection->timestamp = now;
+                        HIP_DEBUG("Activity detected: SPI = %u\n", spi);
+                        HIP_DEBUG_IN6ADDR("dest: ", &dest);
+                    }
+                }
+            }
+
+            if (!feof(p)) {
+                HIP_ERROR("fgets(), bin: %s, chain %s: %s\n",
+                          bins[bin], chains[chain], strerror(errno));
+                return -1;
+            }
+
+            pclose(p);
+        }
+    }
+
+    HIP_DEBUG("-> %u\n", ret);
+    return ret;
+}
+
+/**
  * 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 ::connection_timeout
@@ -2032,7 +2377,7 @@
  */
 void hip_fw_conntrack_periodic_cleanup(void)
 {
-    static time_t      last_check = 0;   // timestamp of last call
+    static time_t      last_check = 0; // timestamp of last call
     struct slist      *iter_conn;
     struct connection *conn;
 
@@ -2052,6 +2397,17 @@
     if (now - last_check >= cleanup_interval) {
         HIP_DEBUG("Checking for connection timeouts\n");
 
+        // If connections are covered by iptables rules, we rely on kernel
+        // packet counters to update timestamps indirectly for these.
+
+        if (total_esp_rules_count > 0) {
+            // cast to signed value
+            const int found = detect_esp_rule_activity(now);
+            if (found == -1 || (unsigned int) found != total_esp_rules_count) {
+                HIP_ERROR("Not all ESP tuples' packet counts were found\n");
+            }
+        }
+
         iter_conn = conn_list;
         while (iter_conn) {
             conn      = iter_conn->data;

=== modified file 'firewall/conntrack.h'
--- firewall/conntrack.h	2011-04-04 16:38:28 +0000
+++ firewall/conntrack.h	2011-04-04 16:38:28 +0000
@@ -29,6 +29,7 @@
 #define _BSD_SOURCE
 
 #include <stdint.h>
+#include <stdbool.h>
 #include <netinet/in.h>
 
 #include "lib/core/protodefs.h"
@@ -63,6 +64,8 @@
                                 const struct in6_addr *dst_hit);
 int hipfw_relay_esp(const struct hip_fw_context *ctx);
 
+void hip_fw_manage_all_esp_tuples(const struct tuple *const tuple,
+                                  const bool insert);
 void hip_fw_conntrack_periodic_cleanup(void);
 
 #endif /* HIP_FIREWALL_CONNTRACK_H */

=== modified file 'firewall/firewall.c'
--- firewall/firewall.c	2011-04-04 16:38:28 +0000
+++ firewall/firewall.c	2011-04-04 16:38:28 +0000
@@ -144,6 +144,7 @@
 int hip_lsi_support           = 0;
 int esp_relay                 = 0;
 int hip_esp_protection        = 0;
+int esp_speedup               = 0; /**< Enable esp speedup via dynamic iptables usage (-u option). */
 #ifdef CONFIG_HIP_MIDAUTH
 int use_midauth = 0;
 #endif

=== modified file 'firewall/firewall.h'
--- firewall/firewall.h	2011-03-22 10:31:37 +0000
+++ firewall/firewall.h	2011-04-04 16:38:28 +0000
@@ -47,6 +47,7 @@
 extern int hip_fw_sock;
 extern int hip_fw_async_sock;
 extern int system_based_opp_mode;
+extern int esp_speedup;
 
 int hipfw_main(const char *const rule_file,
                const bool        kill_old,

=== modified file 'firewall/firewall_defines.h'
--- firewall/firewall_defines.h	2011-04-04 16:38:28 +0000
+++ firewall/firewall_defines.h	2011-04-04 16:38:28 +0000
@@ -30,6 +30,7 @@
 
 #include <libipq.h>
 #include <stdint.h>
+#include <stdbool.h>
 #include <netinet/in.h>
 #include <netinet/ip6.h>
 #include <netinet/tcp.h>
@@ -129,6 +130,7 @@
     int                direction;
     struct connection *connection;
     int                state;
+    int                hook;            /**< iptables chain this tuple originates from. */
     uint32_t           lupdate_seq;
     int                esp_relay;
     struct in6_addr    esp_relay_daddr;
@@ -141,6 +143,8 @@
     int          verify_responder;
     int          state;
     time_t       timestamp;
+    /* members needed for iptables setup */
+    bool         udp_encap; /**< UDP encapsulation enabled? (NAT extension) */
     /* members needed for ESP protection extension */
     int          num_esp_prot_tfms;
     uint8_t      esp_prot_tfms[MAX_NUM_TRANSFORMS];

=== modified file 'firewall/helpers.c'
--- firewall/helpers.c	2011-01-11 13:59:46 +0000
+++ firewall/helpers.c	2011-04-04 16:38:28 +0000
@@ -33,6 +33,8 @@
  */
 
 #include <stdlib.h>
+#include <stdio.h>
+#include <stdarg.h>
 #include <arpa/inet.h>
 #include <netinet/in.h>
 
@@ -79,15 +81,58 @@
 }
 
 /**
- * Executes a system command and prints an error if
- * command wasn't successfull.
+ * Executes a command and prints an error if command wasn't successful.
  *
- * @param command The system command. The caller of this function must take
+ * @param command The command. The caller of this function must take
  *                care that command does not contain malicious code.
- */
-void system_print(const char *command)
-{
-    if (system(command) == -1) {
-        HIP_ERROR("Could not execute system command %s", command);
-    }
+ * @return        Exit code on success, -1 on failure.
+ */
+int system_print(const char *const command)
+{
+    int ret;
+
+    if ((ret = system(command)) == -1) {
+        HIP_ERROR("Could not execute command `%s'", command);
+        return -1;
+    }
+
+    HIP_DEBUG("$ %s -> %d\n", command, WEXITSTATUS(ret));
+
+    return WEXITSTATUS(ret);
+}
+
+/**
+ * printf()-like wrapper around system_print.
+ * Fails and returns an error if the resulting command line
+ * would be longer than ::MAX_COMMAND_LINE characters.
+ *
+ * @param command The command. This is a printf format string.
+ *                The caller of this function must take care that command
+ *                does not contain malicious code.
+ * @return        Exit code on success, -1 on failure.
+ */
+int system_printf(const char *const command, ...)
+{
+    char bfr[MAX_COMMAND_LINE + 1];
+
+    va_list vargs;
+    va_start(vargs, command);
+
+    const int ret = vsnprintf(bfr, sizeof(bfr), command, vargs);
+    if (ret <= 0) {
+        HIP_ERROR("vsnprintf failed\n");
+        va_end(vargs);
+        return -1;
+    }
+
+    // cast to unsigned value (we know that ret >= 0)
+    if ((unsigned) ret > MAX_COMMAND_LINE) {
+        HIP_ERROR("Format '%s' results in unexpectedly large command line "
+                  "(%d characters): not executed.\n", command, ret);
+        va_end(vargs);
+        return -1;
+    }
+
+    va_end(vargs);
+    return system_print(bfr);
 }

=== modified file 'firewall/helpers.h'
--- firewall/helpers.h	2010-10-15 15:29:14 +0000
+++ firewall/helpers.h	2011-04-04 16:38:28 +0000
@@ -30,6 +30,10 @@
 
 const char *addr_to_numeric(const struct in6_addr *addrp);
 struct in6_addr *numeric_to_addr(const char *num);
-void system_print(const char *command);
+int system_print(const char *command);
+int system_printf(const char *command, ...);
+
+/** The maximum command line length (that is, argument to system()) we expect. */
+#define MAX_COMMAND_LINE 196
 
 #endif /* HIP_FIREWALL_HELPERS_H */

=== modified file 'firewall/main.c'
--- firewall/main.c	2011-04-04 16:38:28 +0000
+++ firewall/main.c	2011-04-04 16:38:28 +0000
@@ -53,7 +53,7 @@
 static void hipfw_usage(void)
 {
     puts("HIP Firewall");
-    puts("Usage: hipfw [-f file_name] [-d|-v] [-A] [-F] [-H] [-b] [-a] [-c] [-k] [-i|-I|-e] [-l] [-o] [-p] [-t <seconds>] [-h] [-V]");
+    puts("Usage: hipfw [-f file_name] [-d|-v] [-A] [-F] [-H] [-b] [-a] [-c] [-k] [-i|-I|-e] [-l] [-o] [-p] [-t <seconds>] [-u] [-h] [-V]");
 #ifdef CONFIG_HIP_MIDAUTH
     puts(" [-m]");
 #endif
@@ -73,6 +73,7 @@
     puts("      -l = activate lsi support");
     puts("      -p = run with lowered privileges. iptables rules will not be flushed on exit");
     puts("      -t <seconds> = set timeout interval to <seconds>. Disable if <seconds> = 0");
+    puts("      -u = attempt to speed up esp traffic using iptables rules");
     puts("      -h = print this help");
 #ifdef CONFIG_HIP_MIDAUTH
     puts("      -m = middlebox authentication");
@@ -108,7 +109,7 @@
      * Otherwise may get warnings from system_print() commands. */
     setenv("PATH", HIP_DEFAULT_EXEC_PATH, 1);
 
-    while ((ch = getopt(argc, argv, "aAbcdef:FhHiIklmpt:vV")) != -1) {
+    while ((ch = getopt(argc, argv, "aAbcdef:FhHiIklmpt:uvV")) != -1) {
         switch (ch) {
         case 'A':
             accept_hip_esp_traffic_by_default = 1;
@@ -171,6 +172,9 @@
                 cleanup_interval = connection_timeout;
             }
             break;
+        case 'u':
+            esp_speedup = 1;
+            break;
         case 'v':
             log_level = LOGDEBUG_MEDIUM;
             hip_set_logfmt(LOGFMT_SHORT);
@@ -194,6 +198,25 @@
         puts("         tracking disabled (-F)");
     }
 
+    if (esp_speedup && limit_capabilities) {
+        puts("Conflict: ESP speedups (-u) requires root privileges,\n");
+        puts("          but lowered privleges (-p) requested as well.\n");
+        hipfw_usage();
+        return EXIT_FAILURE;
+    }
+
+    if (esp_speedup && hip_userspace_ipsec) {
+        puts("Conflict: Bypassing userspace ESP processing (-u) impossible\n");
+        puts("          with userspace IPSEC enabled (-i or -I)\n");
+        hipfw_usage();
+        return EXIT_FAILURE;
+    }
+
+    if (esp_speedup && !filter_traffic) {
+        puts("Warning: ESP speedup (-U) has no effect without\n");
+        puts("         connection tracking (-F)\n");
+    }
+
     if (geteuid() != 0) {
         HIP_ERROR("Firewall must be run as root\n");
         exit(-1);

=== modified file 'firewall/pisa.c'
--- firewall/pisa.c	2011-01-14 15:53:02 +0000
+++ firewall/pisa.c	2011-04-04 16:38:28 +0000
@@ -311,6 +311,7 @@
     if (t) {
         t->connection->pisa_state = PISA_STATE_ALLOW;
         HIP_INFO("PISA accepted the connection.\n");
+        hip_fw_manage_all_esp_tuples(t, true);
     } else {
         HIP_ERROR("Connection not found.\n");
     }
@@ -329,6 +330,10 @@
 
     if (t) {
         t->connection->pisa_state = PISA_STATE_DISALLOW;
+        HIP_INFO("PISA removed the connection.\n");
+        hip_fw_manage_all_esp_tuples(t, false);
+    } else {
+        HIP_ERROR("Connection not found.\n");
     }
 }
 

=== modified file 'test/check_firewall.c'
--- test/check_firewall.c	2011-04-04 16:38:28 +0000
+++ test/check_firewall.c	2011-04-04 16:38:28 +0000
@@ -31,8 +31,10 @@
 int main(void)
 {
     int      number_failed;
-    SRunner *sr = srunner_create(firewall_file_buffer());
+    SRunner *sr = srunner_create(NULL);
     srunner_add_suite(sr, firewall_conntrack());
+    srunner_add_suite(sr, firewall_file_buffer());
+    srunner_add_suite(sr, firewall_helpers());
     srunner_add_suite(sr, firewall_line_parser());
     srunner_add_suite(sr, firewall_port_bindings());
 

=== modified file 'test/firewall/conntrack.c'
--- test/firewall/conntrack.c	2011-04-04 16:38:28 +0000
+++ test/firewall/conntrack.c	2011-04-04 16:38:28 +0000
@@ -32,6 +32,8 @@
 #include <check.h>
 #include <signal.h>
 #include <time.h>
+#include <assert.h>
+#include <stdlib.h>
 
 #include "firewall/conntrack.h"
 #include "firewall/conntrack.c"
@@ -41,12 +43,13 @@
 
 static struct connection *setup_connection(void)
 {
-    struct hip_data data = { { { { 0 } } } };
+    const struct hip_fw_context ctx  = { 0 };   // only ctx.udp_encap_hdr is examined
+    struct hip_data             data = { { { { 0 } } } };
 
     inet_pton(AF_INET6, "2001:12:bd2d:d23e:4a09:b2ab:6414:e110", &data.src_hit);
     inet_pton(AF_INET6, "2001:10:f039:6bc5:cab3:0727:7fbc:9dcb", &data.dst_hit);
 
-    insert_new_connection(&data);
+    insert_new_connection(&data, &ctx);
 
     fail_if(conn_list       == NULL, "No connection inserted.");
     fail_if(conn_list->next != NULL, "More than one connection inserted.");
@@ -54,6 +57,28 @@
     return conn_list->data;
 }
 
+static struct esp_tuple *setup_esp_tuple(const uint32_t spi,
+                                         const struct in6_addr *const dest,
+                                         struct connection *const conn)
+{
+    struct esp_tuple *const esp_tuple = calloc(1, sizeof(*esp_tuple));
+
+    fail_if(conn      == NULL, NULL);
+    fail_if(esp_tuple == NULL, NULL);
+
+    esp_tuple->spi   = spi;
+    esp_tuple->tuple = &conn->original;
+    update_esp_address(esp_tuple, dest, NULL);
+    insert_esp_tuple(esp_tuple);
+
+    fail_if(esp_list == NULL, "Failed to insert a new ESP tuple");
+
+    conn->original.esp_tuples = append_to_slist(conn->original.esp_tuples,
+                                                esp_tuple);
+
+    return esp_tuple;
+}
+
 START_TEST(test_hip_fw_conntrack_periodic_cleanup_timeout)
 {
     struct connection *conn;
@@ -128,14 +153,179 @@
 }
 END_TEST
 
+START_TEST(test_parse_iptables_esp_rule)
+{
+    struct {
+        const char  *input;
+        bool         valid;
+        unsigned int pkts;
+        const char  *ip;
+        uint32_t     spi;
+    } test_cases[] = {
+        { "Chain HIPFW-FORWARD (1 references)",                                                                        .valid = false },
+        { " pkts bytes target     prot opt in     out     source               destination         ",                  .valid = false },
+        { "    2   312 ACCEPT     esp      *      *       ::/0                 3ffe:2::1/128       esp spi:469913213", .valid = true, .pkts = 2, .ip = "3ffe:2::1", .spi = 0x1c024e7d },
+        { "    0     0 QUEUE      udp      *      *       ::/0                 ::/0                udp spt:10500",     .valid = false },
+        { "    0     0 QUEUE      esp      *      *       ::/0                 ::/0                ",                  .valid = false },
+        { "    4  2264 QUEUE      139      *      *       ::/0                 ::/0                ",                  .valid = false },
+        { "    3   336 ACCEPT     udp  --  *      *       0.0.0.0/0            192.168.2.1         udp spt:10500 dpt:10500 u32 0x4&0x1fff=0x0&&0x0>>0x16&0x3c@0x8=0xab772758", .valid = true, .pkts = 3, .ip = "::ffff:192.168.2.1", .spi = 0xab772758 }
+    };
+
+    const int num_tests = sizeof(test_cases) / sizeof(*test_cases);
+    int       i;
+
+    for (i = 0; i < num_tests; ++i) {
+        struct in6_addr dest, reference;
+        unsigned int    pkts;
+        uint32_t        spi;
+
+        if (parse_iptables_esp_rule(test_cases[i].input, &pkts, &spi, &dest)) {
+            fail_unless(test_cases[i].valid,        "Invalid rule was considered valid");
+            fail_unless(pkts == test_cases[i].pkts, "Packet count not parsed correctly");
+            fail_unless(spi  == test_cases[i].spi,  "SPI not parsed correctly");
+
+            assert(inet_pton(AF_INET6, test_cases[i].ip, &reference) == 1);
+            fail_unless(IN6_ARE_ADDR_EQUAL(&dest, &reference),
+                        "Destination IP not parsed correctly.");
+        } else {
+            fail_unless(test_cases[i].valid == false, "Valid rule was considered invalid");
+        }
+    }
+}
+END_TEST
+
+START_TEST(test_hip_fw_manage_esp_rule_not_enabled)
+{
+    mock_system = true;
+
+    struct in6_addr dest;
+    assert(inet_pton(AF_INET6, "3ffe::1", &dest));
+
+    struct connection *const conn      = setup_connection();
+    struct esp_tuple  *const esp_tuple = setup_esp_tuple(0xAABBCCDD, &dest, conn);
+
+    esp_speedup         = 0;
+    conn->original.hook = NF_IP_LOCAL_IN;
+
+    fail_if(hip_fw_manage_esp_rule(esp_tuple, &dest, true) == 0,
+            "Success, even though esp speedup is disabled");
+    fail_if(mock_system_last, "Rule was created even though esp speedup is disabled");
+}
+END_TEST
+
+START_TEST(test_hip_fw_manage_esp_rule_needs_userspace)
+{
+    struct in6_addr dest;
+    assert(inet_pton(AF_INET6, "3ffe::1", &dest));
+
+    struct connection *const conn      = setup_connection();
+    struct esp_tuple  *const esp_tuple = setup_esp_tuple(0xAABBCCDD, &dest, conn);
+
+    esp_speedup         = 1;
+    conn->original.hook = NF_IP_LOCAL_IN;
+
+    esp_tuple->esp_prot_tfm = ESP_PROT_TFM_PLAIN;
+    fail_if(hip_fw_manage_esp_rule(esp_tuple, &dest, true) == 0,
+            "Added rule even though ESP transforms requested");
+
+    esp_tuple->esp_prot_tfm = ESP_PROT_TFM_UNUSED; // reset
+    esp_tuple->tuple->esp_relay = 1;
+    fail_if(hip_fw_manage_esp_rule(esp_tuple, &dest, true) == 0,
+            "Added rule even though connection is relayed");
+
+    hip_userspace_ipsec = 1;
+    fail_if(hip_fw_manage_esp_rule(esp_tuple, &dest, true) == 0,
+            "Added rule even though userspace IPSEC requested");
+}
+END_TEST
+
+START_TEST(test_hip_fw_manage_esp_rule_inet6)
+{
+    mock_system = true;
+
+    static const char *const expected = "ip6tables -I HIPFW-INPUT -p 50 "
+                                        "-d 3ffe::1 -m esp --espspi 0xAABBCCDD "
+                                        "-j ACCEPT";
+
+    struct in6_addr dest;
+    assert(inet_pton(AF_INET6, "3ffe::1", &dest));
+
+    struct connection *const conn      = setup_connection();
+    struct esp_tuple  *const esp_tuple = setup_esp_tuple(0xAABBCCDD, &dest, conn);
+
+    esp_speedup         = 1;
+    hip_userspace_ipsec = 0;
+    conn->original.hook = NF_IP_LOCAL_IN;
+
+    fail_if(hip_fw_manage_esp_rule(esp_tuple, &dest, true) != 0);
+    fail_if(strcmp(mock_system_last, expected) != 0, "Unexpected rule was generated");
+}
+END_TEST
+
+START_TEST(test_hip_fw_manage_esp_rule_inet4)
+{
+    mock_system = true;
+
+    static const char *const expected = "iptables -I HIPFW-FORWARD -p 50 "
+                                        "-d 192.168.1.1 -m esp --espspi 0xAABBCCDD "
+                                        "-j ACCEPT";
+
+    struct in6_addr dest;
+    assert(inet_pton(AF_INET6, "::ffff:192.168.1.1", &dest));
+
+    struct connection *const conn      = setup_connection();
+    struct esp_tuple  *const esp_tuple = setup_esp_tuple(0xAABBCCDD, &dest, conn);
+
+    esp_speedup         = 1;
+    hip_userspace_ipsec = 0;
+    conn->original.hook = NF_IP_FORWARD;
+
+    fail_if(hip_fw_manage_esp_rule(esp_tuple, &dest, true) != 0);
+    fail_if(strcmp(mock_system_last, expected) != 0, "Unexpected rule was generated");
+}
+END_TEST
+
+START_TEST(test_hip_fw_manage_esp_rule_inet4_udp)
+{
+    mock_system = true;
+
+    static const char *const expected = "iptables -I HIPFW-OUTPUT -p UDP "
+                                        "--dport 10500 --sport 10500 -d 192.168.1.1 "
+                                        "-m u32 --u32 '4&0x1FFF=0 && 0>>22&0x3C@8=0xAABBCCDD' "
+                                        "-j ACCEPT";
+
+    struct in6_addr dest;
+    assert(inet_pton(AF_INET6, "::ffff:192.168.1.1", &dest));
+
+    struct connection *const conn      = setup_connection();
+    struct esp_tuple  *const esp_tuple = setup_esp_tuple(0xAABBCCDD, &dest, conn);
+
+    esp_speedup         = 1;
+    hip_userspace_ipsec = 0;
+    conn->original.hook = NF_IP_LOCAL_OUT;
+    conn->udp_encap     = true;
+
+    fail_if(hip_fw_manage_esp_rule(esp_tuple, &dest, true) != 0,
+            "Adding an iptables rule failed");
+    fail_if(!mock_system_last, "No iptables command was executed");
+    fail_if(strcmp(mock_system_last, expected) != 0, "Unexpected rule was generated");
+}
+END_TEST
+
 Suite *firewall_conntrack(void)
 {
     Suite *s = suite_create("firewall/conntrack");
 
     TCase *tc_conntrack = tcase_create("Conntrack");
     tcase_add_test(tc_conntrack, test_hip_fw_conntrack_periodic_cleanup_timeout);
+    tcase_add_test(tc_conntrack, test_parse_iptables_esp_rule);
     tcase_add_test(tc_conntrack, test_hip_fw_conntrack_periodic_cleanup_glitched_system_time);
     tcase_add_test(tc_conntrack, test_hip_fw_conntrack_periodic_cleanup_glitched_packet_time);
+    tcase_add_test(tc_conntrack, test_hip_fw_manage_esp_rule_not_enabled);
+    tcase_add_test(tc_conntrack, test_hip_fw_manage_esp_rule_needs_userspace);
+    tcase_add_test(tc_conntrack, test_hip_fw_manage_esp_rule_inet6);
+    tcase_add_test(tc_conntrack, test_hip_fw_manage_esp_rule_inet4);
+    tcase_add_test(tc_conntrack, test_hip_fw_manage_esp_rule_inet4_udp);
     suite_add_tcase(s, tc_conntrack);
 
     return s;

=== added file 'test/firewall/helpers.c'
--- test/firewall/helpers.c	1970-01-01 00:00:00 +0000
+++ test/firewall/helpers.c	2011-04-04 16:38:28 +0000
@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2011 Aalto University and RWTH Aachen University.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#define _BSD_SOURCE
+
+#include <check.h>
+#include <stdlib.h>
+
+#include "lib/core/common.h"
+#include "firewall/helpers.h"
+#include "test/mocks.h"
+#include "test_suites.h"
+
+
+START_TEST(test_system_printf)
+{
+    mock_system = true;
+
+    char str[MAX_COMMAND_LINE + 1];
+    memset(str, '.', sizeof(str));
+    str[MAX_COMMAND_LINE] = '\0';
+
+    fail_unless(system_printf("_%s",  str) == -1, "Truncated command line executed");
+    fail_unless(system_printf("%s", str)   ==  0, "Fitting command line not executed");
+}
+END_TEST
+
+Suite *firewall_helpers(void)
+{
+    Suite *s = suite_create("firewall/helpers");
+
+    TCase *tc_helpers = tcase_create("helpers");
+    tcase_add_test(tc_helpers, test_system_printf);
+    suite_add_tcase(s, tc_helpers);
+
+    return s;
+}

=== modified file 'test/firewall/test_suites.h'
--- test/firewall/test_suites.h	2011-04-04 16:38:28 +0000
+++ test/firewall/test_suites.h	2011-04-04 16:38:28 +0000
@@ -30,6 +30,7 @@
 
 Suite *firewall_conntrack(void);
 Suite *firewall_file_buffer(void);
+Suite *firewall_helpers(void);
 Suite *firewall_line_parser(void);
 Suite *firewall_port_bindings(void);