Re: [RFC PATCH 5/7] tun: Introduce virtio-net hashing feature

On 2023/10/09 20:50, Michael S. Tsirkin wrote:

On Mon, Oct 09, 2023 at 05:44:20PM +0900, Akihiko Odaki wrote:

quoted

On 2023/10/09 17:13, Willem de Bruijn wrote:

quoted

On Sun, Oct 8, 2023 at 12:22 AM Akihiko Odaki [off-list ref] wrote:

quoted

virtio-net have two usage of hashes: one is RSS and another is hash
reporting. Conventionally the hash calculation was done by the VMM.
However, computing the hash after the queue was chosen defeats the
purpose of RSS.

Another approach is to use eBPF steering program. This approach has
another downside: it cannot report the calculated hash due to the
restrictive nature of eBPF.

Introduce the code to compute hashes to the kernel in order to overcome
thse challenges. An alternative solution is to extend the eBPF steering
program so that it will be able to report to the userspace, but it makes
little sense to allow to implement different hashing algorithms with
eBPF since the hash value reported by virtio-net is strictly defined by
the specification.

The hash value already stored in sk_buff is not used and computed
independently since it may have been computed in a way not conformant
with the specification.

Signed-off-by: Akihiko Odaki <redacted>

quoted

@@ -2116,31 +2172,49 @@ static ssize_t tun_put_user(struct tun_struct *tun,
          }

          if (vnet_hdr_sz) {
-               struct virtio_net_hdr gso;
+               union {
+                       struct virtio_net_hdr hdr;
+                       struct virtio_net_hdr_v1_hash v1_hash_hdr;
+               } hdr;
+               int ret;

                  if (iov_iter_count(iter) < vnet_hdr_sz)
                          return -EINVAL;

-               if (virtio_net_hdr_from_skb(skb, &gso,
-                                           tun_is_little_endian(tun), true,
-                                           vlan_hlen)) {
+               if ((READ_ONCE(tun->vnet_hash.flags) & TUN_VNET_HASH_REPORT) &&
+                   vnet_hdr_sz >= sizeof(hdr.v1_hash_hdr) &&
+                   skb->tun_vnet_hash) {

Isn't vnet_hdr_sz guaranteed to be >= hdr.v1_hash_hdr, by virtue of
the set hash ioctl failing otherwise?

Such checks should be limited to control path where possible

There is a potential race since tun->vnet_hash.flags and vnet_hdr_sz are not
read at once.

And then it's a complete mess and you get inconsistent
behaviour with packets getting sent all over the place, right?
So maybe keep a pointer to this struct so it can be
changed atomically then. Maybe even something with rcu I donnu.

I think it's a good idea to use RCU for the vnet_hash members, but 
vnet_hdr_sz is something not specific to vnet_hash so this check will be 
still necessary.