| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251028101144-mutt-send-email-mst@kernel.org>
Date: Tue, 28 Oct 2025 10:38:24 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: xuanzhuo@...ux.alibaba.com, eperezma@...hat.com, andrew+netdev@...n.ch,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, virtualization@...ts.linux.dev,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
stable@...r.kernel.org
Subject: Re: [PATCH net] virtio-net: calculate header alignment mask based on
features
On Tue, Oct 28, 2025 at 11:03:41AM +0800, Jason Wang wrote:
> Commit 56a06bd40fab ("virtio_net: enable gso over UDP tunnel support.")
> switches to check the alignment of the virtio_net_hdr_v1_hash_tunnel
> even when doing the transmission even if the feature is not
> negotiated. This will cause
you mean this causes
>a series performance degradation of pktgen
> as the skb->data can't satisfy the alignment requirement due to the
> increase of the header size then virtio-net must prepare at least 2
> sgs with indirect descriptors which will introduce overheads in the
introduces, accordinglt
> device.
>
> Fixing this by calculate the header alignment during probe so when
> tunnel gso is not negotiated, we can less strict.
>
> Pktgen in guest + XDP_DROP on TAP + vhost_net shows the TX PPS is
> recovered from 2.4Mpps to 4.45Mpps.
>
> Note that we still need a way to recover the performance when tunnel
> gso is enabled, probably a new vnet header format.
you mean improve, not recover as such
>
> Fixes: 56a06bd40fab ("virtio_net: enable gso over UDP tunnel support.")
> Cc: stable@...r.kernel.org
> Signed-off-by: Jason Wang <jasowang@...hat.com>
> ---
> drivers/net/virtio_net.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 31bd32bdecaf..5b851df749c0 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -441,6 +441,9 @@ struct virtnet_info {
> /* Packet virtio header size */
> u8 hdr_len;
>
> + /* header alignment */
> + size_t hdr_align;
> +
It makes no sense to have u8 for length but size_t for alignment,
and u8 would fit in a memory hole we have, anyway.
> /* Work struct for delayed refilling if we run low on memory. */
> struct delayed_work refill;
>
> @@ -3308,8 +3311,9 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb, bool orphan)
> pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
>
> can_push = vi->any_header_sg &&
> - !((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
> + !((unsigned long)skb->data & (vi->hdr_align - 1)) &&
So let me get it straight.
We use the alignment check to be able to cast to the correct type.
The issue is that alignment for the header changed.
virtio_net_hdr_v1 has 2 byte alignment, but:
struct virtio_net_hdr_v1_hash {
struct virtio_net_hdr_v1 hdr;
__le32 hash_value;
#define VIRTIO_NET_HASH_REPORT_NONE 0
#define VIRTIO_NET_HASH_REPORT_IPv4 1
#define VIRTIO_NET_HASH_REPORT_TCPv4 2
#define VIRTIO_NET_HASH_REPORT_UDPv4 3
#define VIRTIO_NET_HASH_REPORT_IPv6 4
#define VIRTIO_NET_HASH_REPORT_TCPv6 5
#define VIRTIO_NET_HASH_REPORT_UDPv6 6
#define VIRTIO_NET_HASH_REPORT_IPv6_EX 7
#define VIRTIO_NET_HASH_REPORT_TCPv6_EX 8
#define VIRTIO_NET_HASH_REPORT_UDPv6_EX 9
__le16 hash_report;
__le16 padding;
};
has 4 byte due to hash_value, and accordingly:
struct virtio_net_hdr_v1_hash_tunnel {
struct virtio_net_hdr_v1_hash hash_hdr;
__le16 outer_th_offset;
__le16 inner_nh_offset;
};
now is 4 byte aligned so everything is messed up:
net tends not to be 4 byte aligned.
> !skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
> +
> /* Even if we can, don't push here yet as this would skew
> * csum_start offset below. */
> if (can_push)
> @@ -6926,15 +6930,20 @@ static int virtnet_probe(struct virtio_device *vdev)
> }
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO) ||
> - virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO))
> + virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO)) {
> vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash_tunnel);
> - else if (vi->has_rss_hash_report)
> + vi->hdr_align = __alignof__(struct virtio_net_hdr_v1_hash_tunnel);
> + } else if (vi->has_rss_hash_report) {
> vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
> - else if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF) ||
> - virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> + vi->hdr_align = __alignof__(struct virtio_net_hdr_v1_hash);
> + } else if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF) ||
> + virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> vi->hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> - else
> + vi->hdr_align = __alignof__(struct virtio_net_hdr_mrg_rxbuf);
> + } else {
> vi->hdr_len = sizeof(struct virtio_net_hdr);
> + vi->hdr_align = __alignof__(struct virtio_net_hdr);
> + }
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM))
> vi->rx_tnl_csum = true;
So how about just fixing the root cause then?
Like this (untested, if you agree pls take over this):
---
virtio_net: fix alignment for virtio_net_hdr_v1_hash
changing alignment of header would mean it's no longer safe to cast a 2
byte aligned pointer between formats. Use two 16 bit fields to make it 2
byte aligned as previously.
Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
--
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 31bd32bdecaf..02ce5316f47d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2535,6 +2535,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
return NULL;
}
+static inline u16
+virtio_net_hash_value(const struct virtio_net_hdr_v1_hash *hdr_hash)
+{
+ return __le16_to_cpu(hdr_hash->hash_value_lo) |
+ (__le16_to_cpu(hdr_hash->hash_value_hi) << 16);
+}
+
static void virtio_skb_set_hash(const struct virtio_net_hdr_v1_hash *hdr_hash,
struct sk_buff *skb)
{
@@ -2561,7 +2568,7 @@ static void virtio_skb_set_hash(const struct virtio_net_hdr_v1_hash *hdr_hash,
default:
rss_hash_type = PKT_HASH_TYPE_NONE;
}
- skb_set_hash(skb, __le32_to_cpu(hdr_hash->hash_value), rss_hash_type);
+ skb_set_hash(skb, virtio_net_hash_value(hdr_hash), rss_hash_type);
}
static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *rq,
@@ -3307,6 +3314,10 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb, bool orphan)
pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
+ /* Make sure it's safe to cast between formats */
+ BUILD_BUG_ON(__alignof__(*hdr) != __alignof__(hdr->hash_hdr));
+ BUILD_BUG_ON(__alignof__(*hdr) != __alignof__(hdr->hash_hdr.hdr));
+
can_push = vi->any_header_sg &&
!((unsigned long)skb->data & (__alignof__(*hdr) - 1)) &&
!skb_header_cloned(skb) && skb_headroom(skb) >= hdr_len;
@@ -6755,7 +6766,7 @@ static int virtnet_xdp_rx_hash(const struct xdp_md *_ctx, u32 *hash,
hash_report = VIRTIO_NET_HASH_REPORT_NONE;
*rss_type = virtnet_xdp_rss_type[hash_report];
- *hash = __le32_to_cpu(hdr_hash->hash_value);
+ *hash = virtio_net_hash_value(hdr_hash);
return 0;
}
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 8bf27ab8bcb4..1db45b01532b 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -193,7 +193,8 @@ struct virtio_net_hdr_v1 {
struct virtio_net_hdr_v1_hash {
struct virtio_net_hdr_v1 hdr;
- __le32 hash_value;
+ __le16 hash_value_lo;
+ __le16 hash_value_hi;
#define VIRTIO_NET_HASH_REPORT_NONE 0
#define VIRTIO_NET_HASH_REPORT_IPv4 1
#define VIRTIO_NET_HASH_REPORT_TCPv4 2
Powered by blists - more mailing lists