Re: [PATCH net-next v4 3/9] ipv6: sr: add support for SRH encapsulation and injection with lwtunnels

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

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

On 11/04/2016 05:26 PM, Tom Herbert wrote:
>> +
>> +int seg6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>> +{
>> +       struct dst_entry *orig_dst = skb_dst(skb);
>> +       struct dst_entry *dst = NULL;
>> +       struct seg6_lwt *slwt;
>> +       int err = -EINVAL;
>> +
>> +       err = seg6_do_srh(skb);
> 
> Technically we're not allowed by the standard to insert extension
> headers when forwarding, only the source host can place EH in packets.
> There was a _long_ discussion about this in 6man WG and it appears
> that for RFC2460bis the plan is to make this point clear. The
> rationale is that inserting extension headers in the middle of the
> network break PMTUD, IPsec AH, amongst other things.
> 
> I think people are going to do this anyway (especially for something
> like SR) so I don't think we should abandon this patch. But, we
> probably need a big disclaimer documented that if someone does this
> they may see problems in the network (in other words they should only
> use this if they know what they are doing).
> 

Agreed that directly inserting an EH breaks a lot of things. Note that
this concerns only seg6_do_srh_inline(). The other function,
seg6_do_srh_encap(), uses an outer IPv6 header to carry the SRH which is
RFC compliant. Also agreed that people are going to use direct insertion
regardless of RFC.

Where do you think the disclaimer should be ? Documentation/ file ?
Perhaps I can create a specific option CONFIG_IPV6_SEG6_INLINE to enable
or disable direct insertion.


>> +
>> +       memcpy(&slwt->tuninfo, tuninfo, tuninfo_len);
>> +
> Thomas pointed out to me that we are just blindly copying the SR
> option from userspace. We really should validate that it is well
> formed and acceptable to send. Minimally, we should check that
> addresses are valid, the TLVs are well formed, and we need to decide
> on rather to allow arbitrary TLVs that are unknown to the kernel. Same
> thing should be true for socket options or other interfaces to program
> SR.
> 

Yep, I overlooked that part of the patch and this indeed also applies to
the setsockopt() interface. I will respin a patch series with this issue
fixed.

Thanks for the feedback

David


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