On 2023/01/07 4:14, Ilya Maximets wrote:
> On 1/6/23 08:58, Nobuhiro MIKI wrote:
>> On 2023/01/03 7:59, Ilya Maximets wrote:
>>> On 10/11/22 08:11, Nobuhiro MIKI wrote:
>>>> SRv6 (Segment Routing IPv6) tunnel vport is responsible
>>>> for encapsulation and decapsulation the inner packets with
>>>> IPv6 header and an extended header called SRH
>>>> (Segment Routing Header). See spec in:
>>>>
>>>> https://datatracker.ietf.org/doc/html/rfc8754
>>>>
>>>> This patch implements SRv6 tunneling in userspace datapath.
>>>> It uses `remote_ip` and `local_ip` options as with existing
>>>> tunnel protocols. It also adds a dedicated `srv6_segs` option
>>>> to define a sequence of routers called segment list.
>>>>
>>>> Signed-off-by: Nobuhiro MIKI <nm...@yahoo-corp.jp>
>>>
>>> Hi. Thanks for the patch and sorry for the late reply.
>>> See some coments inline.
>>>
>>> Best regards, Ilya Maximets.
>>>
>>
>> Hi Ilya,
>>
>> Thanks for your kind review.
>> I have written comments on the points that need to be discussed.
>> I'll fix the other parts including the code format in the next patch.
>>
>>>> +netdev_srv6_pop_header(struct dp_packet *packet)
>>>> +{
>>>> + struct pkt_metadata *md = &packet->md;
>>>> + struct flow_tnl *tnl = &md->tunnel;
>>>> + struct srv6_base_hdr *srh;
>>>> + int hlen = ETH_HEADER_LEN + IPV6_HEADER_LEN;
>>>
>>> We assume here that packet doesn't have any other extension
>>> headers. Is that always true? Should we also check that
>>> the next header is actually a routing header? And if it is
>>> a type 4 header?
>>>
>>
>> Other extension headers can also be inserted. And since those extension
>> headers are in no particular order (although there is a recommended
>> order), we need to loop through all the extension headers to make sure
>> SRH is present. We also need to check that it is type 4.
>
> We have a function that partially parses extension headers
> in lib/flow.c - parse_ipv6_ext_hdrs(). It advances the
> data pointer to the first non-extension header and collects
> some data along the way. We may probably enhance it to
> additionally return a pointer to the RH/SRH header if present.
>
Thanks! I think it works.
>>
>>>> +
>>>> + srh = ALIGNED_CAST(struct srv6_base_hdr *,
>>>> + (char *) dp_packet_data(packet) + hlen);
>>>> + if (srh->segments_left > 0) {
>>>> + VLOG_WARN_RL(&err_rl, "invalid srv6 segments_left=%d\n",
>>>> + srh->segments_left);
>>>
>>> I suppose, this means that we do not support receiving
>>> packets with a segment list specified. It might be OK,
>>> but the limitation should be documented somewhere.
>>>
>>> Is there a reason to not accept such packets? i.e. not
>>> really decap the packet, but swap the destination IPs.
>>>
>>
>> I recognize the question as to whether or not to implement SRv6 End
>> function. I think it would be appropriate to reject the packet at this
>> time for the following two reasons:
>>
>> * We can be sure that all packets going out to type=srv6 vport are decapped.
>> This is consistent with existing tunneling because it is the same behavior.
>
> I don't think there should be a problem if the packet is not
> actually decaped, because OVS will fully re-parse the packet
> from scratch after executing the decap() action. So, I guess,
> we should be fine here. But, of course, I didn't test.
>
>> * We can take the option to accept the packet which is segments_left > 0
>> for SRv6 End function in the future.
>
> OK, sure. If you can add a note about this part not being
> supported in the FAQ, that can be enough for now.
>
I did not know about re-parse.
At this time, I will state in the FAQ that it is unsupported now.
>>>> diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
>>>> index 050eafa6b..33313d8fd 100644
>>>> --- a/lib/tnl-ports.c
>>>> +++ b/lib/tnl-ports.c
>>>> @@ -126,7 +126,7 @@ map_insert(odp_port_t port, struct eth_addr mac,
>>>> struct in6_addr *addr,
>>>> /* XXX: No fragments support. */
>>>> match.wc.masks.nw_frag = FLOW_NW_FRAG_MASK;
>>>>
>>>> - /* 'tp_port' is zero for GRE tunnels. In this case it
>>>> + /* 'tp_port' is zero for GRE and SRv6 tunnels. In this case it
>>>> * doesn't make sense to match on UDP port numbers. */
>>>
>>> Currently to detect that a tunnel port should receive a packet
>>> we're matching on the protocol and the port number.
>>>
>>> GRE is an exception, since it's another IP in IP protocol, but
>>> it has a distinct protocol number and a special header, so it's
>>> simple to detect.
>>>
>>> However, with SRv6 we have a generic IPv6 header with generic
>>> IPIP or IPV6 network protocol number. It looks like OVS will
>>> intercept any other IPIP or IPv6|IPv6 packets, try to decapsuate
>>> them as if they are SRv6 and drop if they are not.
>>> This miht be an unwanted behavior.
>>>
>>> I wonder if we actually need to match on the exstence of the
>>> routing extension header. For that we'll need parts of the
>>> following chnage:
>>> https://patchwork.ozlabs.org/project/openvswitch/patch/20221118212055.3541-1-cpp.code...@gmail.com/
>>>
>>> In any case it should be documented, I think, that addition
>>> of the SRv6 port may cause problems for IPIP traffic destined
>>> to the node.
>>>
>>>
>>
>> Yes, unlike VXLAN and GRE, SRv6 is special and must be detected by IPv6
>> routing extended header which has routing type 4.
>>
>> In the patch you mention, the parsing part of the extended header seems
>> to be applied. I will add the documentation as well.
>
> Ok.
>
> The patch I mentioned is far from being ready and I would not expect
> it to be accepted anytime soon.
> The documentation bit describing the limitation will suffice for now.
>
Sure. I'll add the documentation.
>>>> diff --git a/tests/tunnel-push-pop-ipv6.at b/tests/tunnel-push-pop-ipv6.at
>>>> index c96b77cd1..249c3bf1e 100644
>>>> --- a/tests/tunnel-push-pop-ipv6.at
>>>> +++ b/tests/tunnel-push-pop-ipv6.at
>>>
>>> Beside these "unit" tests it would also be great to have system
>>> tests in tests/system-traffic.at that will show interoperability
>>> between our and native kernel's implementation.
>>>
>>
>> I see, I'll try it.
>
> Thanks!
>
Thanks for the review. I feel we agree on the direction and
will start preparing the next patch.
Best Regards,
Nobuhiro MIKI
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
