Re: [ovs-dev] [PATCH net-next v3] openvswitch: add TTL decrement action

2020-01-16 Thread Nicolas Dichtel
Le 16/01/2020 à 08:21, Pravin Shelar a écrit :
> On Wed, Jan 15, 2020 at 8:40 AM Matteo Croce  wrote:
[snip]
>> @@ -1050,4 +1051,5 @@ struct ovs_zone_limit {
>> __u32 count;
>>  };
>>
>> +#define OVS_DEC_TTL_ATTR_EXEC  0
> 
> I am not sure if we need this, But if you want the nested attribute
> then lets define enum with this single attribute and have actions as
> part of its data. This would be optional argument, so userspace can
> skip it, and in that case datapath can drop the packet.
And note that 0 is a reserved value and should not be used. Look at other
attributes.

Regards,
Nicolas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH net-next v3] openvswitch: add TTL decrement action

2020-01-15 Thread Pravin Shelar
On Wed, Jan 15, 2020 at 8:40 AM Matteo Croce  wrote:
>
> New action to decrement TTL instead of setting it to a fixed value.
> This action will decrement the TTL and, in case of expired TTL, drop it
> or execute an action passed via a nested attribute.
> The default TTL expired action is to drop the packet.
>
> Supports both IPv4 and IPv6 via the ttl and hop_limit fields, respectively.
>
> Tested with a corresponding change in the userspace:
>
> # ovs-dpctl dump-flows
> in_port(2),eth(),eth_type(0x0800), packets:0, bytes:0, used:never, 
> actions:dec_ttl{ttl<=1 action:(drop)},1
> in_port(1),eth(),eth_type(0x0800), packets:0, bytes:0, used:never, 
> actions:dec_ttl{ttl<=1 action:(drop)},2
> in_port(1),eth(),eth_type(0x0806), packets:0, bytes:0, used:never, 
> actions:2
> in_port(2),eth(),eth_type(0x0806), packets:0, bytes:0, used:never, 
> actions:1
>
> # ping -c1 192.168.0.2 -t 42
> IP (tos 0x0, ttl 41, id 61647, offset 0, flags [DF], proto ICMP (1), 
> length 84)
> 192.168.0.1 > 192.168.0.2: ICMP echo request, id 386, seq 1, length 64
> # ping -c1 192.168.0.2 -t 120
> IP (tos 0x0, ttl 119, id 62070, offset 0, flags [DF], proto ICMP (1), 
> length 84)
> 192.168.0.1 > 192.168.0.2: ICMP echo request, id 388, seq 1, length 64
> # ping -c1 192.168.0.2 -t 1
> #
>
Thanks for the patch.

> Co-developed-by: Bindiya Kurle 
> Signed-off-by: Bindiya Kurle 
> Signed-off-by: Matteo Croce 
> ---
>  include/uapi/linux/openvswitch.h |  2 +
>  net/openvswitch/actions.c| 67 ++
>  net/openvswitch/flow_netlink.c   | 71 
>  3 files changed, 140 insertions(+)
>
> diff --git a/include/uapi/linux/openvswitch.h 
> b/include/uapi/linux/openvswitch.h
> index ae2bff14e7e1..9d3f040847af 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -958,6 +958,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_CLONE,/* Nested OVS_CLONE_ATTR_*.  */
> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> +   OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
>
> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>* from userspace. */
> @@ -1050,4 +1051,5 @@ struct ovs_zone_limit {
> __u32 count;
>  };
>
> +#define OVS_DEC_TTL_ATTR_EXEC  0

I am not sure if we need this, But if you want the nested attribute
then lets define enum with this single attribute and have actions as
part of its data. This would be optional argument, so userspace can
skip it, and in that case datapath can drop the packet.

>  #endif /* _LINUX_OPENVSWITCH_H */
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 7fbfe2adfffa..1b0afc9bf1ad 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -964,6 +964,26 @@ static int output_userspace(struct datapath *dp, struct 
> sk_buff *skb,
> return ovs_dp_upcall(dp, skb, key, , cutlen);
>  }
>
> +static int dec_ttl(struct datapath *dp, struct sk_buff *skb,
> +  struct sw_flow_key *key,
> +  const struct nlattr *attr, bool last)
Can you give it better name, for example: dec_ttl_exception_handler().

> +{
> +   /* The first action is always 'OVS_DEC_TTL_ATTR_ARG'. */
> +   struct nlattr *dec_ttl_arg = nla_data(attr);
> +   u32 nested = nla_get_u32(dec_ttl_arg);
> +   int rem = nla_len(attr);
> +
> +   if (nested) {
> +   struct nlattr *actions = nla_next(dec_ttl_arg, );
> +
> +   if (actions)
> +   return clone_execute(dp, skb, key, 0, actions, rem,
> +last, false);
> +   }
> +   consume_skb(skb);
> +   return 0;
> +}
> +
>  /* When 'last' is true, sample() should always consume the 'skb'.
>   * Otherwise, sample() should keep 'skb' intact regardless what
>   * actions are executed within sample().
> @@ -1180,6 +1200,45 @@ static int execute_check_pkt_len(struct datapath *dp, 
> struct sk_buff *skb,
>  nla_len(actions), last, clone_flow_key);
>  }
>
> +static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> +   int err;
> +
> +   if (skb->protocol == htons(ETH_P_IPV6)) {
> +   struct ipv6hdr *nh;
> +
> +   err = skb_ensure_writable(skb, skb_network_offset(skb) +
> + sizeof(*nh));
> +   if (unlikely(err))
> +   return err;
> +
> +   nh = ipv6_hdr(skb);
> +
> +   if (nh->hop_limit <= 1)
> +   return -EHOSTUNREACH;
> +
> +   key->ip.ttl = --nh->hop_limit;
> +   } else {
> +   struct iphdr *nh;
> +   u8 old_ttl;
> +
> +