Re: [PATCH net-next 3/7] ipv6: improve ipv6_find_hdr() to skip empty routing headers

On Tue, Dec 4, 2012 at 10:15 AM, Jesse Gross [off-list ref] wrote:

On Mon, Dec 3, 2012 at 10:06 AM, Pablo Neira Ayuso [off-list ref] wrote:

quoted

On Mon, Dec 03, 2012 at 09:28:55AM -0800, Jesse Gross wrote:

quoted

On Mon, Dec 3, 2012 at 6:04 AM, Pablo Neira Ayuso [off-list ref] wrote:

quoted

On Thu, Nov 29, 2012 at 10:35:45AM -0800, Jesse Gross wrote:

quoted

@@ -159,9 +162,10 @@ int ipv6_find_hdr(const struct sk_buff *skb, unsigned int *offset,
      }
      len = skb->len - start;

-     while (nexthdr != target) {

If the offset is set as parameter via ipv6_find_hdr, we now are always
entering the loop even if we found the target header we're looking
for, before that didn't happen.

Something seems wrong here to me.

If the target header is a routing header then you might still need to
continue searching because the first one that you see could be empty.

OK, but if it's not a routing header what we're searching for (which
seems to be the case of netfilter/IPVS) we waste way more cycles on
copying the IPv6 header again and with way more things that are
completely useless.

We could add a check to short circuit this but it seems like a
premature optimization to me.

Ansis, can you comment?

Pablo, I think that the only concern you have here is about
optimizations, right?

We could either:
1. add an "if" statement that terminates loop early; or
2. switch back to "while () {}" loop and change condition from
"nexthdr != target" to something like "nexthdr != target || (nexthdr
== NEXTHDR_ROUTING && flags && (*flags & IP6_FH_F_SKIP_RH))".

This function seemed like a good candidate to extend it for this. I
think that iptables could make use of it too (to figure out whether L4
checksums need to be updated in presence of routing headers and NAT).

Thanks,
Ansis