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. What I'm suggesting is to send a bugfix to kernel
to accept only format with nested OVS_DEC_TTL_ATTR_ACTION. Since this
feature was never implemented in userspace OVS, this change will not
break anything. On the OVS side we should always format netlink messages
in a correct way. We have a code that checks feature existence in kernel
and it should fail if kernel is broken (as it is right now). In this case
OVS will continue to use old implementation with setting the ttl field.
Since the patch for a kernel is a bug fix, it should be likely backported
to stable versions, and distributions will pick it up too.
Thoughts?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
