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 <[email protected]>
>
> 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.
>> +
>> + 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.
* We can take the option to accept the packet which is segments_left > 0
for SRv6 End function in the future.
>> + 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/[email protected]/
>
> 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.
>> 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.
Best Regards,
Nobuhiro MIKI
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
