[ovs-dev] [RFC v4 1/1] datapath: Add a new action check_pkt_len
Numan Siddique
nusiddiq at redhat.com
Thu Mar 21 17:37:11 UTC 2019
On Thu, Mar 21, 2019 at 11:01 PM Gregory Rose <gvrose8192 at 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 at redhat.com>
> wrote:
>
>>
>>
>> On Sat, Feb 23, 2019 at 2:35 AM Gregory Rose <gvrose8192 at 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 at redhat.com wrote:
>>> > From: Numan Siddique <nusiddiq at 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 at ovn.org>
>>> > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
>>> > CC: Ben Pfaff <blp at ovn.org>
>>> > CC: Greg Rose <gvrose8192 at gmail.com>
>>> > CC: Lorenzo Bianconi <lorenzo.bianconi at 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;
>>>
>>>
>
More information about the dev
mailing list