> From: Numan Siddique <[email protected]>
>
> [Please note, this patch is submitted as RFC in ovs-dev ML to
> get feedback before submitting to netdev ML]
>
> This patch adds a new action 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 (optional) - Nested actions
> to apply if the packet length is greater than the specified 'pkt_len'
>
> * OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - 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 <[email protected]>
> Signed-off-by: Numan Siddique <[email protected]>
> CC: Ben Pfaff <[email protected]>
> CC: Justin Pettit <[email protected]>
> CC: Pravin B Shelar <[email protected]>
Hi Numan,
just fiew comments inline
Regards,
Lorenzo
> ---
> include/uapi/linux/openvswitch.h | 25 ++++-
> net/openvswitch/actions.c | 55 +++++++++-
> net/openvswitch/flow_netlink.c | 175 +++++++++++++++++++++++++++++++
> 3 files changed, 253 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index dbe0cbe4f1b7..c395baffdd42 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -798,6 +798,27 @@ 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,
This is not used actually
> + 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,
> +};
> +
> +#define OVS_CHECK_PKT_LEN_ATTR_MAX (__OVS_CHECK_PKT_LEN_ATTR_MAX - 1)
> +
> /**
> * enum ovs_action_attr - Action types.
> *
> @@ -842,7 +863,8 @@ 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 length of the packet and
> + * execute a set of actions as specified in OVS_CHECK_PKT_LEN_ATTR_*.
> * 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
> * type may not be changed.
> @@ -875,6 +897,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_PUSH_NSH, /* Nested OVS_NSH_KEY_ATTR_*. */
> OVS_ACTION_ATTR_POP_NSH, /* No argument. */
> OVS_ACTION_ATTR_METER, /* u32 meter ID. */
> + OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */
>
> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index e47ebbbe71b8..9551c07eae92 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,46 @@ 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 *a;
> + const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
Please use 'reverse christmas tree' whenever possible
> + u16 actual_pkt_len;
> + u16 pkt_len = 0;
> + int rem;
> +
> + memset(attrs, 0, sizeof(attrs));
> + nla_for_each_nested(a, attr, rem) {
> + int type = nla_type(a);
> +
> + if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
> + return 1;
maybe we can use better error code here
> + attrs[type] = a;
> + }
> + if (rem)
> + return 1;
same here
> +
> + if (!attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN])
> + return 1;
> +
> + a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
> + pkt_len = nla_get_u16(a);
> + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);
> +
> + if (actual_pkt_len > pkt_len)
> + a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
> + else
> + a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
> +
> + if (a)
> + return clone_execute(dp, skb, key, 0, nla_data(a), nla_len(a),
> + last, !last);
> +
Should we provide an error code if OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL
or OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER are not provided
> + return 0;
> +}
> +
> /* 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,8 +1418,17 @@ 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)) {
> kfree_skb(skb);
> return err;
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 435a4bdf8f89..93b8e91315da 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,93 @@ static int validate_userspace(const struct nlattr
> *attr)
> return 0;
> }
>
> +static int copy_action(const struct nlattr *from,
> + struct sw_flow_actions **sfa, bool log);
> +
> +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)
> +{
> + const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
> + const struct nlattr *a;
> + const struct nlattr *pkt_len, *acts_if_greater, *acts_if_lesser_eq;
> + int rem, start, err, nested_acts_start;
> +
> + memset(attrs, 0, sizeof(attrs));
> + nla_for_each_nested(a, attr, rem) {
> + int type = nla_type(a);
> +
> + if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
maybe attrs[type] should have a different error code here
> + return -EINVAL;
> + attrs[type] = a;
> + }
> + if (rem)
> + return -EINVAL;
> +
> + pkt_len = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
> + if (!pkt_len || nla_len(pkt_len) != sizeof(u16))
> + return -EINVAL;
> +
> + acts_if_greater = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
> + if (acts_if_greater && nla_len(acts_if_greater) &&
> + nla_len(acts_if_greater) < NLA_HDRLEN)
> + return -EINVAL;
> +
> + acts_if_lesser_eq = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
> + if (acts_if_lesser_eq && nla_len(acts_if_lesser_eq) &&
> + nla_len(acts_if_lesser_eq) < NLA_HDRLEN)
> + 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;
> +
> + err = copy_action(pkt_len, sfa, log);
> + if (err)
> + return err;
> +
> + if (acts_if_greater) {
> + 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);
> + }
> +
> + if (acts_if_lesser_eq) {
> + 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 +2972,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 +3174,15 @@ static int __ovs_nla_copy_actions(struct net *net,
> const struct nlattr *attr,
> break;
> }
>
> + case OVS_ACTION_ATTR_CHECK_PKT_LEN:
> + err = validate_and_copy_check_pkt_len(net, a, key, sfa,
> + eth_type,
> + vlan_tci, log);
> + if (err)
> + return err;
> + skip_copy = true;
> + break;
> +
> default:
> OVS_NLERR(log, "Unknown Action type %d", type);
> return -EINVAL;
> @@ -3183,6 +3281,77 @@ 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;
> + int err = -1, rem;
> + const struct nlattr *a;
> + const struct nlattr *attrs[OVS_CHECK_PKT_LEN_ATTR_MAX + 1];
reverse christmas tree
> +
> + memset(attrs, 0, sizeof(attrs));
> + nla_for_each_nested(a, attr, rem) {
> + int type = nla_type(a);
> +
> + if (!type || type > OVS_CHECK_PKT_LEN_ATTR_MAX || attrs[type])
> + return err;
> + attrs[type] = a;
> + }
> + if (rem)
> + return err;
> +
> + a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN];
> + if (!a)
> + return err;
> +
> + err = -EMSGSIZE;
> + start = nla_nest_start(skb, OVS_ACTION_ATTR_CHECK_PKT_LEN);
> + if (!start)
> + return err;
> +
> + if (nla_put_u16(skb, OVS_CHECK_PKT_LEN_ATTR_PKT_LEN, nla_get_u16(a)))
> + goto out;
> +
> + a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
> + if (a) {
> + ac_start = nla_nest_start(skb,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER);
> + if (!ac_start)
> + goto out;
> +
> + if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {
> + nla_nest_cancel(skb, ac_start);
> + goto out;
> + } else {
> + nla_nest_end(skb, ac_start);
> + }
> + }
> +
> + a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
> + if (a) {
> + ac_start = nla_nest_start(skb,
> + OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL);
> + if (!ac_start)
> + goto out;
> +
> + if (ovs_nla_put_actions(nla_data(a), nla_len(a), skb)) {
> + nla_nest_cancel(skb, ac_start);
> + goto out;
> + } else {
> + nla_nest_end(skb, ac_start);
> + }
> + }
> +
> + err = 0;
> +out:
maybe we can have 'out' just to take care of error code path
> + if (err)
> + nla_nest_cancel(skb, start);
> + else
> + nla_nest_end(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 +3446,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
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
