[ovs-dev] [PATCH v2] datapath: Add a new action dec_ttl

[Ilya Maximets]

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 <mcroce at linux.microsoft.com>
> Co-developed-by: Bindiya Kurle <bindiyakurle at gmail.com>
> Signed-off-by: Eelco Chaudron <echaudro at 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>

> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 6eeda2a61..237776448 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -716,7 +716,58 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool steal,
>      }
>      dp_packet_batch_init_packet(&pb, packet);
>      odp_execute_actions(dp, &pb, true, nl_attr_get(subactions),
> -                        nl_attr_get_size(subactions), dp_execute_action);
> +                        nl_attr_get_size(subactions), dp_execute_action,
> +                        false);
> +}
> +
> +static bool execute_dec_ttl(struct dp_packet *packet)
> +{
> +    struct eth_header *eth = dp_packet_eth(packet);
> +
> +    if (dl_type_is_ipv4(eth->eth_type)) {
> +        struct ip_header *nh = dp_packet_l3(packet);
> +        uint8_t old_ttl = nh->ip_ttl;
> +
> +        if (old_ttl <= 1) {
> +            return true;
> +        }
> +
> +        nh->ip_ttl--;
> +        nh->ip_csum = recalc_csum16(nh->ip_csum, htons(old_ttl << 8),
> +                                    htons(nh->ip_ttl << 8));
> +    } else if (dl_type_is_ipv6(eth->eth_type)) {
> +        struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(packet);
> +
> +        if (nh->ip6_hlim <= 1) {
> +            return true;
> +        }
> +
> +        nh->ip6_hlim--;
> +    }
> +
> +    return false;
> +}
> +
> +static void odp_dec_ttl_exception_handler(void *dp,
> +                                          struct dp_packet_batch *batch,
> +                                          const struct nlattr *action,
> +                                          odp_execute_cb dp_execute_action)
> +{
> +    const struct nlattr *a;
> +    const struct nlattr *subaction = NULL;
> +    size_t left;
> +
> +    if (nl_attr_is_valid(action, nl_attr_get_size(action))) {
> +        dp_packet_delete_batch(batch, true);
> +        return;
> +    }
> +
> +    NL_NESTED_FOR_EACH_UNSAFE (a, left, action) {
> +        subaction = a;
> +        odp_execute_actions(dp, batch, true, subaction,
> +                            nl_attr_get_size(subaction),
> +                            dp_execute_action, true);
> +    }

I guess, that is the place that forces you to have this 'last_action' nonsence.
Code above tries to keep packets so they will not be freed and could be passed
to the next action.  But there should be no loop here.  The full list of actions
should be passed to odp_execute_actions() instead.  So it will decide what to
do with packets and will free them in the end of execution.

P.S. Current implementation will actually never free packets unless there is a
drop action in a list of actions.  If there will be no drop action, packets will
be leaked, IIUC.

Best regads, Ilya Maximets.