| 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
| ||
|
Message-ID: <4eaa7aaa-f677-4a31-bcc2-badcb5e2b9f6@daynix.com>
Date: Wed, 4 Jun 2025 16:20:18 +0900
From: Akihiko Odaki <akihiko.odaki@...nix.com>
To: Jason Wang <jasowang@...hat.com>
Cc: Jonathan Corbet <corbet@....net>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
"Michael S. Tsirkin" <mst@...hat.com>, Xuan Zhuo
<xuanzhuo@...ux.alibaba.com>, Shuah Khan <shuah@...nel.org>,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, kvm@...r.kernel.org,
virtualization@...ts.linux-foundation.org, linux-kselftest@...r.kernel.org,
Yuri Benditovich <yuri.benditovich@...nix.com>,
Andrew Melnychenko <andrew@...nix.com>,
Stephen Hemminger <stephen@...workplumber.org>, gur.stavi@...wei.com,
Lei Yang <leiyang@...hat.com>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH net-next v12 01/10] virtio_net: Add functions for hashing
On 2025/06/04 10:18, Jason Wang wrote:
> On Tue, Jun 3, 2025 at 1:31 PM Akihiko Odaki <akihiko.odaki@...nix.com> wrote:
>>
>> On 2025/06/03 12:19, Jason Wang wrote:
>>> On Fri, May 30, 2025 at 12:50 PM Akihiko Odaki <akihiko.odaki@...nix.com> wrote:
>>>>
>>>> They are useful to implement VIRTIO_NET_F_RSS and
>>>> VIRTIO_NET_F_HASH_REPORT.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@...nix.com>
>>>> Tested-by: Lei Yang <leiyang@...hat.com>
>>>> ---
>>>> include/linux/virtio_net.h | 188 +++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 188 insertions(+)
>>>>
>>>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>>>> index 02a9f4dc594d..426f33b4b824 100644
>>>> --- a/include/linux/virtio_net.h
>>>> +++ b/include/linux/virtio_net.h
>>>> @@ -9,6 +9,194 @@
>>>> #include <uapi/linux/tcp.h>
>>>> #include <uapi/linux/virtio_net.h>
>>>>
>>>> +struct virtio_net_hash {
>>>> + u32 value;
>>>> + u16 report;
>>>> +};
>>>> +
>>>> +struct virtio_net_toeplitz_state {
>>>> + u32 hash;
>>>> + const u32 *key;
>>>> +};
>>>> +
>>>> +#define VIRTIO_NET_SUPPORTED_HASH_TYPES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \
>>>> + VIRTIO_NET_RSS_HASH_TYPE_TCPv4 | \
>>>> + VIRTIO_NET_RSS_HASH_TYPE_UDPv4 | \
>>>> + VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \
>>>> + VIRTIO_NET_RSS_HASH_TYPE_TCPv6 | \
>>>> + VIRTIO_NET_RSS_HASH_TYPE_UDPv6)
>>>> +
>>>> +#define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
>>>> +
>>>> +static inline void virtio_net_toeplitz_convert_key(u32 *input, size_t len)
>>>> +{
>>>> + while (len >= sizeof(*input)) {
>>>> + *input = be32_to_cpu((__force __be32)*input);
>>>> + input++;
>>>> + len -= sizeof(*input);
>>>> + }
>>>> +}
>>>> +
>>>> +static inline void virtio_net_toeplitz_calc(struct virtio_net_toeplitz_state *state,
>>>> + const __be32 *input, size_t len)
>>>> +{
>>>> + while (len >= sizeof(*input)) {
>>>> + for (u32 map = be32_to_cpu(*input); map; map &= (map - 1)) {
>>>> + u32 i = ffs(map);
>>>> +
>>>> + state->hash ^= state->key[0] << (32 - i) |
>>>> + (u32)((u64)state->key[1] >> i);
>>>> + }
>>>> +
>>>> + state->key++;
>>>> + input++;
>>>> + len -= sizeof(*input);
>>>> + }
>>>> +}
>>>> +
>>>> +static inline u8 virtio_net_hash_key_length(u32 types)
>>>> +{
>>>> + size_t len = 0;
>>>> +
>>>> + if (types & VIRTIO_NET_HASH_REPORT_IPv4)
>>>> + len = max(len,
>>>> + sizeof(struct flow_dissector_key_ipv4_addrs));
>>>> +
>>>> + if (types &
>>>> + (VIRTIO_NET_HASH_REPORT_TCPv4 | VIRTIO_NET_HASH_REPORT_UDPv4))
>>>> + len = max(len,
>>>> + sizeof(struct flow_dissector_key_ipv4_addrs) +
>>>> + sizeof(struct flow_dissector_key_ports));
>>>> +
>>>> + if (types & VIRTIO_NET_HASH_REPORT_IPv6)
>>>> + len = max(len,
>>>> + sizeof(struct flow_dissector_key_ipv6_addrs));
>>>> +
>>>> + if (types &
>>>> + (VIRTIO_NET_HASH_REPORT_TCPv6 | VIRTIO_NET_HASH_REPORT_UDPv6))
>>>> + len = max(len,
>>>> + sizeof(struct flow_dissector_key_ipv6_addrs) +
>>>> + sizeof(struct flow_dissector_key_ports));
>>>> +
>>>> + return len + sizeof(u32);
>>>> +}
>>>> +
>>>> +static inline u32 virtio_net_hash_report(u32 types,
>>>> + const struct flow_keys_basic *keys)
>>>> +{
>>>> + switch (keys->basic.n_proto) {
>>>> + case cpu_to_be16(ETH_P_IP):
>>>> + if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
>>>> + if (keys->basic.ip_proto == IPPROTO_TCP &&
>>>> + (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4))
>>>> + return VIRTIO_NET_HASH_REPORT_TCPv4;
>>>> +
>>>> + if (keys->basic.ip_proto == IPPROTO_UDP &&
>>>> + (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv4))
>>>> + return VIRTIO_NET_HASH_REPORT_UDPv4;
>>>> + }
>>>> +
>>>> + if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv4)
>>>> + return VIRTIO_NET_HASH_REPORT_IPv4;
>>>> +
>>>> + return VIRTIO_NET_HASH_REPORT_NONE;
>>>> +
>>>> + case cpu_to_be16(ETH_P_IPV6):
>>>> + if (!(keys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
>>>> + if (keys->basic.ip_proto == IPPROTO_TCP &&
>>>> + (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6))
>>>> + return VIRTIO_NET_HASH_REPORT_TCPv6;
>>>> +
>>>> + if (keys->basic.ip_proto == IPPROTO_UDP &&
>>>> + (types & VIRTIO_NET_RSS_HASH_TYPE_UDPv6))
>>>> + return VIRTIO_NET_HASH_REPORT_UDPv6;
>>>> + }
>>>> +
>>>> + if (types & VIRTIO_NET_RSS_HASH_TYPE_IPv6)
>>>> + return VIRTIO_NET_HASH_REPORT_IPv6;
>>>> +
>>>> + return VIRTIO_NET_HASH_REPORT_NONE;
>>>> +
>>>> + default:
>>>> + return VIRTIO_NET_HASH_REPORT_NONE;
>>>> + }
>>>> +}
>>>> +
>>>> +static inline void virtio_net_hash_rss(const struct sk_buff *skb,
>>>> + u32 types, const u32 *key,
>>>> + struct virtio_net_hash *hash)
>>>> +{
>>>> + struct virtio_net_toeplitz_state toeplitz_state = { .key = key };
>>>> + struct flow_keys flow;
>>>> + struct flow_keys_basic flow_basic;
>>>> + u16 report;
>>>> +
>>>> + if (!skb_flow_dissect_flow_keys(skb, &flow, 0)) {
>>>> + hash->report = VIRTIO_NET_HASH_REPORT_NONE;
>>>> + return;
>>>> + }
>>>> +
>>>> + flow_basic = (struct flow_keys_basic) {
>>>> + .control = flow.control,
>>>> + .basic = flow.basic
>>>> + };
>>>> +
>>>> + report = virtio_net_hash_report(types, &flow_basic);
>>>> +
>>>> + switch (report) {
>>>> + case VIRTIO_NET_HASH_REPORT_IPv4:
>>>> + virtio_net_toeplitz_calc(&toeplitz_state,
>>>> + (__be32 *)&flow.addrs.v4addrs,
>>>> + sizeof(flow.addrs.v4addrs));
>>>> + break;
>>>> +
>>>> + case VIRTIO_NET_HASH_REPORT_TCPv4:
>>>> + virtio_net_toeplitz_calc(&toeplitz_state,
>>>> + (__be32 *)&flow.addrs.v4addrs,
>>>> + sizeof(flow.addrs.v4addrs));
>>>> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
>>>> + sizeof(flow.ports.ports));
>>>> + break;
>>>> +
>>>> + case VIRTIO_NET_HASH_REPORT_UDPv4:
>>>> + virtio_net_toeplitz_calc(&toeplitz_state,
>>>> + (__be32 *)&flow.addrs.v4addrs,
>>>> + sizeof(flow.addrs.v4addrs));
>>>> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
>>>> + sizeof(flow.ports.ports));
>>>> + break;
>>>> +
>>>> + case VIRTIO_NET_HASH_REPORT_IPv6:
>>>> + virtio_net_toeplitz_calc(&toeplitz_state,
>>>> + (__be32 *)&flow.addrs.v6addrs,
>>>> + sizeof(flow.addrs.v6addrs));
>>>> + break;
>>>> +
>>>> + case VIRTIO_NET_HASH_REPORT_TCPv6:
>>>> + virtio_net_toeplitz_calc(&toeplitz_state,
>>>> + (__be32 *)&flow.addrs.v6addrs,
>>>> + sizeof(flow.addrs.v6addrs));
>>>> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
>>>> + sizeof(flow.ports.ports));
>>>> + break;
>>>> +
>>>> + case VIRTIO_NET_HASH_REPORT_UDPv6:
>>>> + virtio_net_toeplitz_calc(&toeplitz_state,
>>>> + (__be32 *)&flow.addrs.v6addrs,
>>>> + sizeof(flow.addrs.v6addrs));
>>>> + virtio_net_toeplitz_calc(&toeplitz_state, &flow.ports.ports,
>>>> + sizeof(flow.ports.ports));
>>>> + break;
>>>> +
>>>> + default:
>>>> + hash->report = VIRTIO_NET_HASH_REPORT_NONE;
>>>> + return;
>>>
>>> So I still think we need a comment here to explain why this is not an
>>> issue if the device can report HASH_XXX_EX. Or we need to add the
>>> support, since this is the code from the driver side, I don't think we
>>> need to worry about the device implementation issues.
>>
>> This is on the device side, and don't report HASH_TYPE_XXX_EX.
>>
>>>
>>> For the issue of the number of options, does the spec forbid fallback
>>> to VIRTIO_NET_HASH_REPORT_NONE? If not, we can do that.
>>
>> 5.1.6.4.3.4 "IPv6 packets with extension header" says:
>> > If VIRTIO_NET_HASH_TYPE_TCP_EX is set and the packet has a TCPv6
>> > header, the hash is calculated over the following fields:
>> > - Home address from the home address option in the IPv6 destination
>> > options header. If the extension header is not present, use the
>> > Source IPv6 address.
>> > - IPv6 address that is contained in the Routing-Header-Type-2 from the
>> > associated extension header. If the extension header is not present,
>> > use the Destination IPv6 address.
>> > - Source TCP port
>> > - Destination TCP port
>>
>> Therefore, if VIRTIO_NET_HASH_TYPE_TCP_EX is set, the packet has a TCPv6
>> and an home address option in the IPv6 destination options header is
>> present, the hash is calculated over the home address. If the hash is
>> not calculated over the home address in such a case, the device is
>> contradicting with this section and violating the spec. The same goes
>> for the other HASH_TYPE_XXX_EX types and Routing-Header-Type-2.
>
> Just to make sure we are one the same page. I meant:
>
> 1) If the hash is not calculated over the home address (in the case of
> IPv6 destination destination), it can still report
> VIRTIO_NET_RSS_HASH_TYPE_IPv6. This is what you implemented in your
> series. So the device can simply fallback to e.g TCPv6 if it can't
> understand all or part of the IPv6 options.
The spec says it can fallback if "the extension header is not present",
not if the device can't understand the extension header.
> 2) the VIRTIO_NET_SUPPORTED_HASH_TYPES is not checked against the
> tun_vnet_ioctl_sethash(), so userspace may set
> VIRTIO_NET_HASH_TYPE_TCP_EX regardless of what has been returned by
> tun_vnet_ioctl_gethashtypes(). In this case they won't get
> VIRTIO_NET_HASH_TYPE_TCP_EX.
That's right. It's the responsibility of the userspace to set only the
supported hash types.
> 3) implementing part of the hash types might complicate the migration
> or at least we need to describe the expectations of libvirt or other
> management in this case. For example, do we plan to have a dedicated
> Qemu command line like:
>
> -device virtio-net-pci,hash_report=on,supported_hash_types=X,Y,Z?
I posted a patch series to implement such a command line for vDPA[1].
The patch series that wires this tuntap feature up[2] reuses the
infrastructure so it doesn't bring additional complexity.
[1]
https://lore.kernel.org/qemu-devel/20250530-vdpa-v1-0-5af4109b1c19@daynix.com/
[2]
https://lore.kernel.org/qemu-devel/20250530-hash-v5-0-343d7d7a8200@daynix.com/
Regards,
Akihiko Odaki
Powered by blists - more mailing lists