On 13 Nov 2020, at 17:05, Ilya Maximets wrote:
Cc: netdev
On 11/13/20 3:28 PM, Eelco Chaudron wrote:
On 13 Nov 2020, at 13:06, Ilya Maximets wrote:
On 11/13/20 11:04 AM, Eelco Chaudron wrote:
Add support for the dec_ttl action. Instead of programming the
datapath with
a flow that matches the packet TTL and an IP set, use a single
dec_ttl action.
The old behavior is kept if the new action is not supported by the
datapath.
# ovs-ofctl dump-flows br0
cookie=0x0, duration=12.538s, table=0, n_packets=4,
n_bytes=392, ip actions=dec_ttl,NORMAL
cookie=0x0, duration=12.536s, table=0, n_packets=4,
n_bytes=168, actions=NORMAL
# ping -c1 -t 20 192.168.0.2
PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data.
IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP
(1), length 84)
192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865,
seq 1, length 64
Linux netlink datapath support depends on upstream Linux commit:
744676e77720 ("openvswitch: add TTL decrement action")
Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has
been
defined, and to make sure the IDs are in sync, it had to be added
to the
OVS source tree. This required some additional case statements,
which
should be revisited once the OVS implementation is added.
Co-developed-by: Matteo Croce <mcr...@linux.microsoft.com>
Co-developed-by: Bindiya Kurle <bindiyaku...@gmail.com>
Signed-off-by: Eelco Chaudron <echau...@redhat.com>
---
v2: - Used definition instead of numeric value in
format_dec_ttl_action()
- Changed format from "dec_ttl(ttl<=1(<actions>)) to
"dec_ttl(le_1(<actions>))" to be more in line with the
check_pkt_len action.
- Fixed parsing of "dec_ttl()" action for adding a dp flow.
- Cleaned up format_dec_ttl_action()
datapath/linux/compat/include/linux/openvswitch.h | 8 ++
lib/dpif-netdev.c
| 4 +
lib/dpif.c
| 4 +
lib/odp-execute.c
| 102 ++++++++++++++++++++-
lib/odp-execute.h
| 2
lib/odp-util.c
| 42 +++++++++
lib/packets.h
| 13 ++-
ofproto/ofproto-dpif-ipfix.c
| 2
ofproto/ofproto-dpif-sflow.c
| 2
ofproto/ofproto-dpif-xlate.c
| 54 +++++++++--
ofproto/ofproto-dpif.c
| 37 ++++++++
ofproto/ofproto-dpif.h
| 6 +
12 files changed, 253 insertions(+), 23 deletions(-)
<snip>
+static void
+format_dec_ttl_action(struct ds *ds,const struct nlattr *attr,
+ const struct hmap
*portno_names)
+{
+ const struct nlattr *nla_acts = nl_attr_get(attr);
+ int len = nl_attr_get_size(attr);
+
+ ds_put_cstr(ds,"dec_ttl(le_1(");
+
+ if (len > 4 && nla_acts->nla_type ==
OVS_DEC_TTL_ATTR_ACTION) {
+ /* Linux kernel add an additional envelope we
should strip. */
+ len -= nl_attr_len_pad(nla_acts, len);
+ nla_acts = nl_attr_next(nla_acts);
CC: Pravin
I looked at the kernel and I agree that there is a clear bug in
kernel's
implementaion of this action. It receives messages on format:
OVS_ACTION_ATTR_DEC_TTL(<list of actions>),
but reports back in format:
OVS_ACTION_ATTR_DEC_TTL(OVS_DEC_TTL_ATTR_ACTION(<list of
actions>)).
Since 'OVS_DEC_TTL_ATTR_ACTION' exists, it's clear that original
design
was to have it, i.e. the correct format should be the form that
kernel reports back to userspace. I'd guess that there was a plan
to
add more features to OVS_ACTION_ATTR_DEC_TTL in the future, e.g. set
actions execution threshold to something different than 1, so it
make
some sense.
Anyway, the bug is in the kernel part of parsing the netlink message
and
it should be fixed.
It is already in the mainline kernel, so changing it now would break
the UAPI.
Don't think this is allowed from the kernel side.
Well, UAPI is what specified in include/uapi/linux/openvswitch.h. And
it says:
OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
So, the action must have nested OVS_DEC_TTL_ATTR_ACTION, otherwise
it's malformed.
This means that UAPI is broken now in terms that kernel doesn't
respect it's
own UAPI. And that's a bug that should be fixed.
Ack, will cook up a patch, and sent it to net.
//Eelco
