On Sat, Feb 23, 2019 at 2:35 AM Gregory Rose <gvrose8...@gmail.com> wrote:
> Numan, > > I intend to test and review this on my local net-next branch but I'm not > feeling well so it will probably be > early next week before I can do that. Sorry for the delay... > > No worries. Please take your time. If you want to test it out, you probably need the corresponding ovs-vswitchd patch. I still need to address the review comments from Ben. But you can use the one from here - https://github.com/numansiddique/ovs/tree/ovn_mtu_fix/rfc_v4_p4 https://github.com/numansiddique/ovs/commits/ovn_mtu_fix/rfc_v4_p4 for your testing. Thanks Numan - Greg > > On 2/21/2019 10:42 AM, nusid...@redhat.com wrote: > > From: Numan Siddique <nusid...@redhat.com> > > > > [Please note, this patch is submitted as RFC in ovs-dev ML to > > get feedback before submitting to netdev ML. You need net-next tree > > to apply this patch] > > > > 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: Ben Pfaff <b...@ovn.org> > > CC: Greg Rose <gvrose8...@gmail.com> > > CC: Lorenzo Bianconi <lorenzo.bianc...@redhat.com> > > --- > > > > v3 -> v4 > > -------- > > * v4 only has 1 patch - datapath patch which implements the > > * check_pkt_len action > > * Addressed the review comments from Lorenzo, Ben and Greg > > > > > > 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'. */ > > + cpl_arg = nla_data(attr); > > + arg = nla_data(cpl_arg); > > + > > + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN > : 0); > > + > > + 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; > > + } 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..989b5092c526 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,88 @@ static int validate_userspace(const struct nlattr > *attr) > > return 0; > > } > > > > +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) > > +{ > > + static const struct nla_policy pol[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 }, > > + }; > > + 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), pol, 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 +2967,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 +3169,18 @@ 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; > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
