| 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: <CACGkMEu9AMtTn6cZjW=N+OJZMLqgaiLiAS6V0EhuA7fSeXJ69A@mail.gmail.com>
Date: Wed, 29 Oct 2025 09:05:47 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...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 10:39 PM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> 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
The PPS drops to 2.4Mpps after the 56a06bd40fab, so this patch
recovers it the number before 56a06bd40fab.
>
>
> >
> > 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.
Oh right.
>
> > /* 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>
This looks indeed better, will go this way.
>
> --
>
> 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
Should be u32.
> +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
>
Thanks
Powered by blists - more mailing lists