On Mon, Nov 5, 2012 at 8:58 PM, Jesse Gross <je...@nicira.com> wrote:
> On Sun, Nov 4, 2012 at 7:44 AM, Ansis Atteka <aatt...@nicira.com> wrote:
>> On Tue, Oct 30, 2012 at 1:11 AM, Jesse Gross <je...@nicira.com> wrote:
>>> On Sun, Oct 28, 2012 at 2:21 PM, Ansis Atteka <aatt...@nicira.com> wrote:
>>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>>> index ec9b595..7e60b0f 100644
>>>> --- a/datapath/actions.c
>>>> +++ b/datapath/actions.c
>>>> +static void set_ipv6_addr(struct sk_buff *skb, struct ipv6hdr *nh,
>>>> + __be32 addr[4], const __be32 new_addr[4])
>>>> +{
>>>> + int transport_len = skb->len - skb_transport_offset(skb);
>>>> +
>>>> + if (nh->nexthdr == IPPROTO_TCP) {
>>>> + if (likely(transport_len >= sizeof(struct tcphdr)))
>>>> + inet_proto_csum_replace16(&tcp_hdr(skb)->check,
>>>> skb,
>>>> + addr, new_addr, 1);
>>>> + } else if (nh->nexthdr == IPPROTO_UDP) {
>>>> + if (likely(transport_len >= sizeof(struct udphdr))) {
>>>> + struct udphdr *uh = udp_hdr(skb);
>>>> +
>>>> + if (uh->check ||
>>>> + get_ip_summed(skb) == OVS_CSUM_PARTIAL) {
>>>> + inet_proto_csum_replace16(&uh->check, skb,
>>>> + addr, new_addr,
>>>> 1);
>>>> + if (!uh->check)
>>>> + uh->check = CSUM_MANGLED_0;
>>>> + }
>>>> + }
>>>> + }
>>>
>>> IPv6 has an extra little twist when it comes to checksums compared to
>>> IPv4: routing headers. In an IPv6 packet, the destination IP address
>>> used for the purposes of checksum calculation in the pseudo header is
>>> the final destination address. This means that if you have a routing
>>> header then it is that address that matters and not one in the IP
>>> header. This is likely to be a pretty rare case but we should handle
>>> it.
>> There could be a chain of ipv6 extension headers. And the Routing
>> Header could be in the middle of that chain. Can you confirm, if
>> ipv6_find_hdr() function is good enough to get the routing header?
>
> Yes, it seems to be basically fine. Two things that come to mind are:
> what happens if there are multiple routing headers (is this legal?)
RFC3542 has implication "When multiple Routing headers are received
...". This makes me think that multiple routing headers are legal.
This means that L4 checksum shouldn't be updated, if there exists at
least one routing header, where segments_left > 0. Otherwise, we have
to update L4 checksums.
> and ipv6_find_hdr() is in the ip6_tables module so using it will
> create a dependency, which we don't want. We probably should move it
> somewhere else.
This ipv6_find_hdr() function seems to be able to handle only single
routing header. Perhaps I will write my own function that will iterate
over all extension headers and tell the caller, whether the
ipv6hdr->daddr is the final destination address.
>
> You might want to look to see how IPv6 NAT handles this case currently.
Either I am overlooking that IPv6 NAT code or it does not handle
routing headers and checksums as I would have expected.
The nf_nat_ipv6_csum_update() is called, for example, from
tcp_manip_pkt(). And it seems that it always updates L4 checksums
without taking into account routing headers. Will check it tomorrow
morning one more time.
>
>> Also, in the routing header there could be multiple ipv6 addresses
>> that need to be visited. I guess, that, If routing header is present,
>> but ipv6_rt_hdr.segments_left is equal to 0, then we would still need
>> to change the ipv6 destination address in the main IPv6 header and not
>> the last ipv6 address in the routing header (rfc2460; 4.4). Is that
>> right?
>>
>> Type 0 Routing Header seems to be deprecated by rfc5095. To make
>> things simpler, would it make sense to:
>> 1. always change the nh.daddr; and
>> 2. update L4 checksums, if and only if nh.daddr is also the final
>> destination address?
>> Maybe you already assumed that we should always change nh.daddr and
>> not the ipv6_rt_hdr.last_daddr? I couldn't quite tell from your
>> paragraph above.
>
> Yes, I think that this function should always act on the destination
> address in the IPv6 base header, as you have it currently. We just
> need to figure out if there is a routing header present in the chain
> and if so not update the L4 checksum. We probably have to check
> whether the routing header we find actually has any segments left but
> I wouldn't worry about handling RH0 specially. Since we're not
> actually doing any routing here but only updating the checksum we
> should just follow those rules and other devices can enforce security
> policies.
>
>>>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,6,0)
>>>> +static inline void inet_proto_csum_replace16(__sum16 *sum,
>>>
>>> I think this function is coming in 3.7, not 3.6.
>> This function was added just before 3.6-rc2 git tag. So does this mean
>> that I have to check against 3.7 version instead?
>>
>> git describe --tags 2cf545e835aae92173ef0b1f4af385e9c40f21e8
>
> In this case, yes. Since there are always two releases going on in
> parallel, what happened here is that this change went into the
> development version while the previous version was in the RC phase.
> git describe is useful as a starting point but it's usually best to
> check the code to see where things actually landed.
Makes sense.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
