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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 Oct 2016 13:43:03 +0200
From:   David Lebrun <david.lebrun@...ouvain.be>
To:     Tom Herbert <tom@...bertland.com>
CC:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/9] ipv6: implement dataplane support for rthdr type
 4 (Segment Routing Header)

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.


Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)