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 289AFA00C2; Wed, 4 Jan 2023 00:54:02 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B4B7940698; Wed, 4 Jan 2023 00:54:01 +0100 (CET) Received: from mail-ej1-f49.google.com (mail-ej1-f49.google.com [209.85.218.49]) by mails.dpdk.org (Postfix) with ESMTP id E414D40697; Wed, 4 Jan 2023 00:53:59 +0100 (CET) Received: by mail-ej1-f49.google.com with SMTP id fy8so14684009ejc.13; Tue, 03 Jan 2023 15:53:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=pK+xRMbKaJaPAguRiijSkyNucN4MJ8D+YctgX8x+JJs=; b=Yt81rCpjOrCdOVqVpo816PKyuCULDEvNSnpVU7UmH2uC/3oKXmfRIpsvnnfotKw3n9 tqXVdB6UOMdVxojV91I2VL0iG3v52YURvqKA8o9Nbinx7BVRrkbx5er8J2vOOnN9ugY2 Y+X5lLLiI6K5nhacJ/s0sG7vP9KCxBK8TJyvYcwgoFxP1ErgxNglPivzt3BRgeE87Tea l6+jZqSmcN0X0DSaeBRNeYDSlyjpuchQswpyyHBpT+VCcdQWOf47bIUyMWRzggb/BTiY Yp39rOot+LFOTHZwWD7oezhoTzeYQH/uwsPYFFpx5cZVPPR29V5uTVTZvSrRHZd7joBL 4+XA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=pK+xRMbKaJaPAguRiijSkyNucN4MJ8D+YctgX8x+JJs=; b=pWJ2UMFxiNszmQy4EwARaMVvrwc1oznVrYNIKGppCRGAsYcVBOl+bbw3+CRy9f3D+R oM0mDH7syPw54H980AyvqKR9KHj+F1nopV+fw7Pby1OiO4dpspQlq48VvOJFrAlNKE9F kpSEMedb7z6Gr+d4drCtxXbZrOr/NL8C+ILa3CDf/IZSDV2s0EGivkBRxcGZliXzVjSY co+qqgEpxggoDRd6BGGZogL7Iw86L0bQgrYCfUJfAPxcDtjGK3aazTpzpkMTcbnZQsUE tPg1z2gBfiQarLeuSpeGk+PVWmedrdfpFtT4AXS8TR8bfTNEnB8rTaOajXrlWnnGnMj5 uD+g== X-Gm-Message-State: AFqh2kpWx/p1f0KJCuv/3/NnCjUK2pYkEJAIcxoZaOCTo5dp3yqkKWux UUwmJWq1XMWoNqVJzLkMx7T7TRg9DBQ+zTTX4wBU+uMOlL8rV051 X-Google-Smtp-Source: AMrXdXtf8mdXRYf9J3KYwpbOWSgKM16j06ZUHUbxHaKyOyg/AvJkjY6ucJNtZYxJBH9JbMAoHEAT+tAYLtDPHLds+4s= X-Received: by 2002:a17:907:234a:b0:7c1:6344:83e with SMTP id we10-20020a170907234a00b007c16344083emr3228996ejb.560.1672790039041; Tue, 03 Jan 2023 15:53:59 -0800 (PST) MIME-Version: 1.0 References: <20230103185732.2007210-1-ashish.sadanandan@gmail.com> In-Reply-To: <20230103185732.2007210-1-ashish.sadanandan@gmail.com> From: Ashish Sadanandan Date: Tue, 3 Jan 2023 16:53:32 -0700 Message-ID: Subject: Re: [PATCH 1/1] eal/linux: reject mountpt shorter than --huge-dir To: dev@dpdk.org Cc: john.levon@nutanix.com, stable@dpdk.org Content-Type: multipart/alternative; boundary="0000000000000f9c1c05f164c8a8" 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 --0000000000000f9c1c05f164c8a8 Content-Type: text/plain; charset="UTF-8" Please ignore this patch, I'll submit an updated one. I somehow managed to only execute a subset of the fast-tests suite initially and didn't run eal_flags_misc_autotest at all. Now I see that my proposed fix is flawed, I will submit another try soon. Sorry for the noise On Tue, Jan 3, 2023 at 11:57 AM Ashish Sadanandan < ashish.sadanandan@gmail.com> wrote: > The code added for allowing --huge-dir to specify hugetlbfs > sub-directories has a bug where it incorrectly matches mounts that > contain a prefix of the specified --huge-dir. > > Consider --huge-dir=/dev/hugepages1G is passed to rte_eal_init. Given > the following hugetlbfs mounts > > $ mount | grep hugetlbfs > hugetlbfs on /dev/hugepages type hugetlbfs (rw,relatime,pagesize=2M) > hugetlbfs on /dev/hugepages1G type hugetlbfs (rw,relatime,pagesize=1024M) > hugetlbfs on /mnt/huge type hugetlbfs (rw,relatime,pagesize=2M) > > get_hugepage_dir is first called with hugepage_sz=2097152. While > iterating over all mount points, /dev/hugepages is incorrectly > determined to be a match because it's a prefix of --huge-dir. The caller > then obtains an exclusive lock on --huge-dir. > > In the next call to get_hugepage_dir, hugepage_sz=1073741824. This call > correctly determines /dev/hugepages1G is a match. The caller again > attempts to obtain an exclusive lock on --huge-dir and deadlocks because > it's already holding a lock. > > This has been corrected by rejecting the mount point being considered if > its length is smaller than the specified --huge-dir. > > Fixes: 24d5a1ce6b85 ("eal/linux: allow hugetlbfs sub-directories") > Cc: john.levon@nutanix.com > Cc: stable@dpdk.org > --- > lib/eal/linux/eal_hugepage_info.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/lib/eal/linux/eal_hugepage_info.c > b/lib/eal/linux/eal_hugepage_info.c > index a1b6cb31ff..fcc3d82fdf 100644 > --- a/lib/eal/linux/eal_hugepage_info.c > +++ b/lib/eal/linux/eal_hugepage_info.c > @@ -269,16 +269,19 @@ get_hugepage_dir(uint64_t hugepage_sz, char > *hugedir, int len) > * Ignore any mount that doesn't contain the --huge-dir > * directory. > */ > - if (strncmp(internal_conf->hugepage_dir, splitstr[MOUNTPT], > - strlen(splitstr[MOUNTPT])) != 0) { > + size_t mountpt_len = strlen(splitstr[MOUNTPT]); > + > + if (strlen(internal_conf->hugepage_dir) > mountpt_len) > + continue; > + else if (strncmp(internal_conf->hugepage_dir, > splitstr[MOUNTPT], > + mountpt_len) != 0) > continue; > - } > > /* > * We found a match, but only prefer it if it's a longer > match > * (so /mnt/1 is preferred over /mnt for matching > /mnt/1/2)). > */ > - if (strlen(splitstr[MOUNTPT]) > strlen(found)) > + if (mountpt_len > strlen(found)) > strlcpy(found, splitstr[MOUNTPT], len); > } /* end while fgets */ > > -- > 2.27.0 > > --0000000000000f9c1c05f164c8a8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Please ignore this patch, I'll submit an updated one. I somehow ma= naged to only execute a subset of the fast-tests suite initially and didn&#= 39;t run eal_flags_misc_autotest at all. Now I see that my proposed fix is = flawed, I will submit another try soon.

Sorry for = the noise

= On Tue, Jan 3, 2023 at 11:57 AM Ashish Sadanandan <ashish.sadanandan@gmail.com> wrote:
The code added for all= owing --huge-dir to specify hugetlbfs
sub-directories has a bug where it incorrectly matches mounts that
contain a prefix of the specified --huge-dir.

Consider --huge-dir=3D/dev/hugepages1G is passed to rte_eal_init. Given
the following hugetlbfs mounts

$ mount | grep hugetlbfs
hugetlbfs on /dev/hugepages type hugetlbfs (rw,relatime,pagesize=3D2M)
hugetlbfs on /dev/hugepages1G type hugetlbfs (rw,relatime,pagesize=3D1024M)=
hugetlbfs on /mnt/huge type hugetlbfs (rw,relatime,pagesize=3D2M)

get_hugepage_dir is first called with hugepage_sz=3D2097152. While
iterating over all mount points, /dev/hugepages is incorrectly
determined to be a match because it's a prefix of --huge-dir. The calle= r
then obtains an exclusive lock on --huge-dir.

In the next call to get_hugepage_dir, hugepage_sz=3D1073741824. This call correctly determines /dev/hugepages1G is a match. The caller again
attempts to obtain an exclusive lock on --huge-dir and deadlocks because it's already holding a lock.

This has been corrected by rejecting the mount point being considered if its length is smaller than the specified --huge-dir.

Fixes: 24d5a1ce6b85 ("eal/linux: allow hugetlbfs sub-directories"= )
Cc: john.levon@= nutanix.com
Cc: stable@dpdk.org
---
=C2=A0lib/eal/linux/eal_hugepage_info.c | 11 +++++++----
=C2=A01 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/eal/linux/eal_hugepage_info.c b/lib/eal/linux/eal_hugepage= _info.c
index a1b6cb31ff..fcc3d82fdf 100644
--- a/lib/eal/linux/eal_hugepage_info.c
+++ b/lib/eal/linux/eal_hugepage_info.c
@@ -269,16 +269,19 @@ get_hugepage_dir(uint64_t hugepage_sz, char *hugedir,= int len)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Ignore any = mount that doesn't contain the --huge-dir
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* directory.<= br> =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 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (strncmp(interna= l_conf->hugepage_dir, splitstr[MOUNTPT],
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0strlen(splitstr[MOUNTPT])) !=3D 0) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0size_t mountpt_len = =3D strlen(splitstr[MOUNTPT]);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (strlen(internal= _conf->hugepage_dir) > mountpt_len)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0continue;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else if (strncmp(in= ternal_conf->hugepage_dir, splitstr[MOUNTPT],
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0mountpt_len) !=3D 0)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 continue;
-=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 =C2=A0 =C2=A0 =C2=A0 /*
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* We found a = match, but only prefer it if it's a longer match
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* (so /mnt/1 = is preferred over /mnt for matching /mnt/1/2)).
=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 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (strlen(splitstr= [MOUNTPT]) > strlen(found))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (mountpt_len >= ; strlen(found))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 strlcpy(found, splitstr[MOUNTPT], len);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 } /* end while fgets */

--
2.27.0

--0000000000000f9c1c05f164c8a8--