From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-0010f301.pphosted.com (mx0b-0010f301.pphosted.com [148.163.153.244]) by sourceware.org (Postfix) with ESMTPS id 326A03858C54 for ; Tue, 17 Oct 2023 19:26:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 326A03858C54 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 326A03858C54 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=148.163.153.244 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697570764; cv=none; b=uQ6789vMzOmzpNLI7ZjdYglDRXPaD1I4yv4uIY4gMMrewe8I66T/YzRaIfxFVZnhcG8xjQ9g2qhE8rhSS1dOc5JFQA2ztK5UF8Ihvm48FOU5jJAej46ozh+6XpbfYn4m3Bo5a8JR6YCim1KjMvKHg8QaW2nJ8+jW0c3G3B5/OPs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697570764; c=relaxed/simple; bh=LPgcl8ETYzm2ZVearZBPxVsyljWtmC9rbgl7GRctdIk=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=Kp2EExVwS1NgD53cBNuMcmNu7ZWkNekosW2hq+2vWkpOjbQ7q4eoQyioa8uStOUwIQyyLpTthfFCKin+IFiag/iQKcm3JMoa0MadvlL66hIcSpSwqnj7KtJNLAAXBOzLwFdjLn9bvI+xpnpXVlSAVtcTpusE4jt7gtBCDSS0aug= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from pps.filterd (m0102859.ppops.net [127.0.0.1]) by mx0b-0010f301.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 39HIAWD9007350 for ; Tue, 17 Oct 2023 14:26:02 -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=ZO7WVMzdXLCPQqEElI1UdIUDokU9 OeImX8FuTIfIxDw=; b=TbmSE9P5q8KWkZBerfGke6VbS2mSTgHq4ozV0MCy5zed g8ejEClx72aVWCf4ml2/j3zaExN2oYtZinYs3HAoK6NnGQDn/PKAt8lx4ICRy70N u5BlPgqtbZXkiU9S5QSfu9D9ZJUJw1Own0V7JfUj5iZJY/jw/ccJ1JlXQHnmOxLa Vl+Jbn6kDsmiKreY5knCT6mpeU9aoWApDIHLehk0WyAq0zB5tI9R6nYVsrrK2oRb VpL3tXzECqZee8ZULzFo+x9yfII8OIlJS8c/TLkyWV+TNpQouNkeJI/5Ofcr+umP i4VhDseIC4zmT2asGbszRV/1x1pff/0prIrQtAD82g== 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 3tsyan06c7-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Tue, 17 Oct 2023 14:26:02 -0500 (CDT) Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-32d9b520d15so2406164f8f.3 for ; Tue, 17 Oct 2023 12:26:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697570761; x=1698175561; 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=ZO7WVMzdXLCPQqEElI1UdIUDokU9OeImX8FuTIfIxDw=; b=vPRei2PzP088QwMhTzBJXZ+AdPgQR69Pzip6tzL3VpSCiAkHrQeNa0pXQKiba/ucW7 7coVReQSybj0ak5XO2h1gjHjf0KR+CDg6tgjE0A9UEzrgZPLEd3Qm8REmwTXVBmY7cax nZ3EiJHeLjPMRhIlJjVj5wdW/UTQtdKLGtzfcr0ChSeqReTq8ZWLbrqbMAOmEB1h1wsh E9noan9rLNTR6Kd4/dR0M4IrQv6pkhrzv1S6pb/mKypXet4Oqbpr4K5GoASFbSqJ3aTZ W7pZFzoYHaCYv4gEOlVmTSZ2AOkKzBLCbWPMG1fwsVV5+BwouboRDwlBeNeJkkiqZSXh sFag== X-Gm-Message-State: AOJu0Yws+nco3KKjESIjwmM+Y4A2BqmRQQjX26vrkpAo813BvLnT9TKh Rh3JMEfKy7+af3klaokTN6BnetaoA48EApxARtSzeeBCw2z314kjSKax0tYOx7OFZsnCqKpWUfD onEMIvqQWQDVau+vmlsm3jEx/rGARWciSL1AqSKAYxI91tNoEsbN6UyMXZZXXmb3vQH3IIPGxsC XDVOw= X-Received: by 2002:a5d:614d:0:b0:32d:939d:c7cf with SMTP id y13-20020a5d614d000000b0032d939dc7cfmr2402516wrt.52.1697570760989; Tue, 17 Oct 2023 12:26:00 -0700 (PDT) X-Google-Smtp-Source: AGHT+IE3kWw4cNs83gQZuKpbN6bHuV/t2J8EtnLqdsTaaWgJYKNgoedP9FezNyeJJP7tgwsfklgIf9f/iHxl4Nk593k= X-Received: by 2002:a5d:614d:0:b0:32d:939d:c7cf with SMTP id y13-20020a5d614d000000b0032d939dc7cfmr2402509wrt.52.1697570760618; Tue, 17 Oct 2023 12:26:00 -0700 (PDT) MIME-Version: 1.0 References: <301fac87e83ebbbd677750579ae9a3429b461bdf.camel@klomp.org> <20231010134300.53830-1-mark@klomp.org> <20231010134300.53830-10-mark@klomp.org> <20231010220256.GN728@gnu.wildebeest.org> In-Reply-To: <20231010220256.GN728@gnu.wildebeest.org> From: Heather McIntyre Date: Tue, 17 Oct 2023 14:25:49 -0500 Message-ID: Subject: Re: [PATCH 10/16] libdw: make dwarf_getalt thread-safe To: Mark Wielaard Cc: elfutils-devel@sourceware.org Content-Type: multipart/alternative; boundary="0000000000002b117c0607ee7eed" 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.2 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: --0000000000002b117c0607ee7eed Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Since it wasn't too complicated to do, I implemented a dwarf object lock (per struct dwarf lock) instead of having the static global lock. As per your suggestion, I placed the lock over the whole dwarf_getalt function, so now find_debug_altlink is called with the lock already acquired. In addition, I placed locking around dwarf_setalt. I am currently in the process of testing these changes along with others to make sure everything is ironed out. On Tue, Oct 10, 2023 at 5:04=E2=80=AFPM Mark Wielaard wrot= e: > Hi Heather, > > On Tue, Oct 10, 2023 at 03:42:54PM +0200, Mark Wielaard wrote: > > * libdw/dwarf_getalt.c: Add lock for > > dbg->alt_dwarf/main->alt_dwarf. > > This takes care of dwarf_getalt. Shouldn't dwarf_setalt also use > locking? > > > Signed-off-by: Heather S. McIntyre > > Signed-off-by: Mark Wielaard > > --- > > libdw/dwarf_getalt.c | 27 ++++++++++++++++++++++----- > > 1 file changed, 22 insertions(+), 5 deletions(-) > > > > diff --git a/libdw/dwarf_getalt.c b/libdw/dwarf_getalt.c > > index 0a12dfae..e3894c8c 100644 > > --- a/libdw/dwarf_getalt.c > > +++ b/libdw/dwarf_getalt.c > > @@ -44,6 +44,10 @@ > > #include > > #include > > > > +/* find_debug_altlink() modifies "dbg->alt_dwarf". > > + dwarf_getalt() reads "main->alt_dwarf". > > + Mutual exclusion is enforced to prevent a race. */ > > +rwlock_define(static, alt_dwarf_lock); > > Probably overkill, but should we consider a Dwarf lock object instead > of having a static global lock? > > > char * > > internal_function > > @@ -152,7 +156,9 @@ find_debug_altlink (Dwarf *dbg) > > Dwarf *alt =3D dwarf_begin (fd, O_RDONLY); > > if (alt !=3D NULL) > > { > > + rwlock_wrlock(alt_dwarf_lock); > > dbg->alt_dwarf =3D alt; > > + rwlock_unlock(alt_dwarf_lock); > > dbg->alt_fd =3D fd; > > } > > else > > Is this lock wide enough? See also below. It looks like multiple > threads could arrive at this point at the same time. so alt_dwarf and > alt_fd can be (re)set multiple times, causing leaks of the Dwarf and > fd. > > > @@ -163,22 +169,33 @@ find_debug_altlink (Dwarf *dbg) > > Dwarf * > > dwarf_getalt (Dwarf *main) > > { > > + rwlock_rdlock(alt_dwarf_lock); > > + Dwarf *alt_dwarf_local =3D main->alt_dwarf; > > + rwlock_unlock(alt_dwarf_lock); > > + > > /* Only try once. */ > > - if (main =3D=3D NULL || main->alt_dwarf =3D=3D (void *) -1) > > + if (main =3D=3D NULL || alt_dwarf_local =3D=3D (void *) -1) > > return NULL; > > > > - if (main->alt_dwarf !=3D NULL) > > - return main->alt_dwarf; > > + if (alt_dwarf_local !=3D NULL) > > + return alt_dwarf_local; > > > > So at this point it looks like we can have multiple threads running > (because the lock has been dropped above) all with alt_dwarf_local > (and main->alt_dwarf) being NULL. > > > find_debug_altlink (main); > > > > + rwlock_rdlock(alt_dwarf_lock); > > + alt_dwarf_local =3D main->alt_dwarf; > > + rwlock_unlock(alt_dwarf_lock); > > + > > /* If we found nothing, make sure we don't try again. */ > > - if (main->alt_dwarf =3D=3D NULL) > > + if (alt_dwarf_local =3D=3D NULL) > > { > > + rwlock_wrlock(alt_dwarf_lock); > > main->alt_dwarf =3D (void *) -1; > > + rwlock_unlock(alt_dwarf_lock); > > + > > return NULL; > > } > > > > - return main->alt_dwarf; > > + return alt_dwarf_local; > > } > > INTDEF (dwarf_getalt) > > -- > > 2.41.0 > > > > It might be better to take the lock over the whole function (and only > call find_debug_altlink with the lock held). > > Cheers, > > Mark > --0000000000002b117c0607ee7eed--