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 6CE52A054D; Sun, 17 Jul 2022 23:59:43 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3E74C40687; Sun, 17 Jul 2022 23:59:43 +0200 (CEST) Received: from mail-ua1-f46.google.com (mail-ua1-f46.google.com [209.85.222.46]) by mails.dpdk.org (Postfix) with ESMTP id B12AD40141 for ; Sat, 16 Jul 2022 02:16:00 +0200 (CEST) Received: by mail-ua1-f46.google.com with SMTP id s3so3074236uaq.2 for ; Fri, 15 Jul 2022 17:16:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=senetas-com.20210112.gappssmtp.com; s=20210112; h=mime-version:from:date:message-id:subject:to; bh=Vng4v8qfLuz5S9KRv6m83piyelTey314um8nACvQa10=; b=5DmsrJTe/y0FOp/fD2vX65/VOAOFs2bZ6w1QukDUyxP0KADRpTzsLMnJRdPvwcvfLC UrDt+k2WLklpcYeP8SGP6xFC6QmzJ2rRZZSM6+9W2xz7vGu5NzP02LyyvbvHWw+Xvd/2 fQmXnZ25vGv31DI/vyCjyGy8B8jbFbYoTyoFjAq3AyZQKILgWrosCnphHf5p2SXFoYhX R/G39uqOaejBnVNpDvtCT4zNcY1fncMlfyDTnmXC8nNUvqCvOJb+Q8nnMXzdrrSXfVBG e05/bYVCpwJYlCdHtd3BJoQcPI1lHT15gKZGy8cebuDbRFlvFhHfc5kyguD77MNaQm/1 l74w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=Vng4v8qfLuz5S9KRv6m83piyelTey314um8nACvQa10=; b=Mc3zzpjkg+w70liJmSwxcBqUolc12QMAC6dt3ZS+wQnmoYQj+SE2NITsl0UFd01CmG YfqGw52K2JSo1wnsEjwPrheoGEg6fnJh8rtZZ1H9jZtfqZdpFrSYl6uWmYRZ5z2dX13R aoPMOYBAZTgJFUaveY/UJnsgcuJo2BMv7Gu6uNMV6GSEsu6vtsYOwvKhh9AE/GNvmS+K HSb4gR9e+yGWzOna6XIwCI64q7zPCfbE0jxa6QyF7fs2wXG9XLDa4jlgMw+CG8CTxSWJ S045U+CueH5GvdkecCSb5DjtPMikunvbCFtNSoeigyFeLozH73r+XmsMMeRXzHXZ51kx SVXg== X-Gm-Message-State: AJIora8jjYLDWFUV6nxQbclCC+s8zZYlb1/oefsEQXdEuQs48fV66G5e de3D2DZW/TAUTuCQSqk27g/JgG6FywElMt4pYpT+kCmgmzFu+Q== X-Google-Smtp-Source: AGRyM1vgAS1TvKJpT2rwXDxtyVTqBbGNNoG16Mp5gslLAyqIlceFGUllmtxedc+979SV/dkDb656RuyjvC+6Dz4AHyQ= X-Received: by 2002:a1f:7ccd:0:b0:374:542b:9bcf with SMTP id x196-20020a1f7ccd000000b00374542b9bcfmr6558364vkc.34.1657930559583; Fri, 15 Jul 2022 17:15:59 -0700 (PDT) MIME-Version: 1.0 From: John Weston Date: Sat, 16 Jul 2022 10:15:48 +1000 Message-ID: Subject: memif driver segfault memory corruption. To: dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000110f0205e3e10aa4" X-Mailman-Approved-At: Sun, 17 Jul 2022 23:59:41 +0200 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 --000000000000110f0205e3e10aa4 Content-Type: text/plain; charset="UTF-8" Hi, I have been working on the dpdk memif driver and encountered the following issues. Please consider these patches for review. Firstly, the zero copy code in drivers/net/memif/rte_eth_memif.c can readilly overflow the ring buffer for mbufs. This patch adjusts the n_slots used in rte_poktmbuf_alloc_bulk to ensure it does not run off the end of the buffers list. dev@dpdk.org@@ -524,14 +1196,23 @@ refill: */ head = __atomic_load_n(&ring->head, __ATOMIC_RELAXED); n_slots = ring_size - head + mq->last_tail; - - if (n_slots < 32) - goto no_free_mbufs; + // CTAM - critical BUG FIX ! +#if 1 + // only work within mask so alloc_bulk does not overrun the buffers array + if ((head&mask) + n_slots > ring_size) + { + n_slots = ring_size - (head&mask); + } + if (n_slots <=0) + goto no_free_mbufs; +#else + if (n_slots < 32) + goto no_free_mbufs; +#endif ret = rte_pktmbuf_alloc_bulk(mq->mempool, &mq->buffers[head & mask], n_slots); if (unlikely(ret < 0)) goto no_free_mbufs; - while (n_slots--) { s0 = head++ & mask; if (n_slots > 0) Secondly, a segfault can occur on the stats routine on initialisation due to null pointer dereferencing. @@ -1404,10 +2167,14 @@ memif_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) /* RX stats */ for (i = 0; i < nq; i++) { mq = dev->data->rx_queues[i]; - stats->q_ipackets[i] = mq->n_pkts; - stats->q_ibytes[i] = mq->n_bytes; - stats->ipackets += mq->n_pkts; - stats->ibytes += mq->n_bytes; + // CTAM test this, as it may not yet initialised! + if (mq) + { + stats->q_ipackets[i] = mq->n_pkts; + stats->q_ibytes[i] = mq->n_bytes; + stats->ipackets += mq->n_pkts; + stats->ibytes += mq->n_bytes; + } } tmp = (pmd->role == MEMIF_ROLE_CLIENT) ? pmd->run.num_s2c_rings : This can occur in several places in the stats code, and null dereference guards are needed in all locations. Hope this helps someone else. John --000000000000110f0205e3e10aa4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

I have been working on the dpdk mem= if driver and encountered the=C2=A0following issues. Please consider these = patches for review.

Firstly, the zero copy code in= drivers/net/memif/rte_eth_memif.c can readilly=C2=A0overflow the ring buff= er for mbufs. This=C2=A0patch adjusts the n_slots used in rte_poktmbuf_allo= c_bulk to ensure it does not run off the end of the buffers list.

dev@dpdk.org@@ -524,14 +1196,23 @@ refill:

=C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 */

=C2=A0 =C2=A0 =C2=A0 =C2=A0 head =3D __atomic_load_n(&a= mp;ring->head, __ATOMIC_RELAXED);

=C2=A0 =C2=A0 =C2=A0 =C2=A0 n_slots =3D ring_size - hea= d + mq->last_tail;

-

- =C2=A0 =C2=A0 =C2=A0 if (n_slots < 32)

- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto= no_free_mbufs;

+=C2=A0 =C2=A0 // CTAM - critical BUG FIX !

+#if 1

+=C2=A0 =C2=A0 // only work within mask so alloc_bulk d= oes not overrun the buffers array

+=C2=A0 =C2=A0 if ((head&mask) + n_slots > ring_= size)

+=C2=A0 =C2=A0 {

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 n_slots =3D ring_size - (h= ead&mask);

+=C2=A0 =C2=A0 }

+=C2=A0 =C2=A0 if (n_slots <=3D0)

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto no_free_mbufs;

+#else

+=C2=A0 =C2=A0 if (n_slots < 32)

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto no_free_mbufs;

+#endif


=C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D rte_pktmbuf_alloc_b= ulk(mq->mempool, &mq->buffers[head & mask], n_slots);<= /p>

=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (unlikely(ret < 0))

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto no_free_mbufs;

-

=C2=A0 =C2=A0 =C2=A0 =C2=A0 while (n_slots--) {<= /p>

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 s0 =3D head++ & mask;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (n_slots > 0)

=


<= p class=3D"gmail-p1" style=3D"margin:0px;font-variant-numeric:normal;font-v= ariant-east-asian:normal;font-stretch:normal;font-size:11px;line-height:nor= mal;font-family:Menlo;color:rgb(0,0,0)">

Secondly, a segfault can occur on the stats rout= ine on initialisation due to null pointer dereferencing.


=

@@ -1404,10 +2167,14 @@ memif_stats_get(struct rte_eth_dev *= dev, struct rte_eth_stats *stats)

=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* RX stats */

=C2=A0 =C2=A0 =C2=A0 =C2= =A0 for (i =3D 0; i < nq; i++) {

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 mq =3D dev->data->rx_queues[i];

- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 stats->q_ipackets[i] =3D mq->n_pkts;

<= p class=3D"gmail-p1" style=3D"margin:0px;font:11px Menlo;color:rgb(0,0,0)">= - =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 stats->q_ibytes[i] =3D mq->n_bytes;

- =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats->ipackets +=3D mq->n_= pkts;

- =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats->ibytes +=3D mq-&= gt;n_bytes;

+=C2= =A0 =C2=A0 =C2=A0 =C2=A0 // CTAM test this, as it may not yet initia= lised!

+=C2=A0 = =C2=A0 =C2=A0 =C2=A0 if (mq)

+=C2=A0 =C2=A0 =C2=A0 =C2=A0 {

+ = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats->q_ipackets[i] =3D mq->n= _pkts;

+ =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats->q_= ibytes[i] =3D mq->n_bytes;

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 stats->ipackets +=3D mq->n_pkts;

+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 stats->ibytes +=3D mq->n_bytes;

+=C2=A0 =C2=A0 =C2=A0 = =C2=A0 }

= =C2=A0 =C2=A0 =C2=A0 =C2=A0 }

=

=C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0tmp =3D (pmd->r= ole =3D=3D MEMIF_ROLE_CLIENT) ? pmd->run.num_s2c_rings :


This can occur in several places in the stats code, = and null dereference guards are needed in all locations.


Hope this helps someone else.


John


--000000000000110f0205e3e10aa4--