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] [thread-next>] [day] [month] [year] [list]
Message-ID: <2490a542-f0fc-f0db-3494-9fdf3b8aa30b@gmail.com>
Date:   Tue, 12 Nov 2019 10:06:48 -0800
From:   Gregory Rose <gvrose8192@...il.com>
To:     xiangxia.m.yue@...il.com, pshelar@....org, blp@....org
Cc:     dev@...nvswitch.org, netdev@...r.kernel.org, ychen103103@....com
Subject: Re: [ovs-dev] [PATCH net-next v3] net: openvswitch: add hash info to
 upcall


On 11/12/2019 7:08 AM, xiangxia.m.yue@...il.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@...il.com>
>
> When using the kernel datapath, the upcall don't
> include skb hash info relatived. That will introduce
> some problem, because the hash of skb is important
> in kernel stack. For example, VXLAN module uses
> it to select UDP src port. The tx queue selection
> may also use the hash in stack.
>
> Hash is computed in different ways. Hash is random
> for a TCP socket, and hash may be computed in hardware,
> or software stack. Recalculation hash is not easy.
>
> Hash of TCP socket is computed:
>    tcp_v4_connect
>      -> sk_set_txhash (is random)
>
>    __tcp_transmit_skb
>      -> skb_set_hash_from_sk
>
> There will be one upcall, without information of skb
> hash, to ovs-vswitchd, for the first packet of a TCP
> session. The rest packets will be processed in Open vSwitch
> modules, hash kept. If this tcp session is forward to
> VXLAN module, then the UDP src port of first tcp packet
> is different from rest packets.
>
> TCP packets may come from the host or dockers, to Open vSwitch.
> To fix it, we store the hash info to upcall, and restore hash
> when packets sent back.
>
> +---------------+          +-------------------------+
> |   Docker/VMs  |          |     ovs-vswitchd        |
> +----+----------+          +-+--------------------+--+
>       |                       ^                    |
>       |                       |                    |
>       |                       |  upcall            v restore packet hash (not recalculate)
>       |                     +-+--------------------+--+
>       |  tap netdev         |                         |   vxlan module
>       +--------------->     +-->  Open vSwitch ko     +-->
>         or internal type    |                         |
>                             +-------------------------+
>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/364062.html
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>
> ---
> v3:
> * add enum ovs_pkt_hash_types
> * avoid duplicate call the skb_get_hash_raw.
> * explain why we should fix this problem.
> ---
>   include/uapi/linux/openvswitch.h |  2 ++
>   net/openvswitch/datapath.c       | 30 +++++++++++++++++++++++++++++-
>   net/openvswitch/datapath.h       | 12 ++++++++++++
>   3 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index 1887a451c388..e65407c1f366 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -170,6 +170,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_HASH: Packet hash info (e.g. hash, sw_hash and l4_hash in skb).
>    * @OVS_PACKET_ATTR_LEN: Packet size before truncation.
>    * %OVS_PACKET_ATTR_USERSPACE action specify the Maximum received fragment
>    * size.
> @@ -190,6 +191,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_HASH,	    /* Packet hash. */
>   	OVS_PACKET_ATTR_LEN,		/* Packet size before truncation. */
>   	__OVS_PACKET_ATTR_MAX
>   };

Why do you add the new enum before the last entry OVS_PACKET_ATTR_LEN 
instead of
just adding it to the end of the list?

Just curious.

- Greg

> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 2088619c03f0..b556cf62b77c 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -350,7 +350,8 @@ static size_t upcall_msg_size(const struct dp_upcall_info *upcall_info,
>   	size_t size = NLMSG_ALIGN(sizeof(struct ovs_header))
>   		+ nla_total_size(hdrlen) /* OVS_PACKET_ATTR_PACKET */
>   		+ nla_total_size(ovs_key_attr_size()) /* OVS_PACKET_ATTR_KEY */
> -		+ nla_total_size(sizeof(unsigned int)); /* OVS_PACKET_ATTR_LEN */
> +		+ nla_total_size(sizeof(unsigned int)) /* OVS_PACKET_ATTR_LEN */
> +		+ nla_total_size(sizeof(u64)); /* OVS_PACKET_ATTR_HASH */
>   
>   	/* OVS_PACKET_ATTR_USERDATA */
>   	if (upcall_info->userdata)
> @@ -393,6 +394,7 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>   	size_t len;
>   	unsigned int hlen;
>   	int err, dp_ifindex;
> +	u64 hash;
>   
>   	dp_ifindex = get_dpifindex(dp);
>   	if (!dp_ifindex)
> @@ -504,6 +506,23 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>   		pad_packet(dp, user_skb);
>   	}
>   
> +	hash = skb_get_hash_raw(skb);
> +	if (hash) {
> +		if (skb->sw_hash)
> +			hash |= OVS_PACKET_HASH_SW_BIT;
> +
> +		if (skb->l4_hash)
> +			hash |= OVS_PACKET_HASH_L4_BIT;
> +
> +		if (nla_put(user_skb, OVS_PACKET_ATTR_HASH,
> +			    sizeof (u64), &hash)) {
> +			err = -ENOBUFS;
> +			goto out;
> +		}
> +
> +		pad_packet(dp, user_skb);
> +	}
> +
>   	/* Only reserve room for attribute header, packet data is added
>   	 * in skb_zerocopy() */
>   	if (!(nla = nla_reserve(user_skb, OVS_PACKET_ATTR_PACKET, 0))) {
> @@ -543,6 +562,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>   	struct datapath *dp;
>   	struct vport *input_vport;
>   	u16 mru = 0;
> +	u64 hash;
>   	int len;
>   	int err;
>   	bool log = !a[OVS_PACKET_ATTR_PROBE];
> @@ -568,6 +588,14 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>   	}
>   	OVS_CB(packet)->mru = mru;
>   
> +	if (a[OVS_PACKET_ATTR_HASH]) {
> +		hash = nla_get_u64(a[OVS_PACKET_ATTR_HASH]);
> +
> +		__skb_set_hash(packet, hash & 0xFFFFFFFFULL,
> +			       !!(hash & OVS_PACKET_HASH_SW_BIT),
> +			       !!(hash & OVS_PACKET_HASH_L4_BIT));
> +	}
> +
>   	/* Build an sw_flow for sending this packet. */
>   	flow = ovs_flow_alloc();
>   	err = PTR_ERR(flow);
> diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
> index 81e85dde8217..e239a46c2f94 100644
> --- a/net/openvswitch/datapath.h
> +++ b/net/openvswitch/datapath.h
> @@ -139,6 +139,18 @@ struct ovs_net {
>   	bool xt_label;
>   };
>   
> +/**
> + * enum ovs_pkt_hash_types - hash info to include with a packet
> + * to send to userspace.
> + * @OVS_PACKET_HASH_SW_BIT: indicates hash was computed in software stack.
> + * @OVS_PACKET_HASH_L4_BIT: indicates hash is a canonical 4-tuple hash
> + * over transport ports.
> + */
> +enum ovs_pkt_hash_types {
> +	OVS_PACKET_HASH_SW_BIT = (1ULL << 32),
> +	OVS_PACKET_HASH_L4_BIT = (1ULL << 33),
> +};
> +
>   extern unsigned int ovs_net_id;
>   void ovs_lock(void);
>   void ovs_unlock(void);

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ