On Sun, Mar 24, 2019 at 11:47 AM <nusid...@redhat.com> wrote:
>
> From: Numan Siddique <nusid...@redhat.com>
>
> This patch adds a new action - 'check_pkt_len' which checks the
> packet length and executes a set of actions if the packet
> length is greater than the specified length or executes
> another set of actions if the packet length is lesser or equal to.
>
> This action takes below nlattrs
> * OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
>
> * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER - Nested actions
> to apply if the packet length is greater than the specified 'pkt_len'
>
> * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested
> actions to apply if the packet length is lesser or equal to the
> specified 'pkt_len'.
>
> The main use case for adding this action is to solve the packet
> drops because of MTU mismatch in OVN virtual networking solution.
> When a VM (which belongs to a logical switch of OVN) sends a packet
> destined to go via the gateway router and if the nic which provides
> external connectivity, has a lesser MTU, OVS drops the packet
> if the packet length is greater than this MTU.
>
> With the help of this action, OVN will check the packet length
> and if it is greater than the MTU size, it will generate an
> ICMP packet (type 3, code 4) and includes the next hop mtu in it
> so that the sender can fragment the packets.
>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
> Suggested-by: Ben Pfaff <b...@ovn.org>
> Signed-off-by: Numan Siddique <nusid...@redhat.com>
> CC: Gregory Rose <gvrose8...@gmail.com>
> CC: Pravin B Shelar <pshe...@ovn.org>
> ---
This looks pretty good to me. I have couple of comments inlined.
> include/uapi/linux/openvswitch.h | 42 ++++++++
> net/openvswitch/actions.c | 49 +++++++++
> net/openvswitch/flow_netlink.c | 171 +++++++++++++++++++++++++++++++
> 3 files changed, 262 insertions(+)
>
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index dbe0cbe4f1b7..05ab885c718d 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -798,6 +798,44 @@ struct ovs_action_push_eth {
> struct ovs_key_ethernet addresses;
> };
>
> +/*
> + * enum ovs_check_pkt_len_attr - Attributes for
> %OVS_ACTION_ATTR_CHECK_PKT_LEN.
> + *
> + * @OVS_CHECK_PKT_LEN_ATTR_PKT_LEN: u16 Packet length to check for.
> + * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER: Nested OVS_ACTION_ATTR_*
> + * actions to apply if the packer length is greater than the specified
> + * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
> + * @OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL - Nested OVS_ACTION_ATTR_*
> + * actions to apply if the packer length is lesser or equal to the specified
> + * length in the attr - OVS_CHECK_PKT_LEN_ATTR_PKT_LEN.
> + */
> +enum ovs_check_pkt_len_attr {
> + OVS_CHECK_PKT_LEN_ATTR_UNSPEC,
> + OVS_CHECK_PKT_LEN_ATTR_PKT_LEN,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL,
> + __OVS_CHECK_PKT_LEN_ATTR_MAX,
> +
> +#ifdef __KERNEL__
> + OVS_CHECK_PKT_LEN_ATTR_ARG /* struct check_pkt_len_arg */
> +#endif
> +};
> +
> +#define OVS_CHECK_PKT_LEN_ATTR_MAX (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)
> +
> +#ifdef __KERNEL__
> +struct check_pkt_len_arg {
> + u16 pkt_len; /* Same value as OVS_CHECK_PKT_LEN_ATTR_PKT_LEN'. */
> + bool exec_for_greater; /* When true, actions in IF_GREATE will
> + * not change flow keys. False otherwise.
> + */
> + bool exec_for_lesser_equal; /* When true, actions in IF_LESS_EQUAL
> + * will not change flow keys. False
> + * otherwise.
> + */
> +};
> +#endif
> +
> /**
> * enum ovs_action_attr - Action types.
> *
> @@ -842,6 +880,9 @@ struct ovs_action_push_eth {
> * packet, or modify the packet (e.g., change the DSCP field).
> * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
> * actions without affecting the original packet and key.
> + * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the packet length and execute a set
> + * of actions if greater than the specified packet length, else execute
> + * another set of actions.
> *
> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not
> all
> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -876,6 +917,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_POP_NSH, /* No argument. */
> OVS_ACTION_ATTR_METER, /* u32 meter ID. */
> OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */
> + OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>
> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
> * from userspace. */
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index e47ebbbe71b8..bc7b79b29469 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -169,6 +169,10 @@ static int clone_execute(struct datapath *dp, struct
> sk_buff *skb,
> const struct nlattr *actions, int len,
> bool last, bool clone_flow_key);
>
> +static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> + struct sw_flow_key *key,
> + const struct nlattr *attr, int len);
> +
> static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
> __be16 ethertype)
> {
> @@ -1213,6 +1217,41 @@ static int execute_recirc(struct datapath *dp, struct
> sk_buff *skb,
> return clone_execute(dp, skb, key, recirc_id, NULL, 0, last, true);
> }
>
> +static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb,
> + struct sw_flow_key *key,
> + const struct nlattr *attr, bool last)
> +{
> + const struct nlattr *actions, *cpl_arg;
> + const struct check_pkt_len_arg *arg;
> + int rem = nla_len(attr);
> + bool clone_flow_key;
> + u16 actual_pkt_len;
> +
> + /* The first action is always 'OVS_CHECK_PKT_LEN_ATTR_ARG'. */
This comment is bit confusing. It is first "netlink attribute" rather
than "action". Can you change similar comments?
> + cpl_arg = nla_data(attr);
> + arg = nla_data(cpl_arg);
> +
> + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN :
> 0);
> +
I do not see need to add vlan-tag length here. This limit can be
programmed in vlan flow itself. So that we can avoid this calculation
in fast path.
> + if (actual_pkt_len > arg->pkt_len) {
> + /* Second action is always
> + * 'OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER'.
> + */
> + actions = nla_next(cpl_arg, &rem);
> + clone_flow_key = !arg->exec_for_greater;
This is exception case, so we could rather keep
OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL first to optimize usual
scenario.
Can you change ordering of these actions?
> + } else {
> + /* Third action is always
> + * 'OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL'.
> + */
> + actions = nla_next(cpl_arg, &rem);
> + actions = nla_next(actions, &rem);
> + clone_flow_key = !arg->exec_for_lesser_equal;
> + }
> +
> + return clone_execute(dp, skb, key, 0, nla_data(actions),
> + nla_len(actions), last, clone_flow_key);
> +}
> +
> /* Execute a list of actions against 'skb'. */
> static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> struct sw_flow_key *key,
> @@ -1374,6 +1413,16 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
>
> break;
> }
> +
> + case OVS_ACTION_ATTR_CHECK_PKT_LEN: {
> + bool last = nla_is_last(a, rem);
> +
> + err = execute_check_pkt_len(dp, skb, key, a, last);
> + if (last)
> + return err;
> +
> + break;
> + }
> }
>
> if (unlikely(err)) {
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 691da853bef5..ee6eb707b4d9 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -91,6 +91,7 @@ static bool actions_may_change_flow(const struct nlattr
> *actions)
> case OVS_ACTION_ATTR_SET:
> case OVS_ACTION_ATTR_SET_MASKED:
> case OVS_ACTION_ATTR_METER:
> + case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> default:
> return true;
> }
> @@ -2838,6 +2839,87 @@ static int validate_userspace(const struct nlattr
> *attr)
> return 0;
> }
>
> +static const struct nla_policy cpl_policy[OVS_CHECK_PKT_LEN_ATTR_MAX + 1] = {
> + [OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] = {.type = NLA_U16 },
> + [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER] = {.type = NLA_NESTED },
> + [OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL] = {.type = NLA_NESTED
> },
> +};
> +
> +static int validate_and_copy_check_pkt_len(struct net *net,
> + const struct nlattr *attr,
> + const struct sw_flow_key *key,
> + struct sw_flow_actions **sfa,
> + __be16 eth_type, __be16 vlan_tci,
> + bool log, bool last)
> +{
> + const struct nlattr *acts_if_greater, *acts_if_lesser_eq;
> + struct nlattr *a[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
> + struct check_pkt_len_arg arg;
> + int nested_acts_start;
> + int start, err;
> +
> + err = nla_parse_strict(a, OVS_CHECK_PKT_LEN_ATTR_MAX, nla_data(attr),
> + nla_len(attr), cpl_policy, NULL);
> + if (err)
> + return err;
> +
> + if (!a[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] ||
> + !nla_get_u16(a[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN]))
> + return -EINVAL;
> +
> + acts_if_greater = a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
> + acts_if_lesser_eq = a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
> +
> + /* Both the nested action should be present. */
> + if (!acts_if_greater || !acts_if_lesser_eq)
> + return -EINVAL;
> +
> + /* validation done, copy the nested actions. */
> + start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CHECK_PKT_LEN,
> + log);
> + if (start < 0)
> + return start;
> +
> + arg.pkt_len = nla_get_u16(a[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN]);
> + arg.exec_for_greater =
> + last || !actions_may_change_flow(acts_if_greater);
> + arg.exec_for_lesser_equal =
> + last || !actions_may_change_flow(acts_if_lesser_eq);
> +
> + err = ovs_nla_add_action(sfa, OVS_CHECK_PKT_LEN_ATTR_ARG, &arg,
> + sizeof(arg), log);
> + if (err)
> + return err;
> +
> + nested_acts_start = add_nested_action_start(sfa,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER, log);
> + if (nested_acts_start < 0)
> + return nested_acts_start;
> +
> + err = __ovs_nla_copy_actions(net, acts_if_greater, key, sfa,
> + eth_type, vlan_tci, log);
> +
> + if (err)
> + return err;
> +
> + add_nested_action_end(*sfa, nested_acts_start);
> +
> + nested_acts_start = add_nested_action_start(sfa,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL, log);
> + if (nested_acts_start < 0)
> + return nested_acts_start;
> +
> + err = __ovs_nla_copy_actions(net, acts_if_lesser_eq, key, sfa,
> + eth_type, vlan_tci, log);
> +
> + if (err)
> + return err;
> +
> + add_nested_action_end(*sfa, nested_acts_start);
> + add_nested_action_end(*sfa, start);
> + return 0;
> +}
> +
> static int copy_action(const struct nlattr *from,
> struct sw_flow_actions **sfa, bool log)
> {
> @@ -2884,6 +2966,7 @@ static int __ovs_nla_copy_actions(struct net *net,
> const struct nlattr *attr,
> [OVS_ACTION_ATTR_POP_NSH] = 0,
> [OVS_ACTION_ATTR_METER] = sizeof(u32),
> [OVS_ACTION_ATTR_CLONE] = (u32)-1,
> + [OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
> };
> const struct ovs_action_push_vlan *vlan;
> int type = nla_type(a);
> @@ -3085,6 +3168,19 @@ static int __ovs_nla_copy_actions(struct net *net,
> const struct nlattr *attr,
> break;
> }
>
> + case OVS_ACTION_ATTR_CHECK_PKT_LEN: {
> + bool last = nla_is_last(a, rem);
> +
> + err = validate_and_copy_check_pkt_len(net, a, key,
> sfa,
> + eth_type,
> + vlan_tci, log,
> + last);
> + if (err)
> + return err;
> + skip_copy = true;
> + break;
> + }
> +
> default:
> OVS_NLERR(log, "Unknown Action type %d", type);
> return -EINVAL;
> @@ -3183,6 +3279,75 @@ static int clone_action_to_attr(const struct nlattr
> *attr,
> return err;
> }
>
> +static int check_pkt_len_action_to_attr(const struct nlattr *attr,
> + struct sk_buff *skb)
> +{
> + struct nlattr *start, *ac_start = NULL;
> + const struct check_pkt_len_arg *arg;
> + const struct nlattr *a, *cpl_arg;
> + int err = 0, rem = nla_len(attr);
> +
> + start = nla_nest_start(skb, OVS_ACTION_ATTR_CHECK_PKT_LEN);
> + if (!start)
> + return -EMSGSIZE;
> +
> + /* The first nested action in 'attr' is always
> + * 'OVS_CHECK_PKT_LEN_ATTR_ARG'.
> + */
> + cpl_arg = nla_data(attr);
> + arg = nla_data(cpl_arg);
> +
> + if (nla_put_u16(skb, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN, arg->pkt_len)) {
> + err = -EMSGSIZE;
> + goto out;
> + }
> +
> + /* Second action in 'attr' is always
> + * 'OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER'.
> + */
> + a = nla_next(cpl_arg, &rem);
> + ac_start = nla_nest_start(skb,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
> + if (!ac_start) {
> + err = -EMSGSIZE;
> + goto out;
> + }
> +
> + err = ovs_nla_put_actions(nla_data(a), nla_len(a), skb);
> + if (err) {
> + nla_nest_cancel(skb, ac_start);
> + goto out;
> + } else {
> + nla_nest_end(skb, ac_start);
> + }
> +
> + /* Third action in 'attr' is always
> + * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL.
> + */
> + a = nla_next(a, &rem);
> + ac_start = nla_nest_start(skb,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
> + if (!ac_start) {
> + err = -EMSGSIZE;
> + goto out;
> + }
> +
> + err = ovs_nla_put_actions(nla_data(a), nla_len(a), skb);
> + if (err) {
> + nla_nest_cancel(skb, ac_start);
> + goto out;
> + } else {
> + nla_nest_end(skb, ac_start);
> + }
> +
> + nla_nest_end(skb, start);
> + return 0;
> +
> +out:
> + nla_nest_cancel(skb, start);
> + return err;
> +}
> +
> static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
> {
> const struct nlattr *ovs_key = nla_data(a);
> @@ -3277,6 +3442,12 @@ int ovs_nla_put_actions(const struct nlattr *attr, int
> len, struct sk_buff *skb)
> return err;
> break;
>
> + case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> + err = check_pkt_len_action_to_attr(a, skb);
> + if (err)
> + return err;
> + break;
> +
> default:
> if (nla_put(skb, type, nla_len(a), nla_data(a)))
> return -EMSGSIZE;
> --
> 2.20.1
>
