On Thu, Mar 21, 2019 at 11:01 PM Gregory Rose <[email protected]> wrote:
> > On 3/21/2019 8:23 AM, Numan Siddique wrote: > > > > 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 > > > Gah! No, that's my bad. It fell off my radar. I'll look at the new > patch. > > Thanks for the reminder. > Thanks. This is the datapath patch - https://patchwork.ozlabs.org/patch/1046370/ and this is the corresponding ovs-vswitchd patch - https://patchwork.ozlabs.org/patch/1059081/ (this is part of the series - https://patchwork.ozlabs.org/project/openvswitch/list/?series=98190, but probably you would be interested in only ovs patch) Sharing the links so that you can find it easily. Thanks Numan > - Greg > > > 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
