From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id F091245EFC; Fri, 20 Dec 2024 18:10:57 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1EDEC402D1; Fri, 20 Dec 2024 18:10:57 +0100 (CET) Received: from mail-pj1-f54.google.com (mail-pj1-f54.google.com [209.85.216.54]) by mails.dpdk.org (Postfix) with ESMTP id 807C440156 for ; Fri, 20 Dec 2024 18:10:56 +0100 (CET) Received: by mail-pj1-f54.google.com with SMTP id 98e67ed59e1d1-2ee709715d9so1585285a91.3 for ; Fri, 20 Dec 2024 09:10:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1734714655; x=1735319455; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=7MScFW2c5QP6IWx+N9sMvUlpFdl3pLCCaCAyB0LVHYc=; b=OaSdVW8oqUGq1KMhg2BJT0pouxq6s045ha5Ue+H5WOIkYnnjepliGRmRpUJ4ItcoIu mIjiN7kjrTlkYSN1JfkM/kmq1t5x/RFgqOHJSSivaYfgacxuU6imMiUCmDC8CkueJKgm 60mA7szjZSWXbdSCsY0wOPOM4VlSajcbroSTxBsV96JuCf/O4bzjlqnFfztBKRja5ylV fYWVYvPfmO0p4htFwsStitATD8SQ2P2T+2LkdtbFxY/shKLDxu6ma2TIDuxjspXTTcSz IcFSSo0zFjtg6jmtQx30OoeifNFhxSUcUJ+zdY1JPua99/wevpMIeAtP7/ircO7lzubv uIPA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734714655; x=1735319455; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=7MScFW2c5QP6IWx+N9sMvUlpFdl3pLCCaCAyB0LVHYc=; b=VPgnS54iPzaoTRWcP+T3MPG6JIt5uXIN/pjxzGeSn1NRF7uW+AkG2uGE4umECW9KYc NiH291teg+7Pkfe9vjaLH7209Ebs1Ndg9/SEGB+rHFobv2QW9Xm5WLQdvNFHQK/hB4qi FGV+10lHvxVHZ2cSdJ14PPskRf4RmreIbazuvMUsj5C2A0IlmD0uaAwn7d/bwiATus+S CxMZrRsEOLDtdm/InFKKHny93PGjxAJNPPRvDdHFMKemaWCpFviEmr4T7Nl5SwEo5eZO +hXA5hSqPn3QEEj+A7QhsYr3ReFnvnMj9qbsa8kGMSE363dvYoYvHHoLMBTKjGi3pVDV eFmw== X-Gm-Message-State: AOJu0YytHLAxWjpgOJ8NBqhbjyb4FpkY+eZqXF9lqUdJ0izlpzZBqEW4 DtzMSHAUgzB27UEo10RgWB+YC/fXWsMAzjYchCWChumPpG30SiKkcSkpzdadTGo= X-Gm-Gg: ASbGncu2xspUsqleEWl8MP46hyvqNMDd82u8xthwpxpCIWvdJ2SN+LgjmfEUOhnKv5w +Lm7IL81wEKDINAwlWpyMmkcV42A+5MzynmnTJbzrCNR/efSB/vfmg0tb4eYaMR9J6wE3ZF/8YL 8jUIvRjnZvze9HH/64KsTHN6PbeUdBHVOralJtmfIrtT3ZD5ylJVAr4sH7/8/OmvdQ0Za8iEbqc nHQ7Gi3JMjGn4L5caVdlrPu8tkmKnHN0U9wNjxS5kW7HI7NMfaATwd8xg2z4V6ofuVMqjCcdWar ztJSaS10NK77mGnAfUh15akWp9+mSHhASg== X-Google-Smtp-Source: AGHT+IHQCHnKKXMQ401Xwsk9IEdoalyWYUvB8RFGNJstKnlFz9RrvOiCNWfKvp1AHu5rmCmGrGtc4g== X-Received: by 2002:a17:90b:2dc8:b0:2ee:c797:e276 with SMTP id 98e67ed59e1d1-2f452d3000cmr6617787a91.0.1734714655375; Fri, 20 Dec 2024 09:10:55 -0800 (PST) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2f2ed62e385sm5482013a91.22.2024.12.20.09.10.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 20 Dec 2024 09:10:55 -0800 (PST) Date: Fri, 20 Dec 2024 09:10:52 -0800 From: Stephen Hemminger To: Yunjian Wang Cc: , , , , , , Subject: Re: [PATCH v2 1/1] vhost: fix a double fetch when dequeue offloading Message-ID: <20241220091052.68bb13ee@hermes.local> In-Reply-To: <09058cfb25d7583f67d74f09cd36673f1b10f5ec.1734661755.git.wangyunjian@huawei.com> References: <91dc12662805a3867413940f856ba9454b91c579.1734588243.git.wangyunjian@huawei.com> <09058cfb25d7583f67d74f09cd36673f1b10f5ec.1734661755.git.wangyunjian@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, 20 Dec 2024 11:49:55 +0800 Yunjian Wang wrote: > The hdr->csum_start does two successive reads from user space to read a > variable length data structure. The result overflow if the data structure > changes between the two reads. > > To fix this, we can prevent double fetch issue by copying virtio_hdr to > the temporary variable. > > Fixes: 4dc4e33ffa10 ("net/virtio: fix Rx checksum calculation") > Cc: stable@dpdk.org > > Signed-off-by: Yunjian Wang How about something like the following *untested* diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c index 69901ab3b5..c65cb639b2 100644 --- a/lib/vhost/virtio_net.c +++ b/lib/vhost/virtio_net.c @@ -2861,25 +2861,28 @@ vhost_dequeue_offload(struct virtio_net *dev, struct virtio_net_hdr *hdr, } } -static __rte_noinline void +static inline int copy_vnet_hdr_from_desc(struct virtio_net_hdr *hdr, - struct buf_vector *buf_vec) + const struct buf_vector *buf_vec, + uint16_t nr_vec) { - uint64_t len; - uint64_t remain = sizeof(struct virtio_net_hdr); - uint64_t src; - uint64_t dst = (uint64_t)(uintptr_t)hdr; + size_t remain = sizeof(struct virtio_net_hdr); + uint8_t *dst = (uint8_t *)hdr; - while (remain) { - len = RTE_MIN(remain, buf_vec->buf_len); - src = buf_vec->buf_addr; - rte_memcpy((void *)(uintptr_t)dst, - (void *)(uintptr_t)src, len); + while (remain > 0) { + size_t len = RTE_MIN(remain, buf_vec->buf_len); + const void *src = (const void *)(uintptr_t)buf_vec->buf_addr; + if (unlikely(nr_vec == 0)) + return -1; + + memcpy(dst, src, len); remain -= len; dst += len; buf_vec++; + --nr_vec; } + return 0; } static __rte_always_inline int @@ -2908,16 +2911,12 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq, */ if (virtio_net_with_host_offload(dev)) { - if (unlikely(buf_vec[0].buf_len < sizeof(struct virtio_net_hdr))) { - /* - * No luck, the virtio-net header doesn't fit - * in a contiguous virtual area. - */ - copy_vnet_hdr_from_desc(&tmp_hdr, buf_vec); - hdr = &tmp_hdr; - } else { - hdr = (struct virtio_net_hdr *)((uintptr_t)buf_vec[0].buf_addr); - } + if (unlikely(copy_vnet_hdr_from_desc(&tmp_hdr, buf_vec, nr_vec) != 0)) + return -1; + + /* ensure that compiler does not delay copy */ + rte_compiler_barrier(); + hdr = &tmp_hdr; } for (vec_idx = 0; vec_idx < nr_vec; vec_idx++) { @@ -3363,7 +3362,6 @@ virtio_dev_tx_batch_packed(struct virtio_net *dev, { uint16_t avail_idx = vq->last_avail_idx; uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); - struct virtio_net_hdr *hdr; uintptr_t desc_addrs[PACKED_BATCH_SIZE]; uint16_t ids[PACKED_BATCH_SIZE]; uint16_t i; @@ -3382,8 +3380,12 @@ virtio_dev_tx_batch_packed(struct virtio_net *dev, if (virtio_net_with_host_offload(dev)) { vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) { - hdr = (struct virtio_net_hdr *)(desc_addrs[i]); - vhost_dequeue_offload(dev, hdr, pkts[i], legacy_ol_flags); + struct virtio_net_hdr hdr; + + memcpy(&hdr, (void *)desc_addrs[i], sizeof(struct virtio_net_hdr)); + rte_compiler_barrier(); + + vhost_dequeue_offload(dev, &hdr, pkts[i], legacy_ol_flags); } }