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.
>
>>> +
>>> + 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.
>
>>> + ds_put_format(ds, "srv6(");
>>> + ds_put_format(ds, "segments_left=%d", srh->segments_left);
>>> + ds_put_format(ds, ",segs=");
>>> + for (i = 0; i < nr_segs; i++) {
>>> + ds_put_format(ds, i > 0 ? "," : "");
>>> + ipv6_format_addr(&segs[nr_segs - i - 1], ds);
>>> + }
>>> ds_put_char(ds, ')');
>>
>> This is a formatting part, but we also need a parsing part in
>> ovs_parse_tnl_push(). And the unit test in tests/odp.at.
>> With the unit test you will also notice that we need a python
>> parsing library updated for this new format string.
>> See the python/ovs/flow/odp.py and python/ovs/tests/test_odp.py.
>> The parsing library is relatively new, it was added in 3.0.
>>
>
> I will add the parsing part. I will also check about python parsing
> library.
>
>>> 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.
>
>>> 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!
>
> Best Regards,
> Nobuhiro MIKI
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
