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 0FED645AA7 for ; Fri, 4 Oct 2024 03:05:24 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0303340670; Fri, 4 Oct 2024 03:05:24 +0200 (CEST) Received: from mail-pl1-f201.google.com (mail-pl1-f201.google.com [209.85.214.201]) by mails.dpdk.org (Postfix) with ESMTP id 8D2484060F for ; Fri, 4 Oct 2024 03:05:21 +0200 (CEST) Received: by mail-pl1-f201.google.com with SMTP id d9443c01a7336-20b0c92182fso14885355ad.1 for ; Thu, 03 Oct 2024 18:05:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1728003920; x=1728608720; darn=dpdk.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=hDTFuTJocL6DwDHszm7rsmbWpWdRXkVMdu6HJXEBFsc=; b=fHqDpjMiT/nQrbQsnvBwEgkmxbmcF3JzYV43pkK6WeWQnKsbthKKoq/TRcPKyN23IN 37+lQoTqIE4+tuoX6BL/cJNyJPmjt607mJVefU4tAxSs7xBSOuvXBMHHu3YVPj31fV4P CCMWmOMC2QDTBxLTYpNrKbxy7+kqDks1wmJ8gdJDvS55qrHxzTiLD05EPJq2IVQeUdpD UwDy2TEw+BTFFVPCvMnJ07Ggk5EeuMLcSIltngrYM6LAX9el4Ujzh0hIOcS0ENhPof3u Bwpwd93VH+QLfSfZllEmsL9kABnjvaoxjLtknp5iJ19hhkCgpdaiji9bQLF7Mg5yEvLN aOyQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728003920; x=1728608720; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=hDTFuTJocL6DwDHszm7rsmbWpWdRXkVMdu6HJXEBFsc=; b=RocIv5AVr8vxoe4v9lYheDR/OV3Vj6PxEERWFWgZoehQLDtQKi72ia7S/bdQKl2fwX ohX3GF4dL89DJIHlykzaGUj0eUzFjOCWPOuDkM3q5HGEdkK0BtTHoMVsmv65kjHbvyWr jbY9YjS9RKFebb15La2T8t9T0WjU2HN3XIPKHv5HeWStLrZRZxRoHfp3Wo06xkPY+ceq qNGiq+srqh3WxBPwNBSTSdNO878/MIoGdlxSh+SVcYfAqHLDznE4LYWUaty2ZoEhRsW6 UAWSgk9KfzO5KNWreZiNlDINg6ti0rS1cWlRi7N4JDzgCI5VKt7gE97ynxZDAkMWjw1p EcIQ== X-Forwarded-Encrypted: i=1; AJvYcCUhv+YLUReYcEwkWMt7rHGACCJhxzmZJ+0RFXgZektJG7ImQzUAIL8AHOkEOgtqtBu3BFkNhpg=@dpdk.org X-Gm-Message-State: AOJu0YwBO23/Umoc871w2HFfDAZAiFhqNIoJoLVDoVghlMOEnqIzNpyN aQOqy0uU8NkXCPkut9Q9an40Q4UZmc74AsyATvW0bY2Ckybx+EqhAoYCKJT1v0l+UnbDjvp29a/ AfBo3vmUhyw== X-Google-Smtp-Source: AGHT+IEujt8HHVGyc3TDqYMM6Ht8WwsJxXDWkbHzgmnMUefgWPHZ+H5LEl+UG2sln3nio7n0g0wARuUHI0onYA== X-Received: from joshwash.sea.corp.google.com ([2620:15c:11c:202:c808:ef77:413b:184d]) (user=joshwash job=sendgmr) by 2002:a17:902:e74a:b0:20b:4b70:f2d7 with SMTP id d9443c01a7336-20bfe01e577mr13175ad.2.1728003920174; Thu, 03 Oct 2024 18:05:20 -0700 (PDT) Date: Thu, 3 Oct 2024 18:05:18 -0700 Mime-Version: 1.0 X-Mailer: git-send-email 2.47.0.rc0.187.ge670bccf7e-goog Message-ID: <20241004010518.238331-1-joshwash@google.com> Subject: [PATCH] net/gve: fix refill logic causing memory corruption From: Joshua Washington To: Jeroen de Borst , Rushil Gupta , Joshua Washington , Junfeng Guo Cc: dev@dpdk.org, stable@dpdk.org, Ferruh Yigit , Praveen Kaligineedi Content-Type: text/plain; charset="UTF-8" X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org There is a seemingly mundane error in the RX refill path which can lead to major issues and ultimately program crashing. This error occurs as part of an edge case where the exact number of buffers the refill causes the ring to wrap around to 0. The current refill logic is split into two conditions: first, when the number of buffers to refill is greater than the number of buffers left in the ring before wraparound occurs; second, when the opposite is true, and there are enough buffers before wraparound to refill all buffers. In this edge case, the first condition erroneously uses a (<) condition to decide whether to wrap around, when it should have been (<=). In that case, the second condition would run and the tail pointer would be set to an invalid value (RING_SIZE). This causes a number of cascading failures. 1. The first issue rather mundane in that rxq->bufq_tail == RING_SIZE at the end of the refill, this will correct itself on the next refill without any sort of memory leak or courrption; 2. The second failure is that the head pointer would end up overrunning the tail because the last buffer that is refilled is refilled at sw_ring[RING_SIZE] instead of sw_ring[0]. This would cause the driver to give the application a stale mbuf, one that has been potentially freed or is otherwise stale; 3. The third failure comes from the fact that the software ring is being overrun. Because we directly use the sw_ring pointer to refill buffers, when sw_ring[RING_SIZE] is filled, a buffer overflow occurs. The overwritten data has the potential to be important data, and this can potentially cause the program to crash outright. This patch fixes the refill bug while greatly simplifying the logic so that it is much less error-prone. Fixes: 45da16b5b181 ("net/gve: support basic Rx data path for DQO") Cc: junfeng.guo@intel.com Cc: stable@dpdk.org Signed-off-by: Joshua Washington Reviewed-by: Rushil Gupta Reviewed-by: Praveen Kaligineedi --- drivers/net/gve/gve_rx_dqo.c | 62 ++++++++++-------------------------- 1 file changed, 16 insertions(+), 46 deletions(-) diff --git a/drivers/net/gve/gve_rx_dqo.c b/drivers/net/gve/gve_rx_dqo.c index e4084bc0dd..5371bab77d 100644 --- a/drivers/net/gve/gve_rx_dqo.c +++ b/drivers/net/gve/gve_rx_dqo.c @@ -11,66 +11,36 @@ static inline void gve_rx_refill_dqo(struct gve_rx_queue *rxq) { - volatile struct gve_rx_desc_dqo *rx_buf_ring; volatile struct gve_rx_desc_dqo *rx_buf_desc; struct rte_mbuf *nmb[rxq->nb_rx_hold]; uint16_t nb_refill = rxq->nb_rx_hold; - uint16_t nb_desc = rxq->nb_rx_desc; uint16_t next_avail = rxq->bufq_tail; struct rte_eth_dev *dev; uint64_t dma_addr; - uint16_t delta; int i; if (rxq->nb_rx_hold < rxq->free_thresh) return; - rx_buf_ring = rxq->rx_ring; - delta = nb_desc - next_avail; - if (unlikely(delta < nb_refill)) { - if (likely(rte_pktmbuf_alloc_bulk(rxq->mpool, nmb, delta) == 0)) { - for (i = 0; i < delta; i++) { - rx_buf_desc = &rx_buf_ring[next_avail + i]; - rxq->sw_ring[next_avail + i] = nmb[i]; - dma_addr = rte_cpu_to_le_64(rte_mbuf_data_iova_default(nmb[i])); - rx_buf_desc->header_buf_addr = 0; - rx_buf_desc->buf_addr = dma_addr; - } - nb_refill -= delta; - next_avail = 0; - rxq->nb_rx_hold -= delta; - } else { - rxq->stats.no_mbufs_bulk++; - rxq->stats.no_mbufs += nb_desc - next_avail; - dev = &rte_eth_devices[rxq->port_id]; - dev->data->rx_mbuf_alloc_failed += nb_desc - next_avail; - PMD_DRV_LOG(DEBUG, "RX mbuf alloc failed port_id=%u queue_id=%u", - rxq->port_id, rxq->queue_id); - return; - } + if (unlikely(rte_pktmbuf_alloc_bulk(rxq->mpool, nmb, nb_refill))) { + rxq->stats.no_mbufs_bulk++; + rxq->stats.no_mbufs += nb_refill; + dev = &rte_eth_devices[rxq->port_id]; + dev->data->rx_mbuf_alloc_failed += nb_refill; + PMD_DRV_LOG(DEBUG, "RX mbuf alloc failed port_id=%u queue_id=%u", + rxq->port_id, rxq->queue_id); + return; } - if (nb_desc - next_avail >= nb_refill) { - if (likely(rte_pktmbuf_alloc_bulk(rxq->mpool, nmb, nb_refill) == 0)) { - for (i = 0; i < nb_refill; i++) { - rx_buf_desc = &rx_buf_ring[next_avail + i]; - rxq->sw_ring[next_avail + i] = nmb[i]; - dma_addr = rte_cpu_to_le_64(rte_mbuf_data_iova_default(nmb[i])); - rx_buf_desc->header_buf_addr = 0; - rx_buf_desc->buf_addr = dma_addr; - } - next_avail += nb_refill; - rxq->nb_rx_hold -= nb_refill; - } else { - rxq->stats.no_mbufs_bulk++; - rxq->stats.no_mbufs += nb_desc - next_avail; - dev = &rte_eth_devices[rxq->port_id]; - dev->data->rx_mbuf_alloc_failed += nb_desc - next_avail; - PMD_DRV_LOG(DEBUG, "RX mbuf alloc failed port_id=%u queue_id=%u", - rxq->port_id, rxq->queue_id); - } + for (i = 0; i < nb_refill; i++) { + rx_buf_desc = &rxq->rx_ring[next_avail]; + rxq->sw_ring[next_avail] = nmb[i]; + dma_addr = rte_cpu_to_le_64(rte_mbuf_data_iova_default(nmb[i])); + rx_buf_desc->header_buf_addr = 0; + rx_buf_desc->buf_addr = dma_addr; + next_avail = (next_avail + 1) & (rxq->nb_rx_desc - 1); } - + rxq->nb_rx_hold -= nb_refill; rte_write32(next_avail, rxq->qrx_tail); rxq->bufq_tail = next_avail; -- 2.47.0.rc0.187.ge670bccf7e-goog