| Message ID | 20190221184206.5512-1-nusiddiq@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,RFC,v4,1/1] datapath: Add a new action check_pkt_len | expand |
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... - Greg On 2/21/2019 10:42 AM, nusiddiq@redhat.com wrote: > From: Numan Siddique <nusiddiq@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 <blp@ovn.org> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > CC: Ben Pfaff <blp@ovn.org> > CC: Greg Rose <gvrose8192@gmail.com> > CC: Lorenzo Bianconi <lorenzo.bianconi@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;
On Sat, Feb 23, 2019 at 2:35 AM Gregory Rose <gvrose8192@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, nusiddiq@redhat.com wrote: > > From: Numan Siddique <nusiddiq@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 <blp@ovn.org> > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> > > CC: Ben Pfaff <blp@ovn.org> > > CC: Greg Rose <gvrose8192@gmail.com> > > CC: Lorenzo Bianconi <lorenzo.bianconi@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; > >
On Mon, Feb 25, 2019 at 12:09 PM Numan Siddique <nusiddiq@redhat.com> wrote: > > > On Sat, Feb 23, 2019 at 2:35 AM Gregory Rose <gvrose8192@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. > > 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, nusiddiq@redhat.com wrote: >> > From: Numan Siddique <nusiddiq@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 <blp@ovn.org> >> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> >> > CC: Ben Pfaff <blp@ovn.org> >> > CC: Greg Rose <gvrose8192@gmail.com> >> > CC: Lorenzo Bianconi <lorenzo.bianconi@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; >> >>
On 3/21/2019 8:23 AM, Numan Siddique wrote: > > > On Mon, Feb 25, 2019 at 12:09 PM Numan Siddique <nusiddiq@redhat.com > <mailto:nusiddiq@redhat.com>> wrote: > > > > On Sat, Feb 23, 2019 at 2:35 AM Gregory Rose <gvrose8192@gmail.com > <mailto:gvrose8192@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. > > > > 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. - Greg > > Thanks > Numan > > > - Greg > > On 2/21/2019 10:42 AM, nusiddiq@redhat.com > <mailto:nusiddiq@redhat.com> wrote: > > From: Numan Siddique <nusiddiq@redhat.com > <mailto:nusiddiq@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 <blp@ovn.org <mailto:blp@ovn.org>> > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com > <mailto:nusiddiq@redhat.com>> > > CC: Ben Pfaff <blp@ovn.org <mailto:blp@ovn.org>> > > CC: Greg Rose <gvrose8192@gmail.com > <mailto:gvrose8192@gmail.com>> > > CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com > <mailto:lorenzo.bianconi@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; >
On Thu, Mar 21, 2019 at 11:01 PM Gregory Rose <gvrose8192@gmail.com> wrote: > > On 3/21/2019 8:23 AM, Numan Siddique wrote: > > > > On Mon, Feb 25, 2019 at 12:09 PM Numan Siddique <nusiddiq@redhat.com> > wrote: > >> >> >> On Sat, Feb 23, 2019 at 2:35 AM Gregory Rose <gvrose8192@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. >> >> > > 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, nusiddiq@redhat.com wrote: >>> > From: Numan Siddique <nusiddiq@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 <blp@ovn.org> >>> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> >>> > CC: Ben Pfaff <blp@ovn.org> >>> > CC: Greg Rose <gvrose8192@gmail.com> >>> > CC: Lorenzo Bianconi <lorenzo.bianconi@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; >>> >>> >
On 3/21/2019 10:37 AM, Numan Siddique wrote: > 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 > > This patch: https://patchwork.ozlabs.org/patch/1059081/ shows this when applied: Applying: Add a new OVS action check_pkt_larger .git/rebase-apply/patch:1097: new blank line at EOF. + warning: 1 line adds whitespace errors. In regards to the datapath patch 1046370 <https://patchwork.ozlabs.org/patch/1046370/> In execute_check_pkt_len(): + + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0); + This doesn't seem right to me - the skb length should include the length of the entire packet, including any VLAN tags, or at least that is my understanding. Please check it. In validate_and_copy_check_pkt_len() in flow_netlink.c: + 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 }, + }; I don't care for declaring these things within function scope and it is not generally done. I see that flow_netlink.c has one other instance of the nla_policy structure statically declared within the function scope but if you look at datapath.c none of them are. I prefer the way it's done in datapath.c. I also grepped around in other kernel code in the ./net tree and that is also the way it's done there, i.e. I didn't see any other instances of it declared within function scope. I compiled both the ovs-vswitchd and openvswitch kernel module components with no issues. I wanted to use clang but the version of clang on Ubuntu right now doesn't have retpoline support so it won't compile kernel modules. :-/ I did some quick regression testing and found no problems. If you can address the two coding issues I brought up then I'd be glad to add my reviewed and tested by tags. Thanks! - Greg
On 3/21/2019 5:38 PM, Gregory Rose wrote: > > > On 3/21/2019 10:37 AM, Numan Siddique wrote: >> 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 >> >> > > This patch: > > https://patchwork.ozlabs.org/patch/1059081/ > > shows this when applied: > > Applying: Add a new OVS action check_pkt_larger > .git/rebase-apply/patch:1097: new blank line at EOF. > + > warning: 1 line adds whitespace errors. > > In regards to the datapath patch 1046370 > <https://patchwork.ozlabs.org/patch/1046370/> > > In execute_check_pkt_len(): > > + > + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? > VLAN_HLEN : 0); > + > > This doesn't seem right to me - the skb length should include the > length of the entire packet, including any > VLAN tags, or at least that is my understanding. Please check it. > > In validate_and_copy_check_pkt_len() in flow_netlink.c: > > + 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 }, > + }; > > I don't care for declaring these things within function scope and it > is not generally done. I see that > flow_netlink.c has one other instance of the nla_policy structure > statically declared within the function scope > but if you look at datapath.c none of them are. I prefer the way it's > done in datapath.c. I also grepped around > in other kernel code in the ./net tree and that is also the way it's > done there, i.e. I didn't see any other > instances of it declared within function scope. > > I compiled both the ovs-vswitchd and openvswitch kernel module > components with no issues. I wanted to use > clang but the version of clang on Ubuntu right now doesn't have > retpoline support so it won't compile > kernel modules. > > :-/ > > I did some quick regression testing and found no problems. If you can > address the two coding issues I brought up > then I'd be glad to add my reviewed and tested by tags. Oh wait, this is just an RFC. I'll review and test the patches again when they officially come out. Maybe clang will have retpoline support by then. - Greg
On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose <gvrose8192@gmail.com> wrote: > > > On 3/21/2019 5:38 PM, Gregory Rose wrote: > > > > > > On 3/21/2019 10:37 AM, Numan Siddique wrote: > >> 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 > >> > >> > > > > This patch: > > > > https://patchwork.ozlabs.org/patch/1059081/ > > > > shows this when applied: > > > > Applying: Add a new OVS action check_pkt_larger > > .git/rebase-apply/patch:1097: new blank line at EOF. > > + > > warning: 1 line adds whitespace errors. > > > > In regards to the datapath patch 1046370 > > <https://patchwork.ozlabs.org/patch/1046370/> > > > > In execute_check_pkt_len(): > > > > + > > + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? > > VLAN_HLEN : 0); > > + > > > > This doesn't seem right to me - the skb length should include the > > length of the entire packet, including any > > VLAN tags, or at least that is my understanding. Please check it. > > > > In validate_and_copy_check_pkt_len() in flow_netlink.c: > > > > + 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 }, > > + }; > > > > I don't care for declaring these things within function scope and it > > is not generally done. I see that > > flow_netlink.c has one other instance of the nla_policy structure > > statically declared within the function scope > > but if you look at datapath.c none of them are. I prefer the way it's > > done in datapath.c. I also grepped around > > in other kernel code in the ./net tree and that is also the way it's > > done there, i.e. I didn't see any other > > instances of it declared within function scope. > > > > I compiled both the ovs-vswitchd and openvswitch kernel module > > components with no issues. I wanted to use > > clang but the version of clang on Ubuntu right now doesn't have > > retpoline support so it won't compile > > kernel modules. > > > > :-/ > > > > I did some quick regression testing and found no problems. If you can > > address the two coding issues I brought up > > then I'd be glad to add my reviewed and tested by tags. > > Oh wait, this is just an RFC. > > I'll review and test the patches again when they officially come out. > Maybe clang will have retpoline support > by then. > > Thank you for the review. I will address them. I am planning to submit the patch to net-next ML. Looks like the window is open now - http://vger.kernel.org/~davem/net-next.html Please let me know in case you prefer to submit another RFC version here before submitting to net-dev ML. Thanks Numan - Greg > >
On 3/22/2019 2:58 AM, Numan Siddique wrote: > > > On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose <gvrose8192@gmail.com > <mailto:gvrose8192@gmail.com>> wrote: > > > > On 3/21/2019 5:38 PM, Gregory Rose wrote: > > > > > > On 3/21/2019 10:37 AM, Numan Siddique wrote: > >> 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 > >> > >> > > > > This patch: > > > > https://patchwork.ozlabs.org/patch/1059081/ > > > > shows this when applied: > > > > Applying: Add a new OVS action check_pkt_larger > > .git/rebase-apply/patch:1097: new blank line at EOF. > > + > > warning: 1 line adds whitespace errors. > > > > In regards to the datapath patch 1046370 > > <https://patchwork.ozlabs.org/patch/1046370/> > > > > In execute_check_pkt_len(): > > > > + > > + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? > > VLAN_HLEN : 0); > > + > > > > This doesn't seem right to me - the skb length should include the > > length of the entire packet, including any > > VLAN tags, or at least that is my understanding. Please check it. > > > > In validate_and_copy_check_pkt_len() in flow_netlink.c: > > > > + 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 }, > > + }; > > > > I don't care for declaring these things within function scope > and it > > is not generally done. I see that > > flow_netlink.c has one other instance of the nla_policy structure > > statically declared within the function scope > > but if you look at datapath.c none of them are. I prefer the > way it's > > done in datapath.c. I also grepped around > > in other kernel code in the ./net tree and that is also the way > it's > > done there, i.e. I didn't see any other > > instances of it declared within function scope. > > > > I compiled both the ovs-vswitchd and openvswitch kernel module > > components with no issues. I wanted to use > > clang but the version of clang on Ubuntu right now doesn't have > > retpoline support so it won't compile > > kernel modules. > > > > :-/ > > > > I did some quick regression testing and found no problems. If > you can > > address the two coding issues I brought up > > then I'd be glad to add my reviewed and tested by tags. > > Oh wait, this is just an RFC. > > I'll review and test the patches again when they officially come out. > Maybe clang will have retpoline support > by then. > > > Thank you for the review. I will address them. I am planning to submit > the patch > to net-next ML. Looks like the window is open now - > http://vger.kernel.org/~davem/net-next.html > > Please let me know in case you prefer to submit another RFC version > here before submitting to net-dev ML. I think you're good to go. Thanks, - Greg > > Thanks > Numan > > - Greg >
On Fri, Mar 22, 2019 at 9:10 PM Gregory Rose <gvrose8192@gmail.com> wrote: > > On 3/22/2019 2:58 AM, Numan Siddique wrote: > > > > On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose <gvrose8192@gmail.com> wrote: > >> >> >> On 3/21/2019 5:38 PM, Gregory Rose wrote: >> > >> > >> > On 3/21/2019 10:37 AM, Numan Siddique wrote: >> >> 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 >> >> >> >> >> > >> > This patch: >> > >> > https://patchwork.ozlabs.org/patch/1059081/ >> > >> > shows this when applied: >> > >> > Applying: Add a new OVS action check_pkt_larger >> > .git/rebase-apply/patch:1097: new blank line at EOF. >> > + >> > warning: 1 line adds whitespace errors. >> > >> > In regards to the datapath patch 1046370 >> > <https://patchwork.ozlabs.org/patch/1046370/> >> > >> > In execute_check_pkt_len(): >> > >> > + >> > + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? >> > VLAN_HLEN : 0); >> > + >> > >> > This doesn't seem right to me - the skb length should include the >> > length of the entire packet, including any >> > VLAN tags, or at least that is my understanding. Please check it. >> > Hi Greg, I checked and tested it it. I can confirm that skb->len doesn't include the vlan header. Existing code in flow.c also uses the same way to get the packet len - https://github.com/openvswitch/ovs/blob/master/datapath/flow.c#L77 Thanks Numan > >> > In validate_and_copy_check_pkt_len() in flow_netlink.c: >> > >> > + 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 }, >> > + }; >> > >> > I don't care for declaring these things within function scope and it >> > is not generally done. I see that >> > flow_netlink.c has one other instance of the nla_policy structure >> > statically declared within the function scope >> > but if you look at datapath.c none of them are. I prefer the way it's >> > done in datapath.c. I also grepped around >> > in other kernel code in the ./net tree and that is also the way it's >> > done there, i.e. I didn't see any other >> > instances of it declared within function scope. >> > >> > I compiled both the ovs-vswitchd and openvswitch kernel module >> > components with no issues. I wanted to use >> > clang but the version of clang on Ubuntu right now doesn't have >> > retpoline support so it won't compile >> > kernel modules. >> > >> > :-/ >> > >> > I did some quick regression testing and found no problems. If you can >> > address the two coding issues I brought up >> > then I'd be glad to add my reviewed and tested by tags. >> >> Oh wait, this is just an RFC. >> >> I'll review and test the patches again when they officially come out. >> Maybe clang will have retpoline support >> by then. >> >> > Thank you for the review. I will address them. I am planning to submit the > patch > to net-next ML. Looks like the window is open now - > http://vger.kernel.org/~davem/net-next.html > > Please let me know in case you prefer to submit another RFC version here > before submitting to net-dev ML. > > > I think you're good to go. > > Thanks, > > - Greg > > > Thanks > Numan > > - Greg >> >> >
On Mon, Mar 25, 2019 at 12:07 AM Numan Siddique <nusiddiq@redhat.com> wrote: > > > On Fri, Mar 22, 2019 at 9:10 PM Gregory Rose <gvrose8192@gmail.com> wrote: > >> >> On 3/22/2019 2:58 AM, Numan Siddique wrote: >> >> >> >> On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose <gvrose8192@gmail.com> >> wrote: >> >>> >>> >>> On 3/21/2019 5:38 PM, Gregory Rose wrote: >>> > >>> > >>> > On 3/21/2019 10:37 AM, Numan Siddique wrote: >>> >> 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 >>> >> >>> >> >>> > >>> > This patch: >>> > >>> > https://patchwork.ozlabs.org/patch/1059081/ >>> > >>> > shows this when applied: >>> > >>> > Applying: Add a new OVS action check_pkt_larger >>> > .git/rebase-apply/patch:1097: new blank line at EOF. >>> > + >>> > warning: 1 line adds whitespace errors. >>> > >>> > In regards to the datapath patch 1046370 >>> > <https://patchwork.ozlabs.org/patch/1046370/> >>> > >>> > In execute_check_pkt_len(): >>> > >>> > + >>> > + actual_pkt_len = skb->len + (skb_vlan_tag_present(skb) ? >>> > VLAN_HLEN : 0); >>> > + >>> > >>> > This doesn't seem right to me - the skb length should include the >>> > length of the entire packet, including any >>> > VLAN tags, or at least that is my understanding. Please check it. >>> >> > Hi Greg, > > I checked and tested it it. I can confirm that skb->len doesn't include > the vlan header. > Existing code in flow.c also uses the same way to get the packet len - > https://github.com/openvswitch/ovs/blob/master/datapath/flow.c#L77 > > I submitted the patch to net-dev ML and I forgot to CC - ovs-dev - https://patchwork.ozlabs.org/patch/1063378/ Thanks Numan > Thanks > Numan > > > >>> > In validate_and_copy_check_pkt_len() in flow_netlink.c: >>> > >>> > + 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 }, >>> > + }; >>> > >>> > I don't care for declaring these things within function scope and it >>> > is not generally done. I see that >>> > flow_netlink.c has one other instance of the nla_policy structure >>> > statically declared within the function scope >>> > but if you look at datapath.c none of them are. I prefer the way it's >>> > done in datapath.c. I also grepped around >>> > in other kernel code in the ./net tree and that is also the way it's >>> > done there, i.e. I didn't see any other >>> > instances of it declared within function scope. >>> > >>> > I compiled both the ovs-vswitchd and openvswitch kernel module >>> > components with no issues. I wanted to use >>> > clang but the version of clang on Ubuntu right now doesn't have >>> > retpoline support so it won't compile >>> > kernel modules. >>> > >>> > :-/ >>> > >>> > I did some quick regression testing and found no problems. If you can >>> > address the two coding issues I brought up >>> > then I'd be glad to add my reviewed and tested by tags. >>> >>> Oh wait, this is just an RFC. >>> >>> I'll review and test the patches again when they officially come out. >>> Maybe clang will have retpoline support >>> by then. >>> >>> >> Thank you for the review. I will address them. I am planning to submit >> the patch >> to net-next ML. Looks like the window is open now - >> http://vger.kernel.org/~davem/net-next.html >> >> Please let me know in case you prefer to submit another RFC version here >> before submitting to net-dev ML. >> >> >> I think you're good to go. >> >> Thanks, >> >> - Greg >> >> >> Thanks >> Numan >> >> - Greg >>> >>> >>
On 3/24/2019 11:37 AM, Numan Siddique wrote: > > > On Fri, Mar 22, 2019 at 9:10 PM Gregory Rose <gvrose8192@gmail.com > <mailto:gvrose8192@gmail.com>> wrote: > > > On 3/22/2019 2:58 AM, Numan Siddique wrote: >> >> >> On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose >> <gvrose8192@gmail.com <mailto:gvrose8192@gmail.com>> wrote: >> >> >> >> On 3/21/2019 5:38 PM, Gregory Rose wrote: >> > >> > >> > On 3/21/2019 10:37 AM, Numan Siddique wrote: >> >> 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 >> >> >> >> >> > >> > This patch: >> > >> > https://patchwork.ozlabs.org/patch/1059081/ >> > >> > shows this when applied: >> > >> > Applying: Add a new OVS action check_pkt_larger >> > .git/rebase-apply/patch:1097: new blank line at EOF. >> > + >> > warning: 1 line adds whitespace errors. >> > >> > In regards to the datapath patch 1046370 >> > <https://patchwork.ozlabs.org/patch/1046370/> >> > >> > In execute_check_pkt_len(): >> > >> > + >> > + actual_pkt_len = skb->len + >> (skb_vlan_tag_present(skb) ? >> > VLAN_HLEN : 0); >> > + >> > >> > This doesn't seem right to me - the skb length should >> include the >> > length of the entire packet, including any >> > VLAN tags, or at least that is my understanding. Please >> check it. >> > > Hi Greg, > > I checked and tested it it. I can confirm that skb->len doesn't > include the vlan header. > Existing code in flow.c also uses the same way to get the packet len - > https://github.com/openvswitch/ovs/blob/master/datapath/flow.c#L77 > > Thanks > Numan Thanks for clearing that up for me Numan. - Greg > >> > >> > In validate_and_copy_check_pkt_len() in flow_netlink.c: >> > >> > + 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 }, >> > + }; >> > >> > I don't care for declaring these things within function >> scope and it >> > is not generally done. I see that >> > flow_netlink.c has one other instance of the nla_policy >> structure >> > statically declared within the function scope >> > but if you look at datapath.c none of them are. I prefer >> the way it's >> > done in datapath.c. I also grepped around >> > in other kernel code in the ./net tree and that is also the >> way it's >> > done there, i.e. I didn't see any other >> > instances of it declared within function scope. >> > >> > I compiled both the ovs-vswitchd and openvswitch kernel module >> > components with no issues. I wanted to use >> > clang but the version of clang on Ubuntu right now doesn't >> have >> > retpoline support so it won't compile >> > kernel modules. >> > >> > :-/ >> > >> > I did some quick regression testing and found no problems. >> If you can >> > address the two coding issues I brought up >> > then I'd be glad to add my reviewed and tested by tags. >> >> Oh wait, this is just an RFC. >> >> I'll review and test the patches again when they officially >> come out. >> Maybe clang will have retpoline support >> by then. >> >> >> Thank you for the review. I will address them. I am planning to >> submit the patch >> to net-next ML. Looks like the window is open now - >> http://vger.kernel.org/~davem/net-next.html >> >> Please let me know in case you prefer to submit another RFC >> version here before submitting to net-dev ML. > > I think you're good to go. > > Thanks, > > - Greg > >> >> Thanks >> Numan >> >> - Greg >> >
On 3/25/2019 5:30 AM, Numan Siddique wrote: > > > On Mon, Mar 25, 2019 at 12:07 AM Numan Siddique <nusiddiq@redhat.com > <mailto:nusiddiq@redhat.com>> wrote: > > > > On Fri, Mar 22, 2019 at 9:10 PM Gregory Rose <gvrose8192@gmail.com > <mailto:gvrose8192@gmail.com>> wrote: > > > On 3/22/2019 2:58 AM, Numan Siddique wrote: >> >> >> On Fri, Mar 22, 2019 at 6:16 AM Gregory Rose >> <gvrose8192@gmail.com <mailto:gvrose8192@gmail.com>> wrote: >> >> >> >> On 3/21/2019 5:38 PM, Gregory Rose wrote: >> > >> > >> > On 3/21/2019 10:37 AM, Numan Siddique wrote: >> >> 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 >> >> >> >> >> > >> > This patch: >> > >> > https://patchwork.ozlabs.org/patch/1059081/ >> > >> > shows this when applied: >> > >> > Applying: Add a new OVS action check_pkt_larger >> > .git/rebase-apply/patch:1097: new blank line at EOF. >> > + >> > warning: 1 line adds whitespace errors. >> > >> > In regards to the datapath patch 1046370 >> > <https://patchwork.ozlabs.org/patch/1046370/> >> > >> > In execute_check_pkt_len(): >> > >> > + >> > + actual_pkt_len = skb->len + >> (skb_vlan_tag_present(skb) ? >> > VLAN_HLEN : 0); >> > + >> > >> > This doesn't seem right to me - the skb length should >> include the >> > length of the entire packet, including any >> > VLAN tags, or at least that is my understanding. >> Please check it. >> > > Hi Greg, > > I checked and tested it it. I can confirm that skb->len doesn't > include the vlan header. > Existing code in flow.c also uses the same way to get the packet > len - > https://github.com/openvswitch/ovs/blob/master/datapath/flow.c#L77 > > > I submitted the patch to net-dev ML and I forgot to CC - ovs-dev - > https://patchwork.ozlabs.org/patch/1063378/ Yes, I'm subscribed to netdev as well and already saw it. :) Thanks! - Greg > > Thanks > Numan > > Thanks > Numan > >> > >> > In validate_and_copy_check_pkt_len() in flow_netlink.c: >> > >> > + 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 }, >> > + }; >> > >> > I don't care for declaring these things within function >> scope and it >> > is not generally done. I see that >> > flow_netlink.c has one other instance of the nla_policy >> structure >> > statically declared within the function scope >> > but if you look at datapath.c none of them are. I >> prefer the way it's >> > done in datapath.c. I also grepped around >> > in other kernel code in the ./net tree and that is also >> the way it's >> > done there, i.e. I didn't see any other >> > instances of it declared within function scope. >> > >> > I compiled both the ovs-vswitchd and openvswitch kernel >> module >> > components with no issues. I wanted to use >> > clang but the version of clang on Ubuntu right now >> doesn't have >> > retpoline support so it won't compile >> > kernel modules. >> > >> > :-/ >> > >> > I did some quick regression testing and found no >> problems. If you can >> > address the two coding issues I brought up >> > then I'd be glad to add my reviewed and tested by tags. >> >> Oh wait, this is just an RFC. >> >> I'll review and test the patches again when they >> officially come out. >> Maybe clang will have retpoline support >> by then. >> >> >> Thank you for the review. I will address them. I am planning >> to submit the patch >> to net-next ML. Looks like the window is open now - >> http://vger.kernel.org/~davem/net-next.html >> >> Please let me know in case you prefer to submit another RFC >> version here before submitting to net-dev ML. > > I think you're good to go. > > Thanks, > > - Greg > >> >> Thanks >> Numan >> >> - Greg >> >
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;