On Mon, Feb 25, 2019 at 12:09 PM Numan Siddique <[email protected]> wrote:
> > > On Sat, Feb 23, 2019 at 2:35 AM Gregory Rose <[email protected]> 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. > > Hi Greg, Gentle ping to see if you got a chance to try this out. FYI - RFC v4 of the corresponding ovs patch is submitted here - https://patchwork.ozlabs.org/patch/1059081/ Thanks Numan Thanks > Numan > > > > - Greg >> >> On 2/21/2019 10:42 AM, [email protected] wrote: >> > 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. 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 <[email protected]> >> > Signed-off-by: Numan Siddique <[email protected]> >> > CC: Ben Pfaff <[email protected]> >> > CC: Greg Rose <[email protected]> >> > CC: Lorenzo Bianconi <[email protected]> >> > --- >> > >> > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
