Re: [PATCH] openvswitch: Add packet truncation support.

[pravin shelar <pshelar@ovn.org>]

On Wed, Jun 8, 2016 at 6:18 PM, William Tu <u9012063@gmail.com> wrote:
> The patch adds a new OVS action, OVS_ACTION_ATTR_TRUNC, in order to
> truncate packets. A 'max_len' is added for setting up the maximum
> packet size, and a 'cutlen' field is to record the number of bytes
> to trim the packet when the packet is outputting to a port, or when
> the packet is sent to userspace.
>
> Signed-off-by: William Tu <u9012063@gmail.com>
> ---
>  include/uapi/linux/openvswitch.h |  7 +++++++
>  net/openvswitch/actions.c        | 28 +++++++++++++++++++++++++++-
>  net/openvswitch/datapath.c       |  9 +++++++--
>  net/openvswitch/datapath.h       |  2 ++
>  net/openvswitch/flow_netlink.c   |  9 +++++++++
>  net/openvswitch/vport.c          |  1 +
>  6 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index bb0d515..6d7ac5b 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -580,6 +580,11 @@ enum ovs_userspace_attr {
>
>  #define OVS_USERSPACE_ATTR_MAX (__OVS_USERSPACE_ATTR_MAX - 1)
>
> +struct ovs_action_trunc {
> +       uint32_t max_len; /* Max packet size in bytes. */
This could uint16_t. as it is related to packet len.

> +};
> +#define ETH_MIN_FRAME_LEN 60
> +

There is already symbol defined, ETH_ZLEN can used here.

...

> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 9a3eb7a..889b78b 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -750,6 +750,10 @@ static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
>
>         if (likely(vport)) {
>                 u16 mru = OVS_CB(skb)->mru;
> +               u16 cutlen = OVS_CB(skb)->cutlen;
> +
> +               if (cutlen > 0)
> +                       pskb_trim(skb, skb->len - cutlen);
>
>                 if (likely(!mru || (skb->len <= mru + ETH_HLEN))) {
>                         ovs_vport_send(vport, skb);
> @@ -858,10 +862,19 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
>                 return 0;
>
>         /* The only known usage of sample action is having a single user-space
> +        * action, or having a truncate action followed by a single user-space
>          * action. Treat this usage as a special case.
>          * The output_userspace() should clone the skb to be sent to the
>          * user space. This skb will be consumed by its caller.
>          */
> +       if (unlikely(nla_type(a) == OVS_ACTION_ATTR_TRUNC)) {
> +               struct ovs_action_trunc *trunc = nla_data(a);
> +
> +               OVS_CB(skb)->cutlen = skb->len > trunc->max_len ?
> +                                               skb->len - trunc->max_len : 0;

There is no need to initialize the cutlen to zero if max-len is
greater then skb len. it should be already zero.

> +               a = nla_next(a, &rem);
> +       }
> +
There cutlen is set in skb cb, but next action could be other than
userspace, in that case cutlen is leaked to remain action list.

>         if (likely(nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
>                    nla_is_last(a, rem)))
>                 return output_userspace(dp, skb, key, a, actions, actions_len);
> @@ -1044,13 +1057,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>         for (a = attr, rem = len; rem > 0;
>              a = nla_next(a, &rem)) {
>                 int err = 0;
> +               int cutlen = OVS_CB(skb)->cutlen;
>
>                 if (unlikely(prev_port != -1)) {
>                         struct sk_buff *out_skb = skb_clone(skb, GFP_ATOMIC);
>
> -                       if (out_skb)
> +                       if (out_skb) {
> +                               /* skb_clone does not clone the CB. */
Are you sure about this? __copy_skb_header() seem to be doing the copy.

> +                               OVS_CB(out_skb)->cutlen = cutlen;
>                                 do_output(dp, out_skb, prev_port, key);
> +                       }
>
> +                       OVS_CB(skb)->cutlen = 0;
You also need to set cutlen local variable. otherwise next output
could pickup the value.

>                         prev_port = -1;
>                 }
>
> @@ -1059,6 +1077,14 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>                         prev_port = nla_get_u32(a);
>                         break;
>
> +               case OVS_ACTION_ATTR_TRUNC: {
> +                       struct ovs_action_trunc *trunc = nla_data(a);
> +
> +                       OVS_CB(skb)->cutlen = skb->len > trunc->max_len ?
> +                                               skb->len - trunc->max_len : 0;
similar to sample() function comment, no need to set OVS_CB cutlen to zero.

> +                       break;
> +               }
> +
>                 case OVS_ACTION_ATTR_USERSPACE:
>                         output_userspace(dp, skb, key, a, attr, len);
>                         break;
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 856bd8d..fb3e5a3 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -425,6 +425,7 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>         size_t len;
>         unsigned int hlen;
>         int err, dp_ifindex;
> +       int cutlen = OVS_CB(skb)->cutlen;
>
>         dp_ifindex = get_dpifindex(dp);
>         if (!dp_ifindex)
> @@ -461,6 +462,9 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>         else
>                 hlen = skb->len;
>
> +       if (cutlen > 0)
> +               hlen -= cutlen;
> +
I do not think headline should be changed in case of sib-zero copy.
This is size of linear data in skb, changing this value could result
in wrong data copy in skb-zerocpy.
Also no need to compare cut-lea to zero, it can be directly subtracted.