[ovs-dev] Compliance of NSH implementation with latest NSH draft

Jan Scheurich jan.scheurich at ericsson.com
Wed Aug 16 16:21:45 UTC 2017


I checked the IETF tracker for NSH draft version 19:
It seems like final IETF reviews are concluding (deadline Aug 25), so we might consider updating the OVS implementation in 2.8 to align with version 19. At minimum this would mean restricting the nsh_mdtype field to 4 bits and possibly adjust the nsh_flags field to the changed bits in the base header.
I believe the we could add the nsh_ttl match field and a corresponding dec_nsh_ttl action at a later stage.
What do you think?
BR, Jan

From: Jan Scheurich
Sent: Wednesday, 16 August, 2017 16:05
To: Yang, Yi Y <yi.y.yang at intel.com>; Zoltán Balogh <zoltan.balogh at ericsson.com>
Cc: Ben Pfaff (blp at ovn.org) <blp at ovn.org>; Jiri Benc (jbenc at redhat.com) <jbenc at redhat.com>; ovs-dev at openvswitch.org
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

Hi Yi,
It is not correct that our implementation does not support MD2. OVS 2.8 will be able to receive and forward packets with NSH MD1 and MD2 headers (SFF use case). It should also be able to push an NSH header MD1 or MD2 format, with MD2 TLVs specified as encap properties, (Classifier use case) and pop an NSH header with MD1 or MD2 format (last SFF use case).
The only limitation OVS 2.8 has is that it cannot match on or modify MD2 TLV metadata. This will require the introduction of generic TLV match fields to be mapped to specific TLV protocol headers, which we have postponed to a future release.
Regarding the Netlink representation of NSH key and PUSH_NSH action:
For the NSH key we agreed with Jiri and Ben that the nested OVS_KEY_ATTR_NSH must have separate members  OVS_NSH_KEY_ATTR_BASE and OVS_NSH_KEY_ATTR_MD1, as presence of MD1 is optional. A specific MD1 attribute is required to carry the four fixed context headers as key fields.
We do have a problem with the NSH_KEY_BASE as the base header remains a moving target in the NSH draft. The format has evolved as follows from

0                   1                   2                   3

0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|Ver|O|C|R|R|R|R|R|R| Length    | MD type       | Next Protocol |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

in version 12 (Feb 2017), we based on, to
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|Ver|O|U|    TTL    |   Length  |U|U|U|U|MD Type| Next Protocol |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

in version 19 today (actually changed in June).
Field "nsh_mdtype" is shrunk from 8 to 4 bits, field "nsh_flags" would now split over two segments and shrunk from 8 to 2+4 bits, and there should be a new field "nsh_ttl".
So, whatever we release in OVS 2.8 will be compliant only to some version of the NSH draft (probably outdated). If we don't adjust to version 19, our "nsh_flags" field will contain the TTL value and the "nsh_mdtype" field includes some now unassigned bits. Will break as soon as future drafts start assigning bits previously part of the MD type field.
To me this indicates that we should mark the NSH implementation in OVS 2.8 as "experimental" and reserve the right to align the implementation with the approved NSH RFC later, even if that should break backward compatibility on the OpenFlow interface.
If that is not an option on the Netlink uAPI for the kernel datapath, we may not be able to offer NSH support with the kernel now. Or we accept that we need to supersede the initial OVS_NSH_KEY_ attributes with an updated version later on.

For the push_nsh action, the nested OVS_ACTION_ATTR_PUSH_NSH re-uses OVS_NSH_KEY_ATTR_BASE for the fixed part of the NSH header (inheriting the same issues as above).
But the push_nsh action needs no insight whatsoever in the context data. So the PUSH_NSH attribute just adds the opaque OVS_NSH_KEY_ATTR_CONTEXT carrying whatever context data as variable length byte array. This is a simple and future proof representation of arbitrarily formatted context data in the data path which can deal with both MD1 and MD2 already today.

BR Jan

From: Yang, Yi Y [mailto:yi.y.yang at intel.com]
Sent: Wednesday, 16 August, 2017 01:36
To: Jan Scheurich <jan.scheurich at ericsson.com<mailto:jan.scheurich at ericsson.com>>; Zoltán Balogh <zoltan.balogh at ericsson.com<mailto:zoltan.balogh at ericsson.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

Hi, Jan
I carefully considered netlink attributes, its core spirit is we shouldn't use variable-length netlink attribute if we can use fixed length, if you check OVS_KEY_ATTR_TUNNEL, that is a good example for this, actually that is just why Jiri, Eric and Ben hope we can change our implementation and follow this spirit, if we use OVS_NSH_KEY_ATTR to handle both MD1 and MD2, basically it violates this spirit, my another concern is we mustn't push more things about MD2 to this implementation, this will make people confused, we announced we can't support MD 2, why do we add some stuff here?
So my point is to use different netlink attributes for MD1 and MD2, that is clearer, to be important, MD1 attribute is length-fixed, for MD2, I don't think we have clear design for it, let us use another attribute to handle it specially. For implementation, what do you think the consolidated attribute OVS_NSH_KEY_ATTR_CONTEXT can bring?

From: Jan Scheurich [mailto:jan.scheurich at ericsson.com]
Sent: Wednesday, August 16, 2017 6:50 AM
To: Zoltán Balogh <zoltan.balogh at ericsson.com<mailto:zoltan.balogh at ericsson.com>>; Yang, Yi Y <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

I am suspecting the masked set action for NSH in the datapath flow

tunnel(tun_id=0x0,src=20.0.0.1,dst=20.0.0.2,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),nsh(spi=0x3020,si=255), packets:1, bytes:108, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),set(nsh(spi=0x3020,si=254)),pop_eth,
clone(tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:03,src=aa:55:00:00:00:02,
dl_type=0x0800),ipv4(src=20.0.0.2,dst=20.0.0.3,proto=17,tos=0,ttl=64,frag=0x4000),
udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc000004,vni=0x0)),out_port(2)),
set(ipv4(src=30.0.0.2,dst=30.0.0.3)),tnl_pop(4789))
which rewrites the SI to 254 to cause the corruption of the NSH header. There is quite some complexity in
commit_nsh() assembling the nested ACTION_MASKED_SET_ATTR with the nested KEY_NSH_ATTR. I haven't checked the code executing the masked set action for NSH in the datapath either.
BR, Jan

From: Jan Scheurich
Sent: Wednesday, 16 August, 2017 00:17
To: Zoltán Balogh <zoltan.balogh at ericsson.com<mailto:zoltan.balogh at ericsson.com>>; 'Yang, Yi Y' <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

I fixed the crash and another bug that caused md2 printout to have excess 8 bytes in push_nsh.
Changes pushed to gitlab.
Still the final triangle bridge test fails with a mismatch in datapath flows:
-tunnel(tun_id=0x0,src=30.0.0.2,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),nsh(np=1,spi=0x3020,si=254), packets:1, bytes:108, used:0.0s, actions:pop_nsh(),recirc(0x2)
-tunnel(tun_id=0x0,src=30.0.0.2,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0x2),in_port(4789),packet_type(ns=1,id=0x800),ipv4(frag=no), packets:1, bytes:84, used:0.0s, actions:push_eth(src=00:00:00:00:00:00,dst=aa:55:aa:55:00:03),6
+tunnel(tun_id=0x0,src=30.0.0.2,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),nsh(spi=0x300000,si=254), packets:1, bytes:108, used:0.0s, actions:drop
So the parsed NSH packet from the tunnel br-in2 to br-in3 is bad (missing np=1 and wrong spi). Either the push_nsh action didn't work, or the parse_nsh function screwed up. The funny thing is that the nsh packet must have passed the first tunnel correctly.
BR, Jan

From: Zoltán Balogh
Sent: Tuesday, 15 August, 2017 20:57
To: Jan Scheurich <jan.scheurich at ericsson.com<mailto:jan.scheurich at ericsson.com>>; 'Yang, Yi Y' <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

Hi,

I can confirm. There are 3 nsh unit tests that do fail on Jan's last commit. I attached gdb and created a backtrace for each case:

## ------------------------------ ##
## openvswitch 2.8.90 test suite. ##
## ------------------------------ ##

2390: nsh - md2 encap over a veth link FAILED (nsh.at:211)
2389: nsh - md1 encap over a veth link FAILED (nsh.at:85)
2391: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED (nsh.at:577)

## ------------- ##
## Test results. ##
## ------------- ##

ERROR: All 3 tests were run,
3 failed unexpectedly.
## -------------------------- ##
## testsuite.log was created. ##
## -------------------------- ##

Please send `tests/testsuite.log' and all information you think might help:

To: <bugs at openvswitch.org<mailto:bugs at openvswitch.org>>
Subject: [openvswitch 2.8.90] testsuite: 2389 2390 2391 failed

*** backtrace 2389

bt
#0 0x00007f18ca610428 in __GI_raise (sig=sig at entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1 0x00007f18ca61202a in __GI_abort () at abort.c:89
#2 0x00000000004a84d5 in format_odp_push_nsh_action (push_nsh=0x7ffdf233ff40, ds=0x7ffdf2341830) at lib/odp-util.c:381
#3 format_odp_action (ds=ds at entry=0x7ffdf2341830, a=a at entry=0x1b1b588, portno_names=portno_names at entry=0x0) at lib/odp-util.c:1071
#4 0x00000000004a908e in format_odp_actions (ds=0x7ffdf2341830, actions=0x1b1b588, actions_len=100, portno_names=0x0) at lib/odp-util.c:1097
#5 0x000000000043a19a in ofproto_trace__ (ofproto=ofproto at entry=0x1af4790, flow=flow at entry=0x7ffdf2341850, packet=0x0, recirc_queue=recirc_queue at entry=0x7ffdf2341790, ofpacts=ofpacts at entry=0x0, ofpacts_len=ofpacts_len at entry=0, output=0x7ffdf2341830) at ofproto/ofproto-dpif-trace.c:588
#6 0x000000000043a335 in ofproto_trace (ofproto=0x1af4790, flow=flow at entry=0x7ffdf2341850, packet=<optimized out>, ofpacts=ofpacts at entry=0x0, ofpacts_len=ofpacts_len at entry=0, next_ct_states=next_ct_states at entry=0x7ffdf2341820, output=0x7ffdf2341830) at ofproto/ofproto-dpif-trace.c:632
#7 0x000000000043a906 in ofproto_unixctl_trace (conn=0x1b009e0, argc=<optimized out>, argv=<optimized out>, aux=<optimized out>) at ofproto/ofproto-dpif-trace.c:427
#8 0x0000000000515e21 in process_command (request=0x1afedd0, conn=0x1b009e0) at lib/unixctl.c:313
#9 run_connection (conn=0x1b009e0) at lib/unixctl.c:347
#10 unixctl_server_run (server=0x1abd440) at lib/unixctl.c:398
#11 0x0000000000406cdf in main (argc=10, argv=0x7ffdf2341cf8) at vswitchd/ovs-vswitchd.c:120

p push_nsh->md_type
$2 = 0 '\000'

*** backtrace 2390

bt
#0 0x00007f18859b6428 in __GI_raise (sig=sig at entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1 0x00007f18859b802a in __GI_abort () at abort.c:89
#2 0x00000000004a84d5 in format_odp_push_nsh_action (push_nsh=0x7fff9307a1f0, ds=0x7fff9307bae0) at lib/odp-util.c:381
#3 format_odp_action (ds=ds at entry=0x7fff9307bae0, a=a at entry=0x1b4d468, portno_names=portno_names at entry=0x0) at lib/odp-util.c:1071
#4 0x00000000004a908e in format_odp_actions (ds=0x7fff9307bae0, actions=0x1b4d468, actions_len=104, portno_names=0x0) at lib/odp-util.c:1097
#5 0x000000000043a19a in ofproto_trace__ (ofproto=ofproto at entry=0x1b26790, flow=flow at entry=0x7fff9307bb00, packet=0x0, recirc_queue=recirc_queue at entry=0x7fff9307ba40, ofpacts=ofpacts at entry=0x0, ofpacts_len=ofpacts_len at entry=0, output=0x7fff9307bae0) at ofproto/ofproto-dpif-trace.c:588
#6 0x000000000043a335 in ofproto_trace (ofproto=0x1b26790, flow=flow at entry=0x7fff9307bb00, packet=<optimized out>, ofpacts=ofpacts at entry=0x0, ofpacts_len=ofpacts_len at entry=0, next_ct_states=next_ct_states at entry=0x7fff9307bad0, output=0x7fff9307bae0) at ofproto/ofproto-dpif-trace.c:632
#7 0x000000000043a906 in ofproto_unixctl_trace (conn=0x1b58a60, argc=<optimized out>, argv=<optimized out>, aux=<optimized out>) at ofproto/ofproto-dpif-trace.c:427
#8 0x0000000000515e21 in process_command (request=0x1b2c540, conn=0x1b58a60) at lib/unixctl.c:313
#9 run_connection (conn=0x1b58a60) at lib/unixctl.c:347
#10 unixctl_server_run (server=0x1aef440) at lib/unixctl.c:398
#11 0x0000000000406cdf in main (argc=10, argv=0x7fff9307bfa8) at vswitchd/ovs-vswitchd.c:120

p push_nsh->md_type
$2 = 0 '\000'

*** backtrace 2391

bt
#0 0x00007fba40955428 in __GI_raise (sig=sig at entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1 0x00007fba4095702a in __GI_abort () at abort.c:89
#2 0x00000000004a84d5 in format_odp_push_nsh_action (push_nsh=0x7ffc1ef37420, ds=0x7ffc1ef37630) at lib/odp-util.c:381
#3 format_odp_action (ds=ds at entry=0x7ffc1ef37630, a=a at entry=0x18fee18, portno_names=portno_names at entry=0x0) at lib/odp-util.c:1071
#4 0x00000000004a908e in format_odp_actions (ds=0x7ffc1ef37630, actions=0x18fee14, actions_len=156, portno_names=0x0) at lib/odp-util.c:1097
#5 0x000000000056483f in format_dpif_flow (ds=<optimized out>, f=<optimized out>, ports=<optimized out>, type=<optimized out>, dpctl_p=<optimized out>) at lib/dpctl.c:762
#6 0x0000000000566b50 in dpctl_dump_flows (argc=<optimized out>, argc at entry=2, argv=argv at entry=0x18fb810, dpctl_p=dpctl_p at entry=0x7ffc1ef39c50) at lib/dpctl.c:923
#7 0x0000000000563f90 in dpctl_unixctl_handler (conn=0x18fb730, argc=2, argv=0x18fb810, aux=0x5667e0 <dpctl_dump_flows>) at lib/dpctl.c:2004
#8 0x0000000000515e21 in process_command (request=0x1900960, conn=0x18fb730) at lib/unixctl.c:313
#9 run_connection (conn=0x18fb730) at lib/unixctl.c:347
#10 unixctl_server_run (server=0x17e9440) at lib/unixctl.c:398
#11 0x0000000000406cdf in main (argc=10, argv=0x7ffc1ef39ea8) at vswitchd/ovs-vswitchd.c:120

p push_nsh->md_type
$2 = 144 '\220'

As you can see, the value of push_nsh->md_type is either 0x00 or 0x90 in format_odp_push_nsh_action() and this leads to abort.

Best regards,
Zoltan

-------- Original Message --------
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements
From: Jan Scheurich <jan.scheurich at ericsson.com<mailto:jan.scheurich at ericsson.com>>
Date: Aug 15, 2017, 18:03
To: "'Yang, Yi Y'" <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>,Zoltán Balogh <zoltan.balogh at ericsson.com<mailto:zoltan.balogh at ericsson.com>>
Hi,
I have pushed my updates to a new branch "ovs_key_nsh_rework_jan" in gitlab.
The codes does compile but the NSH unit tests fail due to a crash of the vswitchd after ofproto/receive of an NSH packet.
I haven't had time to trouble-shoot and fix this yet.
Please look into that if you have time because I will be largely unavailable tomorrow.
BR, Jan

From: Jan Scheurich
Sent: Tuesday, 15 August, 2017 11:47
To: 'Yang, Yi Y' <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>; Zoltán Balogh <zoltan.balogh at ericsson.com<mailto:zoltan.balogh at ericsson.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

Hi Yi,
I'll be providing my comments as a delta patch on your last commit.
As agreed I am changing the ATTR_ACTION_PUSH_NSH to a structured action attribute with ATTR_KEY_NSH_BASE and ATTR_KEY_NSH_CONTEXT, which is a flat variable length byte sequence. The push_nsh action does not need awareness of the MD type and structure. The MD length comes from the nested attribute length.
The kernel datapath doesn't compile with your latest commit as you have removed struct ovs_action_push_nsh from openvswitch.h which is used in the current versions of datapath/actions.c.
It seems also the datapath needs an optimized internal struct representation of the push_nsh action, and I think we should apply the same concept in the user space datapath. Just need to find out how.
BR, Jan

From: Jan Scheurich
Sent: Tuesday, 15 August, 2017 09:47
To: Yang, Yi Y <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>; Zoltán Balogh <zoltan.balogh at ericsson.com<mailto:zoltan.balogh at ericsson.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

Hi Yi,
Thanks for providing the changes. I'm having a look right now. Since they are significant that might take me a while.
/Jan
From: Yang, Yi Y [mailto:yi.y.yang at intel.com]
Sent: Tuesday, 15 August, 2017 06:07
To: Zoltán Balogh <zoltan.balogh at ericsson.com<mailto:zoltan.balogh at ericsson.com>>; Jan Scheurich <jan.scheurich at ericsson.com<mailto:jan.scheurich at ericsson.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

I have fixed the issues, I'll send out kernel datapath patch v3 if you don't have other comments about new changes.

From: Zoltán Balogh [mailto:zoltan.balogh at ericsson.com]
Sent: Monday, August 14, 2017 11:52 PM
To: Jan Scheurich <jan.scheurich at ericsson.com<mailto:jan.scheurich at ericsson.com>>; Yang, Yi Y <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

Hello Yi,

I run make check on your branch and 3 tests failed:



## ------------- ##

## Test results. ##

## ------------- ##



ERROR: 2390 tests were run,

3 failed unexpectedly.

1 test was skipped.

## -------------------------- ##

## testsuite.log was created. ##

## -------------------------- ##



Please send `tests/testsuite.log' and all information you think might help:



   To: <bugs at openvswitch.org<mailto:bugs at openvswitch.org>>

   Subject: [openvswitch 2.8.90] testsuite: 953 997 2391 failed

Do these tests fail in your setup as well? Tomorrow, I'm going to continue with the debug.

Best regards,
Zoltan

From: Zoltán Balogh
Sent: Monday, August 14, 2017 4:13 PM
To: Jan Scheurich <jan.scheurich at ericsson.com<mailto:jan.scheurich at ericsson.com>>; Yang, Yi Y <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

Hi,
Sure, I can. I'll be back with updates.
/Zoltan

From: Jan Scheurich
Sent: Monday, August 14, 2017 4:08 PM
To: Yang, Yi Y <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>
Cc: Zoltán Balogh <zoltan.balogh at ericsson.com<mailto:zoltan.balogh at ericsson.com>>
Subject: RE: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

Hi Yi,
Zoltan is back from vacation today.
@Zoltan: Can you have a look at this issue and help Yi?
Thanks, Jan

From: Yang, Yi Y [mailto:yi.y.yang at intel.com]
Sent: Monday, 14 August, 2017 16:02
To: Yang, Yi Y <yi.y.yang at intel.com<mailto:yi.y.yang at intel.com>>; Jan Scheurich <jan.scheurich at ericsson.com<mailto:jan.scheurich at ericsson.com>>
Subject: Yang, Yi Y sent you a message in Skype for Business-FYI: basically I have worked out a ovs nsh version per their requirements

Yang, Yi Y 15:59:
Hi, Jan, I pushed origin/ovs_key_nsh_rework  to gitlab
Yang, Yi Y 16:00:
it will be great if you can have a check.
Yang, Yi Y 16:02:
nsh unit test 2391 failed, I'm still debugging it, I have no idea about why it is so, it will be great if you can help with this issue.


More information about the dev mailing list