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 305B3455AF; Sun, 7 Jul 2024 07:50:51 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 730B040614; Sun, 7 Jul 2024 07:50:50 +0200 (CEST) Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) by mails.dpdk.org (Postfix) with ESMTP id 862A44027F for ; Sun, 7 Jul 2024 07:50:48 +0200 (CEST) Received: by mail-wm1-f46.google.com with SMTP id 5b1f17b1804b1-4256742f67fso19962075e9.3 for ; Sat, 06 Jul 2024 22:50:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; t=1720331448; x=1720936248; darn=dpdk.org; h=from:in-reply-to:references:cc:to:subject:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=z2lz7KoSom81shfKsR7A2gLLf5aT6brmM9i7xmeXe3g=; b=Uj2Y0i50btjxStAgwwQ8vlceV7qpwC8u2CGhlWf62wvbXCHiZbMyxK/YlbPrcHeryE 9ncnBdunOh7182nwBZt5jyTsg7RxDqcdeOKzU+aqHVFyPckH421lLloMmS1Q8AMHRJlf Mxo5St7WfgZ/qkyedDWWQwleUUtOTqc8LnVh4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720331448; x=1720936248; h=from:in-reply-to:references:cc:to:subject:user-agent:mime-version :date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=z2lz7KoSom81shfKsR7A2gLLf5aT6brmM9i7xmeXe3g=; b=P1oav4KoIxpBQqQn7Njwx5Don99flWZjvfzuYQ3pg1wTBWipA3ZKU/QjnfcoDz590H 9424VAyB+ClIJqRq683hHGLnO6TrUqMZyIC6HhzWVBXsVXzJeEleGI1XvarNndnAt+CT VrPIiKdDG3Ta8FSzhhCsqq1+riuBqh+DVmH59PpWm1Pz/j2ewgimHQd+knzEpZUAG8X6 e+v82N6gLUtiMhrgG9rRTCnuf+ueJKJ6oiQL75clfinXQhhx9Kv6lVpouR3Si+QXzOZL sbjFaXyFzywBe5zVYBnRLGlgRvmA0/JoWUgXwvXK2MCyF7Y67dkQBuRzKazNMfCiB7GK JwQg== X-Gm-Message-State: AOJu0YzbQ+yaxUcmQR+aSUlS7vY2vUSi9cmaCABOgtPVloUj8u3zN/20 UIl2L/s9kLuV6oSUaNcdCkmCABMx6ixl06jdNTlsXfZNPG7D+3cUZ8SozLhtoMvWzQwHhVsH7WQ VTg+xnuSyplcXhgJ+EzJ/IqSohIoNpw== X-Google-Smtp-Source: AGHT+IHyYGWaDwdqtIeOH9txX+HgfyowwThjxC0xW6dK2xLVWO4Fn9a7hd7cXToQXsAdfZini5rB3A== X-Received: by 2002:a05:600c:3225:b0:426:5546:71a with SMTP id 5b1f17b1804b1-42655460881mr34208085e9.2.1720331448008; Sat, 06 Jul 2024 22:50:48 -0700 (PDT) Received: from [192.168.0.8] ([92.81.76.237]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4265c2a99c7sm34370705e9.1.2024.07.06.22.50.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 06 Jul 2024 22:50:47 -0700 (PDT) Message-ID: <8bf5e505-191b-46ea-8b90-0ed4fc15d306@broadcom.com> Date: Sun, 7 Jul 2024 08:50:45 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] net/memif: fix buffer overflow in zero copy Rx To: Ferruh Yigit , Jakub Grajciar Cc: dev@dpdk.org, stable@dpdk.org, Mihai Brodschi References: In-Reply-To: From: Mihai Brodschi Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha-256; boundary="000000000000e19638061ca1e074" 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 --000000000000e19638061ca1e074 Content-Language: en-US Content-Type: text/plain; charset="UTF-8" Hi Ferruh, On 07/07/2024 05:12, Ferruh Yigit wrote: > On 6/28/2024 10:01 PM, Mihai Brodschi wrote: >> rte_pktmbuf_alloc_bulk is called by the zero-copy receiver to allocate >> new mbufs to be provided to the sender. The allocated mbuf pointers >> are stored in a ring, but the alloc function doesn't implement index >> wrap-around, so it writes past the end of the array. This results in >> memory corruption and duplicate mbufs being received. >> > > Hi Mihai, > > I am not sure writing past the ring actually occurs. > > As far as I can see is to keep the ring full as much as possible, when > initially 'head' and 'tail' are 0, it fills all ring. > Later tails moves and emptied space filled again. So head (in modulo) is > always just behind tail after refill. In next run, refill will only fill > the part tail moved, and this is calculated by 'n_slots'. As this is > only the size of the gap, starting from 'head' (with modulo) shouldn't > pass the ring length. > > Do you observe this issue practically? If so can you please provide your > backtrace and numbers that is showing how to reproduce the issue? The alloc function writes starting from the ring's head, but the ring's head can be located at the end of the ring's memory buffer (ring_size - 1). The correct behavior would be to wrap around to the start of the buffer (0), but the alloc function has no awareness of the fact that it's writing to a ring, so it writes to ring_size, ring_size + 1, etc. Let's look at the existing code: We assume the ring size is 256 and we just received 32 packets. The previous tail was at index 255, now it's at index 31. The head is initially at index 255. head = __atomic_load_n(&ring->head, __ATOMIC_RELAXED); // head = 255 n_slots = ring_size - head + mq->last_tail; // n_slots = 32 if (n_slots < 32) // not taken goto no_free_mbufs; ret = rte_pktmbuf_alloc_bulk(mq->mempool, &mq->buffers[head & mask], n_slots); // This will write 32 mbuf pointers starting at index (head & mask) = 255. // The ring size is 256, so apart from the first one all pointers will be // written out of bounds (index 256 .. 286, when it should be 0 .. 30). I can reproduce a crash 100% of the time with my application, but the output is not very helpful, since it crashes elsewhere because of mempool corruption. Applying this patch fixes the crashes completely. >> Allocate 2x the space for the mbuf ring, so that the alloc function >> has a contiguous array to write to, then copy the excess entries >> to the start of the array. >> > > Even issue is valid, I am not sure about solution to double to buffer > memory, but lets confirm the issue first before discussing the solution. Initially, I thought about splitting the call to rte_pktmbuf_alloc_bulk in two, but I thought that might be bad for performance if the mempool is being used concurrently from multiple threads. If we want to use only one call to rte_pktmbuf_alloc_bulk, we need an array to store the allocated mbuf pointers. This array must be of length ring_size, since that's the maximum amount of mbufs which may be allocated in one go. We need to copy the pointers from this array to the ring. If we instead allocate twice the space for the ring, we can skip copying the pointers which were written to the ring, and only copy those that were written outside of its bounds. >> Fixes: 43b815d88188 ("net/memif: support zero-copy slave") >> Cc: stable@dpdk.org >> Signed-off-by: Mihai Brodschi >> --- >> v2: >> - fix email formatting >> >> --- >> drivers/net/memif/rte_eth_memif.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c >> index 16da22b5c6..3491c53cf1 100644 >> --- a/drivers/net/memif/rte_eth_memif.c >> +++ b/drivers/net/memif/rte_eth_memif.c >> @@ -600,6 +600,10 @@ eth_memif_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts) >> ret = rte_pktmbuf_alloc_bulk(mq->mempool, &mq->buffers[head & mask], n_slots); >> if (unlikely(ret < 0)) >> goto no_free_mbufs; >> + if (unlikely(n_slots > ring_size - (head & mask))) { >> + rte_memcpy(mq->buffers, &mq->buffers[ring_size], >> + (n_slots + (head & mask) - ring_size) * sizeof(struct rte_mbuf *)); >> + } >> >> while (n_slots--) { >> s0 = head++ & mask; >> @@ -1245,8 +1249,12 @@ memif_init_queues(struct rte_eth_dev *dev) >> } >> mq->buffers = NULL; >> if (pmd->flags & ETH_MEMIF_FLAG_ZERO_COPY) { >> + /* >> + * Allocate 2x ring_size to reserve a contiguous array for >> + * rte_pktmbuf_alloc_bulk (to store allocated mbufs). >> + */ >> mq->buffers = rte_zmalloc("bufs", sizeof(struct rte_mbuf *) * >> - (1 << mq->log2_ring_size), 0); >> + (1 << (mq->log2_ring_size + 1)), 0); >> if (mq->buffers == NULL) >> return -ENOMEM; >> } > Apologies for sending this multiple times, I'm not familiar with mailing lists. -- This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it. --000000000000e19638061ca1e074 Content-Type: application/pkcs7-signature; name="smime.p7s" Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="smime.p7s" Content-Description: S/MIME Cryptographic Signature MIIQcwYJKoZIhvcNAQcCoIIQZDCCEGACAQExDzANBglghkgBZQMEAgEFADALBgkqhkiG9w0BBwGg gg3KMIIFDTCCA/WgAwIBAgIQeEqpED+lv77edQixNJMdADANBgkqhkiG9w0BAQsFADBMMSAwHgYD VQQLExdHbG9iYWxTaWduIFJvb3QgQ0EgLSBSMzETMBEGA1UEChMKR2xvYmFsU2lnbjETMBEGA1UE AxMKR2xvYmFsU2lnbjAeFw0yMDA5MTYwMDAwMDBaFw0yODA5MTYwMDAwMDBaMFsxCzAJBgNVBAYT AkJFMRkwFwYDVQQKExBHbG9iYWxTaWduIG52LXNhMTEwLwYDVQQDEyhHbG9iYWxTaWduIEdDQyBS MyBQZXJzb25hbFNpZ24gMiBDQSAyMDIwMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA vbCmXCcsbZ/a0fRIQMBxp4gJnnyeneFYpEtNydrZZ+GeKSMdHiDgXD1UnRSIudKo+moQ6YlCOu4t rVWO/EiXfYnK7zeop26ry1RpKtogB7/O115zultAz64ydQYLe+a1e/czkALg3sgTcOOcFZTXk38e aqsXsipoX1vsNurqPtnC27TWsA7pk4uKXscFjkeUE8JZu9BDKaswZygxBOPBQBwrA5+20Wxlk6k1 e6EKaaNaNZUy30q3ArEf30ZDpXyfCtiXnupjSK8WU2cK4qsEtj09JS4+mhi0CTCrCnXAzum3tgcH cHRg0prcSzzEUDQWoFxyuqwiwhHu3sPQNmFOMwIDAQABo4IB2jCCAdYwDgYDVR0PAQH/BAQDAgGG MGAGA1UdJQRZMFcGCCsGAQUFBwMCBggrBgEFBQcDBAYKKwYBBAGCNxQCAgYKKwYBBAGCNwoDBAYJ KwYBBAGCNxUGBgorBgEEAYI3CgMMBggrBgEFBQcDBwYIKwYBBQUHAxEwEgYDVR0TAQH/BAgwBgEB /wIBADAdBgNVHQ4EFgQUljPR5lgXWzR1ioFWZNW+SN6hj88wHwYDVR0jBBgwFoAUj/BLf6guRSSu TVD6Y5qL3uLdG7wwegYIKwYBBQUHAQEEbjBsMC0GCCsGAQUFBzABhiFodHRwOi8vb2NzcC5nbG9i YWxzaWduLmNvbS9yb290cjMwOwYIKwYBBQUHMAKGL2h0dHA6Ly9zZWN1cmUuZ2xvYmFsc2lnbi5j b20vY2FjZXJ0L3Jvb3QtcjMuY3J0MDYGA1UdHwQvMC0wK6ApoCeGJWh0dHA6Ly9jcmwuZ2xvYmFs c2lnbi5jb20vcm9vdC1yMy5jcmwwWgYDVR0gBFMwUTALBgkrBgEEAaAyASgwQgYKKwYBBAGgMgEo CjA0MDIGCCsGAQUFBwIBFiZodHRwczovL3d3dy5nbG9iYWxzaWduLmNvbS9yZXBvc2l0b3J5LzAN BgkqhkiG9w0BAQsFAAOCAQEAdAXk/XCnDeAOd9nNEUvWPxblOQ/5o/q6OIeTYvoEvUUi2qHUOtbf jBGdTptFsXXe4RgjVF9b6DuizgYfy+cILmvi5hfk3Iq8MAZsgtW+A/otQsJvK2wRatLE61RbzkX8 9/OXEZ1zT7t/q2RiJqzpvV8NChxIj+P7WTtepPm9AIj0Keue+gS2qvzAZAY34ZZeRHgA7g5O4TPJ /oTd+4rgiU++wLDlcZYd/slFkaT3xg4qWDepEMjT4T1qFOQIL+ijUArYS4owpPg9NISTKa1qqKWJ jFoyms0d0GwOniIIbBvhI2MJ7BSY9MYtWVT5jJO3tsVHwj4cp92CSFuGwunFMzCCA18wggJHoAMC AQICCwQAAAAAASFYUwiiMA0GCSqGSIb3DQEBCwUAMEwxIDAeBgNVBAsTF0dsb2JhbFNpZ24gUm9v dCBDQSAtIFIzMRMwEQYDVQQKEwpHbG9iYWxTaWduMRMwEQYDVQQDEwpHbG9iYWxTaWduMB4XDTA5 MDMxODEwMDAwMFoXDTI5MDMxODEwMDAwMFowTDEgMB4GA1UECxMXR2xvYmFsU2lnbiBSb290IENB IC0gUjMxEzARBgNVBAoTCkdsb2JhbFNpZ24xEzARBgNVBAMTCkdsb2JhbFNpZ24wggEiMA0GCSqG SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDMJXaQeQZ4Ihb1wIO2hMoonv0FdhHFrYhy/EYCQ8eyip0E XyTLLkvhYIJG4VKrDIFHcGzdZNHr9SyjD4I9DCuul9e2FIYQebs7E4B3jAjhSdJqYi8fXvqWaN+J J5U4nwbXPsnLJlkNc96wyOkmDoMVxu9bi9IEYMpJpij2aTv2y8gokeWdimFXN6x0FNx04Druci8u nPvQu7/1PQDhBjPogiuuU6Y6FnOM3UEOIDrAtKeh6bJPkC4yYOlXy7kEkmho5TgmYHWyn3f/kRTv riBJ/K1AFUjRAjFhGV64l++td7dkmnq/X8ET75ti+w1s4FRpFqkD2m7pg5NxdsZphYIXAgMBAAGj QjBAMA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSP8Et/qC5FJK5N UPpjmove4t0bvDANBgkqhkiG9w0BAQsFAAOCAQEAS0DbwFCq/sgM7/eWVEVJu5YACUGssxOGhigH M8pr5nS5ugAtrqQK0/Xx8Q+Kv3NnSoPHRHt44K9ubG8DKY4zOUXDjuS5V2yq/BKW7FPGLeQkbLmU Y/vcU2hnVj6DuM81IcPJaP7O2sJTqsyQiunwXUaMld16WCgaLx3ezQA3QY/tRG3XUyiXfvNnBB4V 14qWtNPeTCekTBtzc3b0F5nCH3oO4y0IrQocLP88q1UOD5F+NuvDV0m+4S4tfGCLw0FREyOdzvcy a5QBqJnnLDMfOjsl0oZAzjsshnjJYS8Uuu7bVW/fhO4FCU29KNhyztNiUGUe65KXgzHZs7XKR1g/ XzCCBVIwggQ6oAMCAQICDHbaeqlxkxwG0oD4oTANBgkqhkiG9w0BAQsFADBbMQswCQYDVQQGEwJC RTEZMBcGA1UEChMQR2xvYmFsU2lnbiBudi1zYTExMC8GA1UEAxMoR2xvYmFsU2lnbiBHQ0MgUjMg UGVyc29uYWxTaWduIDIgQ0EgMjAyMDAeFw0yMjExMTQxMTQ3MjRaFw0yNTExMTQxMTQ3MjRaMIGS MQswCQYDVQQGEwJJTjESMBAGA1UECBMJS2FybmF0YWthMRIwEAYDVQQHEwlCYW5nYWxvcmUxFjAU BgNVBAoTDUJyb2FkY29tIEluYy4xFzAVBgNVBAMTDk1paGFpIEJyb2RzY2hpMSowKAYJKoZIhvcN AQkBFhttaWhhaS5icm9kc2NoaUBicm9hZGNvbS5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAw ggEKAoIBAQDKeSQ6fd3ArZpB+9ObkhCvLHNKaI4Zarn0m98M/IZYwHIXVxxLVn0g9I8RbzaUa6GZ k6TzMA22mdd6Sy/mnwJHOy7pNVd/2MBVwIkhNYL+5CwdBjBanvOOLh9FBl8QzKhifV7xYDMWJQJD Mr+QIRdtZOKkm9i0sRs9bwF2Rxbvnxj2EwgBSPe4FVpHEx4Is25hBIOZcEIvZTVoZgisovq6vB5I ERa8kmgfcp8zNafingkraXyOhds+xUiXbrZOthVlXg3ijylyQ50+iCWICS3qWXOw1tJXqTZUGgB/ PmiSLVSsz9RLsdo8tAV035w8AbZbKyFKl7mQzcIIE/9Zbk/PAgMBAAGjggHcMIIB2DAOBgNVHQ8B Af8EBAMCBaAwgaMGCCsGAQUFBwEBBIGWMIGTME4GCCsGAQUFBzAChkJodHRwOi8vc2VjdXJlLmds b2JhbHNpZ24uY29tL2NhY2VydC9nc2djY3IzcGVyc29uYWxzaWduMmNhMjAyMC5jcnQwQQYIKwYB BQUHMAGGNWh0dHA6Ly9vY3NwLmdsb2JhbHNpZ24uY29tL2dzZ2NjcjNwZXJzb25hbHNpZ24yY2Ey MDIwME0GA1UdIARGMEQwQgYKKwYBBAGgMgEoCjA0MDIGCCsGAQUFBwIBFiZodHRwczovL3d3dy5n bG9iYWxzaWduLmNvbS9yZXBvc2l0b3J5LzAJBgNVHRMEAjAAMEkGA1UdHwRCMEAwPqA8oDqGOGh0 dHA6Ly9jcmwuZ2xvYmFsc2lnbi5jb20vZ3NnY2NyM3BlcnNvbmFsc2lnbjJjYTIwMjAuY3JsMCYG A1UdEQQfMB2BG21paGFpLmJyb2RzY2hpQGJyb2FkY29tLmNvbTATBgNVHSUEDDAKBggrBgEFBQcD BDAfBgNVHSMEGDAWgBSWM9HmWBdbNHWKgVZk1b5I3qGPzzAdBgNVHQ4EFgQUTKjubK5dUstAoG+s gC9E5CNgobQwDQYJKoZIhvcNAQELBQADggEBADk/H+GmVd7WyerJTClll6xJOZorGnuKIVwthtoZ sVIrdxY2sspHYC0cmnRDxpw5/18UBLwjjIgPbv2PwJMPiiS4BG5r9ykQLpsSfbBzSiaUKkEX7jdH 5ONn8aGl4W0jcGJEKHK0KHziK1SJYWRExzSFfdTwFLTEj/g3yVZQT+mB+zv8NMRAmdG8DJ4waVPi L+E3ld0mdxuSCcvvAzi7ZNBrkCWUuC/YaiMtIRuyDqYnppUEkIXHE+SMfA+dirfXGmIYfk16DAOk rnI0rl6IAv30qz/Du0BDNsHi3gsTsQMfrA5M0saDCy65Bina2ExB2ZK6YyuajQd6BDtsygsH2Uwx ggJtMIICaQIBATBrMFsxCzAJBgNVBAYTAkJFMRkwFwYDVQQKExBHbG9iYWxTaWduIG52LXNhMTEw LwYDVQQDEyhHbG9iYWxTaWduIEdDQyBSMyBQZXJzb25hbFNpZ24gMiBDQSAyMDIwAgx22nqpcZMc BtKA+KEwDQYJYIZIAWUDBAIBBQCggdQwLwYJKoZIhvcNAQkEMSIEIOs3X2cuGYcYfZ2IC5pFiuHo d9qnDSQFBIqZTDsHSpL+MBgGCSqGSIb3DQEJAzELBgkqhkiG9w0BBwEwHAYJKoZIhvcNAQkFMQ8X DTI0MDcwNzA1NTA0OFowaQYJKoZIhvcNAQkPMVwwWjALBglghkgBZQMEASowCwYJYIZIAWUDBAEW MAsGCWCGSAFlAwQBAjAKBggqhkiG9w0DBzALBgkqhkiG9w0BAQowCwYJKoZIhvcNAQEHMAsGCWCG SAFlAwQCATANBgkqhkiG9w0BAQEFAASCAQCkp3GtmTprJCYvhLLJZn3kqYEAFLMPmRqU9WMqppCZ 0B7O+En06Vv9KGECb0hglE9oh60RAVT8fiLVQvln5DBJeYGx69JyHvS1cbiDzi1N9uovKR/FJG4V 3vrtcphCQg4mBlN1BLR24Bb/FUElSUHcws2ICWC8NshDuGjHylZnvjo4SBrvclIJaYg7bIiNEpot GsTXLdMQipa/UN4pqdArmvK+OTSHjAA2q98vhxhGCKdtxSfT2LMiwX0PZ9eW1FzASyhyuPglinBq b6wz1ywSk8tRqZ9uR72GltGL/0sccg/1GR7AB+YP27ZfzhlQJWk9prx0J28wlBzLVL0LFm2B --000000000000e19638061ca1e074--