Commit log needs some work.
So my understanding is, this patch does not do much functionally,
but makes adding the hash feature easier. OK.
On Thu, Feb 13, 2025 at 03:54:06PM +0900, Akihiko Odaki 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
I think you mean "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 fill the buffer in tun.
What is missing here is description of what the patch does.
I think it is
"Replace advancing the iterator with writing zeros".
There could be performance cost to the dirtying extra cache lines, though.
Could you try checking that please?
I think we should mention the risks of the patch, too.
Maybe:
Also in theory, 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 specification also says the device MUST set num_buffers to 1 when
> the field is present so set it when the specified header size is big
> enough to contain the field.
This part I dislike. tun has no idea what the number of buffers is.
Why 1 specifically?
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>
> ---
> drivers/net/tap.c | 2 +-
> drivers/net/tun.c | 6 ++++--
> drivers/net/tun_vnet.h | 14 +++++++++-----
> 3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index 1287e241f4454fb8ec4975bbaded5fbaa88e3cc8..d96009153c316f669e626d95002e5fe8add3a1b2 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -711,7 +711,7 @@ static ssize_t tap_put_user(struct tap_queue *q,
> int total;
>
> if (q->flags & IFF_VNET_HDR) {
> - struct virtio_net_hdr vnet_hdr;
> + struct virtio_net_hdr_v1 vnet_hdr;
>
> vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index b14231a743915c2851eaae49d757b763ec4a8841..a3aed7e42c63d8b8f523c0141c7d970ab185178c 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1987,7 +1987,9 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun,
> ssize_t ret;
>
> if (tun->flags & IFF_VNET_HDR) {
> - struct virtio_net_hdr gso = { 0 };
> + struct virtio_net_hdr_v1 gso = {
> + .num_buffers = cpu_to_tun_vnet16(tun->flags, 1)
> + };
>
> vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz);
> ret = tun_vnet_hdr_put(vnet_hdr_sz, iter, &gso);
> @@ -2040,7 +2042,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
> }
>
> if (vnet_hdr_sz) {
> - struct virtio_net_hdr gso;
> + struct virtio_net_hdr_v1 gso;
>
> ret = tun_vnet_hdr_from_skb(tun->flags, tun->dev, skb, &gso);
> if (ret)
> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
> index fd7411c4447ffb180e032fe3e22f6709c30da8e9..b4f406f522728f92266898969831c26a87930f6a 100644
> --- a/drivers/net/tun_vnet.h
> +++ b/drivers/net/tun_vnet.h
> @@ -135,15 +135,17 @@ static inline int tun_vnet_hdr_get(int sz, unsigned int flags,
> }
>
> static inline int tun_vnet_hdr_put(int sz, struct iov_iter *iter,
> - const struct virtio_net_hdr *hdr)
> + const struct virtio_net_hdr_v1 *hdr)
> {
> + int content_sz = MIN(sizeof(*hdr), sz);
> +
> if (unlikely(iov_iter_count(iter) < sz))
> return -EINVAL;
>
> - if (unlikely(copy_to_iter(hdr, sizeof(*hdr), iter) != sizeof(*hdr)))
> + if (unlikely(copy_to_iter(hdr, content_sz, iter) != content_sz))
> return -EFAULT;
>
> - iov_iter_advance(iter, sz - sizeof(*hdr));
> + iov_iter_zero(sz - content_sz, iter);
>
> return 0;
> }
> @@ -157,11 +159,11 @@ static inline int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb,
> static inline int tun_vnet_hdr_from_skb(unsigned int flags,
> const struct net_device *dev,
> const struct sk_buff *skb,
> - struct virtio_net_hdr *hdr)
> + struct virtio_net_hdr_v1 *hdr)
> {
> int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0;
>
> - if (virtio_net_hdr_from_skb(skb, hdr,
> + if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)hdr,
> tun_vnet_is_little_endian(flags), true,
> vlan_hlen)) {
> struct skb_shared_info *sinfo = skb_shinfo(skb);
> @@ -179,6 +181,8 @@ static inline int tun_vnet_hdr_from_skb(unsigned int flags,
> return -EINVAL;
> }
>
> + hdr->num_buffers = cpu_to_tun_vnet16(flags, 1);
> +
> 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,
> --
> Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>
