Re: [PATCH 1/9] ipv6: implement dataplane support for rthdr type 4 (Segment Routing Header)

[David Lebrun <david.lebrun@uclouvain.be>]

[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]

On 10/17/2016 07:01 PM, Tom Herbert wrote:
>> +struct ipv6_sr_hdr {
>> +       __u8    nexthdr;
>> +       __u8    hdrlen;
>> +       __u8    type;
>> +       __u8    segments_left;
>> +       __u8    first_segment;
>> +       __be16  flags;
> 
> Bad alignment for 16 bit field could be unpleasant on some
> architectures. Might be better to split this into to u8's, defined
> flags are only in first eight bits anyway.
> 

Will do

>> +config IPV6_SEG6
>> +       bool "IPv6: Segment Routing support"
>> +       depends on IPV6
>> +       select CRYPTO_HMAC
>> +       select CRYPTO_SHA1
>> +       select CRYPTO_SHA256
>> +       ---help---
>> +         Experimental support for IPv6 Segment Routing dataplane as defined
> 
> I don't think calling this experimental is relevant.

OK

>> +       if (skb->ip_summed == CHECKSUM_COMPLETE)
>> +               skb->ip_summed = CHECKSUM_NONE;
>> +
> Because the packet is being changed? Would it make sense to update the
> checksum complete value based on the changes being made. Consider the
> case that the next hop is local to the host (someone may try to
> implement network virtualization this way).
> 

Seems to make sense, I will try your suggestion

>>
>> +#ifdef CONFIG_IPV6_SEG6
>> +       /* segment routing */
>> +       if (hdr->type == IPV6_SRCRT_TYPE_4)
>> +               return ipv6_srh_rcv(skb);
>> +#endif
> 
> This doesn't belong in one of the switch statements in ipv6_rthdr_rcv?
> 

From what I see, ipv6_rthdr_rcv was initially implemented to support
RH0, and then specific code was added at multiple points to handle MIP6.
The first switch already handles a specific case (i.e. segments_left ==
0), so the call to ipv6_srh_rcv() must happen before that. I choose not
to inline ipv6_srh_rcv into ipv6_rthdr_rcv as it would make the code
quite messy.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]