On 2023/03/22 8:43, Ilya Maximets wrote:
> On 3/22/23 00:41, Ilya Maximets wrote:
>> On 3/15/23 07:07, Nobuhiro MIKI wrote:
>>> This patch adds ODP actions for SRv6 and its tests.
>>>
>>> Signed-off-by: Nobuhiro MIKI <nm...@yahoo-corp.jp>
>>> ---
>>> lib/odp-util.c | 56 +++++++++++++++++++++++++++++++++++
>>> python/ovs/flow/odp.py | 8 +++++
>>> python/ovs/tests/test_odp.py | 16 ++++++++++
>>> tests/odp.at | 1 +
>>> tests/tunnel-push-pop-ipv6.at | 23 ++++++++++++++
>>> 5 files changed, 104 insertions(+)
>>>
>>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>>> index dbd4554d0626..75f4d5242183 100644
>>> --- a/lib/odp-util.c
>>> +++ b/lib/odp-util.c
>>> @@ -714,6 +714,24 @@ format_odp_tnl_push_header(struct ds *ds, struct
>>> ovs_action_push_tnl *data)
>>> ds_put_char(ds, ')');
>>> }
>>>
>>> + ds_put_char(ds, ')');
>>> + } else if (data->tnl_type == OVS_VPORT_TYPE_SRV6) {
>>> + const struct srv6_base_hdr *srh;
>>> + struct in6_addr *segs;
>>> + int nr_segs;
>>> + int i;
>>> +
>>> + srh = (const struct srv6_base_hdr *) l4;
>>> + segs = ALIGNED_CAST(struct in6_addr *, srh + 1);
>>> + nr_segs = srh->last_entry + 1;
>>> +
>>> + ds_put_format(ds, "srv6(");
>>> + ds_put_format(ds, "segments_left=%d", srh->rt_hdr.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, ')');
>>> } else if (data->tnl_type == OVS_VPORT_TYPE_GRE ||
>>> data->tnl_type == OVS_VPORT_TYPE_IP6GRE) {
>>> @@ -1534,6 +1552,7 @@ ovs_parse_tnl_push(const char *s, struct
>>> ovs_action_push_tnl *data)
>>> uint8_t hwid, dir;
>>> uint32_t teid;
>>> uint8_t gtpu_flags, gtpu_msgtype;
>>> + uint8_t segments_left;
>>>
>>> if (!ovs_scan_len(s, &n, "tnl_push(tnl_port(%"SCNi32"),",
>>> &data->tnl_port)) {
>>> return -EINVAL;
>>> @@ -1775,6 +1794,43 @@ ovs_parse_tnl_push(const char *s, struct
>>> ovs_action_push_tnl *data)
>>> tnl_type = OVS_VPORT_TYPE_GTPU;
>>> header_len = sizeof *eth + ip_len +
>>> sizeof *udp + sizeof *gtph;
>>> + } else if (ovs_scan_len(s, &n, "srv6(segments_left=%"SCNu8,
>>> + &segments_left)) {
>>> + struct srv6_base_hdr *srh = (struct srv6_base_hdr *) (ip6 + 1);
>>> + uint8_t n_segs = segments_left + 1;
>>> + char seg_s[IPV6_SCAN_LEN + 1];
>>> + struct in6_addr *segs;
>>> + struct in6_addr seg;
>>> +
>>> + ip6->ip6_nxt = IPPROTO_ROUTING;
>>> +
>>> + srh->rt_hdr.type = IPV6_SRCRT_TYPE_4;
>>> + srh->rt_hdr.segments_left = segments_left;
>>> + srh->rt_hdr.hdrlen = 2 * n_segs;
>>> + srh->last_entry = n_segs - 1;
>>> +
>>> + tnl_type = OVS_VPORT_TYPE_SRV6;
>>> + header_len = sizeof *eth + ip_len +
>>> + sizeof *srh + 8 * srh->rt_hdr.hdrlen;
>>> +
>>> + /* Parse segment list */
>>> + if (!ovs_scan_len(s, &n, ",segs="IPV6_SCAN_FMT, seg_s)
>>> + || inet_pton(AF_INET6, seg_s, &seg) != 1) {
>>> + return -EINVAL;
>>> + }
>>> +
>>> + segs = ALIGNED_CAST(struct in6_addr *, srh + 1);
>>> + segs += n_segs - 1;
>>> + memcpy(segs--, &seg, sizeof(struct in6_addr));
>>> +
>>> + while (ovs_scan_len(s, &n, ","IPV6_SCAN_FMT, seg_s)
>>> + && inet_pton(AF_INET6, seg_s, &seg) == 1) {
>>> + memcpy(segs--, &seg, sizeof(struct in_addr));
>>
>> We will overwrite the header here if segments_left + 1 is less
>> than total number of specified segments. E.g. in the following
>> case: srv6(segments_left=0,segs=2001:cafe::92,2001:cafe::93).
>>
>> One solution for this may be to create an SRV6_MAX_SEGS + 1 sized
>> array and parse values there. Once all the values parsed,
>> copy them into an actual header. If more than SRV6_MAX_SEGS
>> values found, then fail the parsing.
>>
>> What do you think?
>>
Thanks again for your review.
In this process of generating a new header, I think it is necessary
to ensure that the number of parsed segments is exactly the same as
segments_left + 1.
Of course, it is also necessary to check that the number of segments
is less than SRV6_MAX_SEGS.
1. make sure segments_left + 1 <= SRV6_MAX_SEGS.
2. put parsed values into SRV6_MAX_SEGS + 1 sized array.
3. fail if the number of the parses values is different from
segments_left + 1.
>> Also, the sizeof seems incorrect here. It should be in6, not in.
>> It also should be just 'sizeof *segs' to avoid any potential
>> problems of this kind.
OK. I'll fix it.
>>
>> Would be good to add a test to tests/odp.at for many segments
>> parsing while segments_left is small.
I'll add some test cases.
>>
>> And it's probably possible to avoid some code duplication here.
>> E.g. (didn't test):
>>
>> if (strncmp(s, ",segs=", 6)) {
>> return -EINVAL;
>> }
>> s += 6;
>
> We need to check &s[n] and advance n, of course. :/
>
>> while (ovs_scan_len(s, &n, IPV6_SCAN_FMT, seg_s)
>> && inet_pton(AF_INET6, seg_s, &seg) == 1) {
>>
>> ...
>>
>> if (s[n] == ',') {
>> s++;
>
> n++ here.
>
>> }
>> }
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
