On Thu, Feb 20, 2025 at 08:58:38AM +0100, Paolo Abeni wrote: > Hi, > > On 2/15/25 7:04 AM, Akihiko Odaki wrote: > > tun simply advances iov_iter when it needs to pad virtio header, > > which leaves the garbage in the buffer as is. This will become > > especially problematic when tun starts to allow enabling the hash > > reporting feature; even if the feature is enabled, the packet may lack a > > hash value and may contain a hole in the virtio header because the > > packet arrived before the feature gets enabled or does not contain the > > header fields to be hashed. If the hole is not filled with zero, it is > > impossible to tell if the packet lacks a hash value. > > Should virtio starting sending packets only after feature negotiation? > In other words, can the above happen without another bug somewhere else?
Not if this is connected with a guest with the standard virtio driver, no. The issue is that tun has no concept of feature negotiation, and we don't know who uses the vnet header feature, or why. > I guess the following question is mostly for Jason and Michael: could be > possible (/would it make any sense) to use a virtio_net_hdr `flags` bit > to explicitly signal the hash fields presence? i.e. making the actual > virtio_net_hdr size 'dynamic'. But it is dynamic - that is why we have TUNSETVNETHDRSZ. > > In theory, a user of tun can fill the buffer with zero before calling > > read() to avoid such a problem, but leaving the garbage in the buffer is > > awkward anyway so replace advancing the iterator with writing zeros. > > > > A user might have initialized the buffer to some non-zero value, > > expecting tun to skip writing it. As this was never a documented > > feature, this seems unlikely. > > > > The overhead of filling the hole in the header is negligible when the > > header size is specified according to the specification as doing so will > > not make another cache line dirty under a reasonable assumption. Below > > is a proof of this statement: > > > > The first 10 bytes of the header is always written and tun also writes > > the packet itself immediately after the > > packet unless the packet is > > ^^^^^ this possibly should be 'virtio header'. Otherwise the sentence > is hard to follow for me. > > > empty. This makes a hole between these writes whose size is: sz - 10 > > where sz is the specified header size. > > > > Therefore, we will never make another cache line dirty when: > > sz < L1_CACHE_BYTES + 10 > > where L1_CACHE_BYTES is the cache line size. Assuming > > L1_CACHE_BYTES >= 16, this inequation holds when: sz < 26. > > > > sz <= 20 according to the current specification so we even have a > > margin of 5 bytes in case that the header size grows in a future version > > of the specification. > > FTR, the upcoming GSO over UDP tunnel support will add other 4 bytes to > the header. but that will still fit the given boundary. > > /P
