netdev - Re: [net-next,v2] openvswitch: Add skb->len info to upcall.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 16 Jun 2016 18:50:35 -0700
From:	pravin shelar <pshelar@....org>
To:	William Tu <u9012063@...il.com>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Pravin Shelar <pshelar@...ira.com>
Subject: Re: [net-next,v2] openvswitch: Add skb->len info to upcall.

On Thu, Jun 16, 2016 at 11:17 AM, William Tu <u9012063@...il.com> wrote:
> The commit f2a4d086ed4c ("openvswitch: Add packet truncation support.")
> introduces packet truncation before sending to userspace upcall receiver.
> This patch passes up the skb->len before truncation so that the upcall
> receiver knows the original packet size. Potentially this will be used
> by sFlow, where OVS translates sFlow config header=N to a sample action,
> truncating packet to N byte in kernel datapath. Thus, only N bytes instead
> of full-packet size is copied from kernel to userspace, saving the
> kernel-to-userspace bandwidth.
>
> Signed-off-by: William Tu <u9012063@...il.com>
> Cc: Pravin Shelar <pshelar@...ira.com>
>
> ---
> v1->v2:
> - pass skb->len to userspace instead of cutlen
> ---
>  include/uapi/linux/openvswitch.h |  2 ++
>  net/openvswitch/actions.c        |  1 +
>  net/openvswitch/datapath.c       | 15 +++++++++++++++
>  net/openvswitch/datapath.h       |  2 ++
>  4 files changed, 20 insertions(+)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index 8274675..f9e204e 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -166,6 +166,7 @@ enum ovs_packet_cmd {
>   * output port is actually a tunnel port. Contains the output tunnel key
>   * extracted from the packet as nested %OVS_TUNNEL_KEY_ATTR_* attributes.
>   * @OVS_PACKET_ATTR_MRU: Present for an %OVS_PACKET_CMD_ACTION and
> + * @OVS_PACKET_ATTR_SKBLEN: Packet size before truncation.
>   * %OVS_PACKET_ATTR_USERSPACE action specify the Maximum received fragment
>   * size.
>   *
> @@ -185,6 +186,7 @@ enum ovs_packet_attr {
>         OVS_PACKET_ATTR_PROBE,      /* Packet operation is a feature probe,
>                                        error logging should be suppressed. */
>         OVS_PACKET_ATTR_MRU,        /* Maximum received IP fragment size. */
> +       OVS_PACKET_ATTR_SKBLEN,         /* Packet size before truncation. */
>         __OVS_PACKET_ATTR_MAX
>  };
>
skb is linux specific name. I do not think it should be used in OVS
datapath interface which is not platform specific.

> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 1ecbd77..bb18ca1 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -793,6 +793,7 @@ static int output_userspace(struct datapath *dp, struct sk_buff *skb,
>         memset(&upcall, 0, sizeof(upcall));
>         upcall.cmd = OVS_PACKET_CMD_ACTION;
>         upcall.mru = OVS_CB(skb)->mru;
> +       upcall.skblen = skb->len;
>
In case of GSO packet up call breaks the packet and sends multiple
upcall, therefore this packet length is not correct.

>         for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
>                  a = nla_next(a, &rem)) {
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 6739342..3866d87 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -277,6 +277,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key)
>                 upcall.cmd = OVS_PACKET_CMD_MISS;
>                 upcall.portid = ovs_vport_find_upcall_portid(p, skb);
>                 upcall.mru = OVS_CB(skb)->mru;
> +               upcall.skblen = skb->len;

same as output_userspace(), this could be gso packet.

...
...
> diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
> index ab85c1c..f87f526 100644
> --- a/net/openvswitch/datapath.h
> +++ b/net/openvswitch/datapath.h
> @@ -120,6 +120,7 @@ struct ovs_skb_cb {
>   * counter.
>   * @egress_tun_info: If nonnull, becomes %OVS_PACKET_ATTR_EGRESS_TUN_KEY.
>   * @mru: If not zero, Maximum received IP fragment size.
> + * @skblen: The packet size before truncation.
>   */
>  struct dp_upcall_info {
>         struct ip_tunnel_info *egress_tun_info;
> @@ -129,6 +130,7 @@ struct dp_upcall_info {
>         u32 portid;
>         u8 cmd;
>         u16 mru;
> +       u32 skblen;
>  };
>
There is no need to pass this as parameter, queue_userspace_packet()
has access to the packet length.