From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-0010f301.pphosted.com (mx0a-0010f301.pphosted.com [148.163.149.254]) by sourceware.org (Postfix) with ESMTPS id 471C03858D33 for ; Tue, 17 Oct 2023 20:01:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 471C03858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=rice.edu Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rice.edu ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 471C03858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=148.163.149.254 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697572896; cv=none; b=HsNWYDkY/juk/iEd3w258lpnCYnWf7WyPntascRd0NnHMFlF3kjAjN37I7jcSpKnpEbs0LfHITmYCiQDB53ky5/eybTT6iOJeueeCbqpNieVj8W+MUlgzqBita8KX1UPdEVIntAzi1EatKc4+cRS3xQ6+1gP92tf9rOCpUTp1Xk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697572896; c=relaxed/simple; bh=V5CQM6flHg2gn5DV4VB10opXNfKGJAuni9kQnHzYDlY=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=Skzbiz4NnMexuqQb9buPtDZ/5dKig79mTlBga1gZc10InffSV29wmIB01WQFkEwl+3kXtB2Vcu+ZcWza5d6J56oI+DgPgmrIptWugskki2MYHgnHqMSYmCdWan1XkgGQF1vD0UxXrTUzVrOMzBKCgCF37MV3YdhUdLR6k1jA9Eg= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from pps.filterd (m0102855.ppops.net [127.0.0.1]) by mx0b-0010f301.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 39HJo9cH029578 for ; Tue, 17 Oct 2023 15:01:33 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rice.edu; h= mime-version:references:in-reply-to:from:date:message-id:subject :to:cc:content-type; s=ricemail; bh=ZmxHy4A60G1KVmuZlQ4aB9dvXo0b vPMnJ4eBmUB1/Uk=; b=oJAsBf81KuTIysj0OpkeVCN1ief4YJ2AHnoptbI+Jklr lBw9pGPjF6oVVdGyFCmEDk7va+M2kTjcCnG5epxfmd4wdo+9xXvdVzO0hIglg1GK qmHGbQiQlh3r0RQRoysax0WJrWTUDTBKGwbFk0g86psW0twca8CzPnwTdrMKvsNH m6GTrAsRE7A939QTuybr0r/Fj0kMzxvKQpmr7uQiPVZcwjgkwka23KDNkpIWaSSW a1wwq3jLmENyO0phe6Ju9HkPGoghJ+JgC8Di+L2qWY5NYFCeFKW3yESMHeJ8m4l6 PKHgRpw/Ht46/kUfazRYsM78NZeJflW6jVR+fzEPpQ== Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by mx0b-0010f301.pphosted.com (PPS) with ESMTPS id 3tsc869x3a-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Tue, 17 Oct 2023 15:01:33 -0500 (CDT) Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-32d9b520d15so2419167f8f.3 for ; Tue, 17 Oct 2023 13:01:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697572890; x=1698177690; 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=ZmxHy4A60G1KVmuZlQ4aB9dvXo0bvPMnJ4eBmUB1/Uk=; b=ghTdPd0o1Ow8c4F4NXXPMo6PLjTNj6rDszgacLBF5ZJgInWOWfbrX5TShNp+6fZoes rmenia9eagljOivFzc6w0fWCZObaYsk3pXG1fdVcM0eKf48JYCAHBa7/z2A22Rw/gx0W CfZPEKyk2jCfX8T3KH++26YEeq0CHwkyw7RrgIQHWIvJTQCuKNfXt5VPv4zYffNa2YTn CjMzcILFmOwr2RlwhLVd4tLQg0vWo9d+5W8q34WYTyOgXMRqoQyOl9vq6jx4oQeMCsZc Ad30RVXn8Tt0Uo9wbBZ9VXmdpfeIXy9SL5R75JjLcV9bNxHREA7sR/TO/7fIBF6C2R6l B8Cg== X-Gm-Message-State: AOJu0YyqRKcLlfpD5Mo4QGPw9pScrR+Dv07fhWrk5o3tXEa4A6LrJU9S V/83BDCWIne9s9fK0rHoCp0PdutyNFcDihWeAZW6W31eOYORef2ltdmEgVE+Iuwe4fabkU54bJa Fks5EYwoevfY0Td5QBuQ4QCqlS6rtY8MEtCSXbAngOAIwNL5X0wG1TV+sJa3qkbXGypK9pJpDem 9bmeg= X-Received: by 2002:a5d:540d:0:b0:32d:a2bb:c996 with SMTP id g13-20020a5d540d000000b0032da2bbc996mr2626431wrv.69.1697572890199; Tue, 17 Oct 2023 13:01:30 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHvQVh0Lvwq4VhSOEDed2FhxeG6EcSrENwH+nshbdf5eq8xiwq3YEN4Sk3HomwiC9+Kc75ixevmW8DW7mAs8W8= X-Received: by 2002:a5d:540d:0:b0:32d:a2bb:c996 with SMTP id g13-20020a5d540d000000b0032da2bbc996mr2626419wrv.69.1697572889885; Tue, 17 Oct 2023 13:01:29 -0700 (PDT) MIME-Version: 1.0 References: <301fac87e83ebbbd677750579ae9a3429b461bdf.camel@klomp.org> <20231010134300.53830-1-mark@klomp.org> <20231010134300.53830-12-mark@klomp.org> <20231011171716.GO728@gnu.wildebeest.org> In-Reply-To: <20231011171716.GO728@gnu.wildebeest.org> From: Heather McIntyre Date: Tue, 17 Oct 2023 15:01:18 -0500 Message-ID: Subject: Re: [PATCH 12/16] libdw: Make libdw_find_split_unit thread-safe To: Mark Wielaard Cc: elfutils-devel@sourceware.org Content-Type: multipart/alternative; boundary="0000000000001518680607eefd63" X-Proofpoint-DLP: Gmail-Outbound X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-17_03,2023-10-17_01,2023-05-22_02 X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,HTML_MESSAGE,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --0000000000001518680607eefd63 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Mark, As per your suggestion, I have placed the lock around the entire __libdw_find_split_unit function call. On Wed, Oct 11, 2023 at 12:17=E2=80=AFPM Mark Wielaard wro= te: > Hi Heather, > > On Tue, Oct 10, 2023 at 03:42:56PM +0200, Mark Wielaard wrote: > > From: Heather McIntyre > > > > * (try_split_file): Use eu_tsearch. > > Add lock for cu->split. > > > > Signed-off-by: Heather S. McIntyre > > Signed-off-by: Mark Wielaard > > --- > > libdw/libdw_find_split_unit.c | 43 +++++++++++++++++++++++++++++------ > > 1 file changed, 36 insertions(+), 7 deletions(-) > > > > diff --git a/libdw/libdw_find_split_unit.c > b/libdw/libdw_find_split_unit.c > > index 533f8072..a288e9bd 100644 > > --- a/libdw/libdw_find_split_unit.c > > +++ b/libdw/libdw_find_split_unit.c > > @@ -34,13 +34,19 @@ > > #include "libelfP.h" > > > > #include > > -#include > > +#include > > #include > > #include > > #include > > #include > > #include > > > > +/* __libdw_link_skel_split() modifies "skel->split =3D split" > > + and "split->split =3D skel". > > + "cu->split" is read at multiple locations and conditionally updated. > > + Mutual exclusion is enforced to prevent a race. */ > > +rwlock_define(static, cu_split_lock); > > __libdw_link_skel_split is defined in libdw/libdwP.h and is called in > try_split_file. (It is also called in src/readelf.c but that is a > terrible hack, so ignore that for now.) > > A Dwarf_CU is created (see libdw/libdw_findcu.c) with split set to > (Dwarf_CU *) -1 to indicate the split dwarf is not yet set. If > cu->split is NULL it means the split dwarf was searched for, but not > found. > > > static void > > try_split_file (Dwarf_CU *cu, const char *dwo_path) > > { > > @@ -57,7 +63,7 @@ try_split_file (Dwarf_CU *cu, const char *dwo_path) > > if (split->unit_type =3D=3D DW_UT_split_compile > > && cu->unit_id8 =3D=3D split->unit_id8) > > { > > - if (tsearch (split->dbg, &cu->dbg->split_tree, > > + if (eu_tsearch (split->dbg, &cu->dbg->split_tree, > > __libdw_finddbg_cb) =3D=3D NULL) > > { > > /* Something went wrong. Don't link. */ > > @@ -65,9 +71,13 @@ try_split_file (Dwarf_CU *cu, const char *dwo_path) > > break; > > } > > > > + rwlock_wrlock(cu_split_lock); > > + > > /* Link skeleton and split compile units. */ > > __libdw_link_skel_split (cu, split); > > > > + rwlock_unlock(cu_split_lock); > > + > > Is this locking wide enough? It seems like multiple threads can race > to this code and set different Dwarfs (which means they'll leak). See > below. > > > /* We have everything we need from this ELF > > file. And we are going to close the fd to > > not run out of file descriptors. */ > > @@ -75,8 +85,13 @@ try_split_file (Dwarf_CU *cu, const char *dwo_path) > > break; > > } > > } > > + > > + rwlock_rdlock(cu_split_lock); > > + > > if (cu->split =3D=3D (Dwarf_CU *) -1) > > dwarf_end (split_dwarf); > > + > > + rwlock_unlock(cu_split_lock); > > This means __libdw_link_skel_split wasn't called above (so the > split_dwarf created by dwarf_begin didn't contain the expected CU). > > > } > > /* Always close, because we don't want to run out of file > > descriptors. See also the elf_fcntl ELF_C_FDDONE call > > @@ -89,9 +104,13 @@ Dwarf_CU * > > internal_function > > __libdw_find_split_unit (Dwarf_CU *cu) > > { > > + rwlock_rdlock(cu_split_lock); > > + Dwarf_CU *cu_split_local =3D cu->split; > > + rwlock_unlock(cu_split_lock); > > + > > /* Only try once. */ > > - if (cu->split !=3D (Dwarf_CU *) -1) > > - return cu->split; > > + if (cu_split_local !=3D (Dwarf_CU *) -1) > > + return cu_split_local; > > > > /* We need a skeleton unit with a comp_dir and [GNU_]dwo_name > attributes. > > The split unit will be the first in the dwo file and should have > the > > @@ -116,7 +135,11 @@ __libdw_find_split_unit (Dwarf_CU *cu) > > free (dwo_path); > > } > > > > - if (cu->split =3D=3D (Dwarf_CU *) -1) > > + rwlock_rdlock(cu_split_lock); > > + cu_split_local =3D cu->split; > > + rwlock_unlock(cu_split_lock); > > + > > + if (cu_split_local =3D=3D (Dwarf_CU *) -1) > > { > > /* Try compdir plus dwo_name. */ > > Dwarf_Attribute compdir; > > It looks like multiple threads can end up here after having seen > cu->split/cu_split_local =3D=3D (Dwarf_CU *) -1) Which means multiple > threads will enter try_split_file and might all call > __libdw_link_skel_split as mentioned above. > > > @@ -138,9 +161,15 @@ __libdw_find_split_unit (Dwarf_CU *cu) > > } > > } > > > > + rwlock_wrlock(cu_split_lock); > > + > > /* If we found nothing, make sure we don't try again. */ > > if (cu->split =3D=3D (Dwarf_CU *) -1) > > - cu->split =3D NULL; > > + { > > + cu->split =3D NULL; > > + cu_split_local =3D cu->split; > > + } > > > > - return cu->split; > > + rwlock_unlock(cu_split_lock); > > + return cu_split_local; > > } > > I think it would be better to keep the lock during the whole > __libdw_find_split_unit call. > > Cheers, > > Mark > --0000000000001518680607eefd63--