| 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: <CACGkMEvTvYsECf8MOtTd7c1-YskUP-3rbec=qHTUuDgNLjPs6w@mail.gmail.com>
Date: Mon, 16 Jun 2025 11:20:41 +0800
From: Jason Wang <jasowang@...hat.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, Willem de Bruijn <willemdebruijn.kernel@...il.com>,
Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
"Michael S. Tsirkin" <mst@...hat.com>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
Eugenio Pérez <eperezma@...hat.com>,
Yuri Benditovich <yuri.benditovich@...nix.com>, Akihiko Odaki <akihiko.odaki@...nix.com>
Subject: Re: [PATCH RFC v3 6/8] virtio_net: enable gso over UDP tunnel support.
On Thu, Jun 12, 2025 at 6:18 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 6/12/25 6:05 AM, Jason Wang wrote:
> > On Fri, Jun 6, 2025 at 7:46 PM Paolo Abeni <pabeni@...hat.com> wrote:
> >>
> >> If the related virtio feature is set, enable transmission and reception
> >> of gso over UDP tunnel packets.
> >>
> >> Most of the work is done by the previously introduced helper, just need
> >> to determine the UDP tunnel features inside the virtio_net_hdr and
> >> update accordingly the virtio net hdr size.
> >>
> >> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> >> ---
> >> v2 -> v3:
> >> - drop the VIRTIO_HAS_EXTENDED_FEATURES conditionals
> >>
> >> v1 -> v2:
> >> - test for UDP_TUNNEL_GSO* only on builds with extended features support
> >> - comment indentation cleanup
> >> - rebased on top of virtio helpers changes
> >> - dump more information in case of bad offloads
> >> ---
> >> drivers/net/virtio_net.c | 70 +++++++++++++++++++++++++++++++---------
> >> 1 file changed, 54 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 18ad50de4928..0b234f318e39 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -78,15 +78,19 @@ static const unsigned long guest_offloads[] = {
> >> VIRTIO_NET_F_GUEST_CSUM,
> >> VIRTIO_NET_F_GUEST_USO4,
> >> VIRTIO_NET_F_GUEST_USO6,
> >> - VIRTIO_NET_F_GUEST_HDRLEN
> >> + VIRTIO_NET_F_GUEST_HDRLEN,
> >> + VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_MAPPED,
> >> + VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM_MAPPED,
> >> };
> >>
> >> #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> >> - (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> >> - (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> >> - (1ULL << VIRTIO_NET_F_GUEST_UFO) | \
> >> - (1ULL << VIRTIO_NET_F_GUEST_USO4) | \
> >> - (1ULL << VIRTIO_NET_F_GUEST_USO6))
> >> + (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
> >> + (1ULL << VIRTIO_NET_F_GUEST_ECN) | \
> >> + (1ULL << VIRTIO_NET_F_GUEST_UFO) | \
> >> + (1ULL << VIRTIO_NET_F_GUEST_USO4) | \
> >> + (1ULL << VIRTIO_NET_F_GUEST_USO6) | \
> >> + (1ULL << VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_MAPPED) | \
> >> + (1ULL << VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM_MAPPED))
> >>
> >> struct virtnet_stat_desc {
> >> char desc[ETH_GSTRING_LEN];
> >> @@ -436,9 +440,14 @@ struct virtnet_info {
> >> /* Packet virtio header size */
> >> u8 hdr_len;
> >>
> >> + /* UDP tunnel support */
> >> + u8 tnl_offset;
> >> +
> >> /* Work struct for delayed refilling if we run low on memory. */
> >> struct delayed_work refill;
> >>
> >> + bool rx_tnl_csum;
> >> +
> >> /* Is delayed refill enabled? */
> >> bool refill_enabled;
> >>
> >> @@ -2531,14 +2540,22 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
> >> if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> >> virtio_skb_set_hash(&hdr->hash_v1_hdr, skb);
> >>
> >> - if (flags & VIRTIO_NET_HDR_F_DATA_VALID)
> >> - skb->ip_summed = CHECKSUM_UNNECESSARY;
> >> + /* restore the received value */
> >
> > Nit: this comment seems to be redundant
> >
> >> + hdr->hdr.flags = flags;
> >> + if (virtio_net_chk_data_valid(skb, &hdr->hdr, vi->rx_tnl_csum)) {
> >
> > Nit: this function did more than just check DATA_VALID, we probably
> > need a better name.
>
> What about virtio_net_handle_csum_offload()?
Works for me.
>
> >
> >> + net_warn_ratelimited("%s: bad csum: flags: %x, gso_type: %x rx_tnl_csum %d\n",
> >> + dev->name, hdr->hdr.flags,
> >> + hdr->hdr.gso_type, vi->rx_tnl_csum);
> >> + goto frame_err;
> >> + }
> >>
> >> - if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> >> - virtio_is_little_endian(vi->vdev))) {
> >> - net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
> >> + if (virtio_net_hdr_tnl_to_skb(skb, &hdr->hdr, vi->tnl_offset,
> >
> > I wonder why virtio_net_chk_data_valid() is not part of the
> > virtio_net_hdr_tnl_to_skb().
>
> It can't be part of virtio_net_hdr_tnl_to_skb(), as hdr to skb
> conversion is actually not symmetric with respect to the checksum - only
> the driver handles DATA_VALID.
>
> Tun must not call virtio_net_chk_data_valid() (or whatever different
> name will use).
Note that we've already dealt with this via introducing a boolean
has_data_valid:
static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
struct virtio_net_hdr *hdr,
bool little_endian,
bool has_data_valid,
int vlan_hlen)
>
> /P
>
Thanks
Powered by blists - more mailing lists