| 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: <6efc6937-2da7-4eb1-a2de-c9e5146d10ea@daynix.com>
Date: Mon, 23 Sep 2024 20:15:44 +0200
From: Akihiko Odaki <akihiko.odaki@...nix.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
Jonathan Corbet <corbet@....net>, Jason Wang <jasowang@...hat.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>
Subject: Re: [PATCH RFC v3 2/9] virtio_net: Add functions for hashing
On 2024/09/18 14:50, Willem de Bruijn wrote:
> Akihiko Odaki 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>
>> ---
>> include/linux/virtio_net.h | 198 +++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 198 insertions(+)
>>
>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>> index 6c395a2600e8..7ee2e2f2625a 100644
>> --- a/include/linux/virtio_net.h
>> +++ b/include/linux/virtio_net.h
>> @@ -9,6 +9,183 @@
>> #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;
>> + u32 key_buffer;
>> + const __be32 *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(struct virtio_net_toeplitz_state *state,
>> + const __be32 *input, size_t len)
>> +{
>> + u32 key;
>> +
>> + while (len) {
>> + state->key++;
>> + key = be32_to_cpu(*state->key);
>> +
>> + for (u32 bit = BIT(31); bit; bit >>= 1) {
>> + if (be32_to_cpu(*input) & bit)
>> + state->hash ^= state->key_buffer;
>> +
>> + state->key_buffer =
>> + (state->key_buffer << 1) | !!(key & bit);
>> + }
>> +
>> + input++;
>> + len--;
>> + }
>> +}
>> +
>> +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 4 + len;
>
> Avoid raw constants like this 4. What field does it capture?
It is: sizeof_field(struct virtio_net_toeplitz_state, key_buffer)
I'll replace it with v4.
>
> Instead of working from shortest to longest and using max, how about
> the inverse and return as soon as a match is found.
I think it is less error-prone to use max() as it does not require to
sort the numbers. The compiler should properly optimize it in the way
you suggested.
>
>> +}
>> +
>> +static inline u32 virtio_net_hash_report(u32 types,
>> + struct flow_dissector_key_basic key)
>> +{
>> + switch (key.n_proto) {
>> + case htons(ETH_P_IP):
>> + if (key.ip_proto == IPPROTO_TCP &&
>> + (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv4))
>> + return VIRTIO_NET_HASH_REPORT_TCPv4;
>> +
>> + if (key.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 htons(ETH_P_IPV6):
>> + if (key.ip_proto == IPPROTO_TCP &&
>> + (types & VIRTIO_NET_RSS_HASH_TYPE_TCPv6))
>> + return VIRTIO_NET_HASH_REPORT_TCPv6;
>> +
>> + if (key.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 bool virtio_net_hash_rss(const struct sk_buff *skb,
>> + u32 types, const __be32 *key,
>> + struct virtio_net_hash *hash)
>> +{
>> + u16 report;
>
> nit: move below the struct declarations.
I'll change accordingly with v4.
>
>> + struct virtio_net_toeplitz_state toeplitz_state = {
>> + .key_buffer = be32_to_cpu(*key),
>> + .key = key
>> + };
>> + struct flow_keys flow;
>> +
>> + if (!skb_flow_dissect_flow_keys(skb, &flow, 0))
>> + return false;
>> +
>> + report = virtio_net_hash_report(types, flow.basic);
>> +
>> + switch (report) {
>> + case VIRTIO_NET_HASH_REPORT_IPv4:
>> + virtio_net_toeplitz(&toeplitz_state,
>> + (__be32 *)&flow.addrs.v4addrs,
>> + sizeof(flow.addrs.v4addrs) / 4);
>> + break;
>> +
>> + case VIRTIO_NET_HASH_REPORT_TCPv4:
>> + virtio_net_toeplitz(&toeplitz_state,
>> + (__be32 *)&flow.addrs.v4addrs,
>> + sizeof(flow.addrs.v4addrs) / 4);
>> + virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
>> + 1);
>> + break;
>> +
>> + case VIRTIO_NET_HASH_REPORT_UDPv4:
>> + virtio_net_toeplitz(&toeplitz_state,
>> + (__be32 *)&flow.addrs.v4addrs,
>> + sizeof(flow.addrs.v4addrs) / 4);
>> + virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
>> + 1);
>> + break;
>> +
>> + case VIRTIO_NET_HASH_REPORT_IPv6:
>> + virtio_net_toeplitz(&toeplitz_state,
>> + (__be32 *)&flow.addrs.v6addrs,
>> + sizeof(flow.addrs.v6addrs) / 4);
>> + break;
>> +
>> + case VIRTIO_NET_HASH_REPORT_TCPv6:
>> + virtio_net_toeplitz(&toeplitz_state,
>> + (__be32 *)&flow.addrs.v6addrs,
>> + sizeof(flow.addrs.v6addrs) / 4);
>> + virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
>> + 1);
>> + break;
>> +
>> + case VIRTIO_NET_HASH_REPORT_UDPv6:
>> + virtio_net_toeplitz(&toeplitz_state,
>> + (__be32 *)&flow.addrs.v6addrs,
>> + sizeof(flow.addrs.v6addrs) / 4);
>> + virtio_net_toeplitz(&toeplitz_state, &flow.ports.ports,
>> + 1);
>> + break;
>> +
>> + default:
>> + return false;
>> + }
>> +
>> + hash->value = toeplitz_state.hash;
>> + hash->report = report;
>> +
>> + return true;
>> +}
>> +
>> static inline bool virtio_net_hdr_match_proto(__be16 protocol, __u8 gso_type)
>> {
>> switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
>> @@ -239,4 +416,25 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
>> return 0;
>> }
>>
>> +static inline int virtio_net_hdr_v1_hash_from_skb(const struct sk_buff *skb,
>> + struct virtio_net_hdr_v1_hash *hdr,
>> + bool has_data_valid,
>> + int vlan_hlen,
>> + const struct virtio_net_hash *hash)
>> +{
>> + int ret;
>> +
>> + memset(hdr, 0, sizeof(*hdr));
>> +
>> + ret = virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr,
>> + true, has_data_valid, vlan_hlen);
>> + if (!ret) {
>> + hdr->hdr.num_buffers = cpu_to_le16(1);
>> + hdr->hash_value = cpu_to_le32(hash->value);
>> + hdr->hash_report = cpu_to_le16(hash->report);
>> + }
>> +
>> + return ret;
>> +}
>> +
>
> I don't think that this helper is very helpful, as all the information
> it sets are first passed in. Just set the hdr fields directy in the
> caller. It is easier to follow the control flow.
I'll remove it in v4.
Regards,
Akihiko Odaki
Powered by blists - more mailing lists