Re: [PATCH v2 2/3] tun: Pad virtio header with zero

Akihiko Odaki
2025-01-10T10:25:36Z

On 2025/01/10 12:27, Jason Wang wrote:
> On Thu, Jan 9, 2025 at 2:59 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> tun used to simply advance iov_iter when it needs to pad virtio header,
>> which leaves the garbage in the buffer as is. This is 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.
>
> I'm not sure I will get here, could we do this in the series of hash reporting?

I'll create another series dedicated for this and num_buffers change as
suggested by Willem.

>
>>
>> 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 fill the buffer in tun.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> drivers/net/tun_vnet.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/tun_vnet.c b/drivers/net/tun_vnet.c
>> index fe842df9e9ef..ffb2186facd3 100644
>> --- a/drivers/net/tun_vnet.c
>> +++ b/drivers/net/tun_vnet.c
>> @@ -138,7 +138,8 @@ int tun_vnet_hdr_put(int sz, struct iov_iter *iter,
>> if (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;
>
> There're various callers of iov_iter_advance(), do we need to fix them all?

No. For example, there are iov_iter_advance() calls for SOCK_ZEROCOPY in
tun_get_user() and tap_get_user(). They are fine as they are not writing
buffers after skipping.

The problem is that read_iter() and recvmsg() says it wrote N bytes but
it leaves some of this N bytes uninialized. Such an implementation may
be created even without iov_iter_advance() (for example just returning a
too big number), and it is equally problematic with the current
tun_get_user()/tap_get_user().

Regards,
Akihiko Odaki

>
> Thanks
>
>> }
>
>>
>> --
>> 2.47.1
>>
>