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.
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 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.
Signed-off-by: Akihiko Odaki akihiko.odaki@daynix.com --- Changes in v2: - Dropped the code to set num_buffers to 1. - Incorporated a grammatical improvement suggested by Michael S. Tsirkin. - Added an explanation of this patch's risk suggested by Michael S. Tsirkin. - Noted that it will not make another cache line dirty. - Added an error check. - Link to v1: https://lore.kernel.org/r/20250213-buffers-v1-1-ec4a0821957a@daynix.com --- drivers/net/tun_vnet.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h index fd7411c4447ffb180e032fe3e22f6709c30da8e9..58b9ac7a5fc4084c789fe94fe36b5f8631bf1fa4 100644 --- a/drivers/net/tun_vnet.h +++ b/drivers/net/tun_vnet.h @@ -143,7 +143,8 @@ static inline int tun_vnet_hdr_put(int sz, struct iov_iter *iter, if (unlikely(copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr))) return -EFAULT;
- iov_iter_advance(iter, sz - sizeof(*hdr)); + if (iov_iter_zero(sz - sizeof(*hdr), iter) != sz - sizeof(*hdr)) + return -EFAULT;
return 0; }
--- base-commit: f54eab84fc17ef79b701e29364b7d08ca3a1d2f6 change-id: 20250116-buffers-96e14bf023fc prerequisite-change-id: 20241230-tun-66e10a49b0c7:v6 prerequisite-patch-id: 871dc5f146fb6b0e3ec8612971a8e8190472c0fb prerequisite-patch-id: 2797ed249d32590321f088373d4055ff3f430a0e prerequisite-patch-id: ea3370c72d4904e2f0536ec76ba5d26784c0cede prerequisite-patch-id: 837e4cf5d6b451424f9b1639455e83a260c4440d prerequisite-patch-id: ea701076f57819e844f5a35efe5cbc5712d3080d prerequisite-patch-id: 701646fb43ad04cc64dd2bf13c150ccbe6f828ce prerequisite-patch-id: 53176dae0c003f5b6c114d43f936cf7140d31bb5
Best regards,
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?
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'.
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
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
On Thu, Feb 20, 2025 at 4:45 PM Michael S. Tsirkin mst@redhat.com wrote:
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.
Yes, tun currently only recognizes a subset of the whole virtio-net header.
Thanks
Hello:
This patch was applied to netdev/net-next.git (main) by Jakub Kicinski kuba@kernel.org:
On Sat, 15 Feb 2025 15:04:50 +0900 you 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.
[...]
Here is the summary with links: - [net-next,v2] tun: Pad virtio headers https://git.kernel.org/netdev/net-next/c/4adf74971018
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
Akihiko Odaki -
Jason Wang -
Michael S. Tsirkin -
Paolo Abeni -
patchwork-bot+netdevbpf＠kernel.org