From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id C36C71F953 for ; Wed, 17 Nov 2021 18:08:45 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7280E385841C for ; Wed, 17 Nov 2021 18:08:44 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7280E385841C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1637172524; bh=MkEc0WXfhK9YupVlJMVVuKoVEU3IL66v+j2KVG+6KVI=; h=Subject:Date:References:To:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:From; b=Uln5gLtBZaWpF1bLOJ4ngE7I2hKkTCpddEqugYl1vnI/vNXjaHZngv43qKCF3cRBC 6USxIYiqdVPzjl3l3wkVbVnbBIRwq1JcSdADjii56QDVaeYRM4NGS2T0rIsf+X3pOs 4Jfssg5MyLGWMJhLtaiNZRrw5P57gioZiUoXD/cY= Received: from mx0a-0010f301.pphosted.com (mx0a-0010f301.pphosted.com [148.163.149.254]) by sourceware.org (Postfix) with ESMTPS id D8D973858402 for ; Wed, 17 Nov 2021 18:08:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D8D973858402 Received: from pps.filterd (m0102856.ppops.net [127.0.0.1]) by mx0b-0010f301.pphosted.com (8.16.1.2/8.16.1.2) with ESMTP id 1AHHuL0B003282 for ; Wed, 17 Nov 2021 12:08:21 -0600 Received: from mx2.mail.rice.edu (smtp1.mail.rice.edu [128.42.199.100]) by mx0b-0010f301.pphosted.com (PPS) with ESMTPS id 3cd699g1ug-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 17 Nov 2021 12:08:21 -0600 Received: from mx2.mail.rice.edu (localhost [127.0.0.1]) by mx2.mail.rice.edu (Postfix) with ESMTP id E00A640FF25 for ; Wed, 17 Nov 2021 12:08:20 -0600 (CST) Received: from localhost (localhost [127.0.0.1]) by mx2.mail.rice.edu (Postfix) with ESMTP id DE2BC414A2C; Wed, 17 Nov 2021 12:08:20 -0600 (CST) X-Virus-Scanned: by amavis-2.12.1 at mx2.mail.rice.edu, auth channel Received: from mx2.mail.rice.edu ([127.0.0.1]) by localhost (mx2.mail.rice.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id 64vUqeyQnI41; Wed, 17 Nov 2021 12:08:19 -0600 (CST) Received: from [192.168.50.202] (c-98-200-175-18.hsd1.tx.comcast.net [98.200.175.18]) (using TLSv1.2 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: johnmc@rice.edu) by mx2.mail.rice.edu (Postfix) with ESMTPSA id 51C6D209C24; Wed, 17 Nov 2021 12:08:18 -0600 (CST) Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: Fwd: [PATCH v5 00/22] Some rtld-audit fixes Date: Wed, 17 Nov 2021 12:08:17 -0600 References: To: John Mellor-Crummey via Libc-alpha Message-Id: <0D3F0C5F-2586-42F9-916D-2F327432AF13@rice.edu> X-Mailer: Apple Mail (2.3608.120.23.2.4) X-Proofpoint-GUID: KpmI1hKC4hiti7ZNazYDXQXS9K-NTfYw X-Proofpoint-ORIG-GUID: KpmI1hKC4hiti7ZNazYDXQXS9K-NTfYw X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.0.607.475 definitions=2021-11-17_06,2021-11-17_01,2020-04-07_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 lowpriorityscore=0 malwarescore=0 priorityscore=1501 mlxscore=0 suspectscore=0 mlxlogscore=999 adultscore=0 spamscore=0 clxscore=1011 phishscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000 definitions=main-2111170082 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: John Mellor-Crummey via Libc-alpha Reply-To: John Mellor-Crummey Cc: Xiaozhu Meng , John Mellor-Crummey , "Mark W. Krentel" Errors-To: libc-alpha-bounces+e=80x24.org@sourceware.org Sender: "Libc-alpha" We are grateful to the members of the libc-alpha team who have been = working on authoring (Adhemerval Zanella) and reviewing (Florian Weimer) = fixes for the auditor problems that we previously reported to this list. Jonathon Anderson, the auditor expert on my team, has reviewed the = patches and offers some feedback about them. -- John Mellor-Crummey Professor Dept of Computer Science Rice University email: johnmc@rice.edu phone: 713-348-5179 > Begin forwarded message: >=20 > From: Jonathon Anderson > Subject: Re: [PATCH v5 00/22] Some rtld-audit fixes > Date: November 16, 2021 at 5:38:28 PM CST >=20 >=20 > =3D=3D=3D Reply to [00/20]: =3D=3D=3D >=20 > Many thanks to Adhemerval Zanella for authoring and maintaining these = fixes and to Florian Weimer for reviewing them! We are very happy to see = progress towards getting our bugs resolved. >=20 > I have reviewed the tests included with these patches, presuming all = these tests pass all of our reported 'Tier 1' issues are resolved by = this patchset. I have also tested this patch series in an x86-64 Fedora = rawhide container, all of our 'Tier 1' issue reproducers pass (except = for the ARM-specific reproducer which I skipped). Not including the ones = Adhemerval raised questions about, our 'Tier 2' issues that are not yet = resolved in upstream + these patches are as follows (see [1] for full = details): >=20 > - "Various Glibc functions cannot be called from an auditor": Several = Glibc functions segfault when called from an auditor, many dependent on = the initialization of the Glibc in the application namespace = (LM_ID_BASE). Patch [13/20] fixes and tests the isspace case. gethostid = does not have a test but our reproducer passes. dlopen/dladdr are also = known to have this issue, fixes for these are not included in this = patchset. >=20 > - "La_activity calls are missing or mis-ordered with respect to = la_obj* calls": The calls to la_activity seen in practice often differ = from our presumed interpretation of the rtld-audit man page. Patch = [17/20] fixes case 2 (la_activity is not called during application exit, = even though la_objclose is). A fix for case 4 = (la_activity(LA_ACT_DELETE) is only called after la_objclose, and = immediately followed by la_activity(LA_ACT_CONSISTENT)) is not included = in this patchset. >=20 > Overall, we are very pleased with these patches, and would love to see = them included in official releases of Glibc and RHEL in the near future. >=20 > In addition, since the time of the initial development of these = patches we have uncovered a pair of new 'Tier 2' issues (see [1] for = full details): >=20 > - "Related recursive dl*open calls can crash or cause inconsistent = state": Dlopen and dlmopen when called recursively (under very specific = conditions) will crash uncatchably or return prior to the init = constructors of the requested binaries (the difference is presumed to be = based on whether loader assertions were enabled during the build). >=20 > - "Auditor namespaces (undocumentedly) differ from normal = namespaces": The separate namespace used to load an auditor behaves very = differently from a "normal" namespace created by the application, = however there is no indication in the auditor callbacks that a namespace = falls into this "special" case. Since LD_AUDIT can load auditors = arbitrarily *all* auditors must be robust against the idiosyncrasies of = auditor namespaces, complicating already delicate auditor code. >=20 > Of course, we would also like our 'Tier 2' issues to also be resolved = soon. Below are our responses to the questions Adhemerval raised = concerning a few of our issues: >=20 > > There is also some point brough by John Melloc-Crummey documents = that > > I don't have a straighforward answer so I haven't added on this > > patchset: > > > > 1 la_activity(LA_ACT_ADD) is never called for auditor namespaces, > > even though la_objopen and la_activity(LA_ACT_CONSISTENT) are. > > > > There is no easy solution for this: we need at least to load the > > *first* auditor to actually issue the la_activity(LA_ACT_ADD). It > > means that it would *only* work for subsequent audit modules, and > > adding this specific semantic is confusing and does not really > > improve things (it only helps when multiple audit modules are = used). >=20 > Our reproducer for this issue passes even though there is no explicit = test, so this question may be resolved in part already. To provide some = of our motivation for this issue: >=20 > The intended use case is in fact multiple audit modules. As detailed = in the introduction to our issue document [1], the available interfaces = aside from LD_AUDIT for intercepting functions are insufficient for = libraries intended to accelerate applications and for performance tools. = We predict a near-future scenario where large-scale applications are = accelerated by auditors, in order to provide suitable performance = measurements in this environment we need first-class support for = multiple auditors. We see this already taking place with Spindle [2], an = auditor which accelerates dynamic library loading using cross-node = caching and communication. If Spindle is helping or hurting the = performance of an application, we want to be able to see that in our = performance measurements. Degraded callbacks from Glibc for auditors = increases the burden and complexity on our own code to work = around/defend against such idiosyncrasies. If there are multiple = auditors other than HPCToolkit, e.g. spindle and any other auditor, each = of the auditors may need to address these issues. >=20 > > 2. la_objopen is called for the main binary and for ld.so before = the > > first la_activity(LA_ACT_ADD) call. This contradicts the = pattern > > found in a successful dlopen (where la_activity(LA_ACT_ADD) > > precedes la_objopen). > > > > The constrain here is we need to handle DT_AUDIT and DT_DEPAUDIT > > dynamic tags, which means we need to first load the executable in > > memory to parse the required audit modules. So we need to first = parse > > the dynamic audit tags, load the audit modules, and then load the > > object itself. >=20 > We are wholly sympathetic to the difficulty of adjusting complex and = delicate code! >=20 > =46rom an outside perspective this constraint does not seem to be an = issue, consider that the executable's la_objopen must occur *after* the = DT_*AUDIT auditors have been loaded so that it can be reported to the = newly-loaded auditors, this also matches what happens in miniature = experiments. I would think then that there is (hopefully one) code path = where la_activity(ADD) could be called just before the la_objopen for = the executable or just after DT_*AUDIT auditors have been loaded. >=20 > > 3. For non-PIE executables the base address listed in = link_map->l_addr > > for the main application binary is 0, even though dladdr is = able to > > recover the correct offset. La_objopen is affected by this. > > > > This would require to change an internal semantic for = link_map->l_addr. > > This is not straighfoward and I am not sure about the direct = gains. >=20 > Again, we are wholly sympathetic to the difficulty of refactoring = complex code! >=20 > The motivation for providing a consistent link_map->l_addr value is to = unify the handling for the main executable with any other binary and to = allow access to the ELF header of the main executable (which provides = fields not available anywhere else: type, ABI, entry point...). An = alternative would be to re-open the file from its path = (link_map->l_name), however this is a serious performance concern for = large-scale executions (metadata servers are known to be a bottleneck of = parallel filesystems). dladdr is not always an option for auditors, as = noted by another of our 'Tier 2' issues. Right now, we only require the = program headers which we can obtain from getauxval(AT_PHDR), however = this technique has questionable portability and robustness (getauxval = returns an unsigned long, not a pointer). >=20 > =46rom an outside perspective the current l_addr semantic is fairly = undocumented, the dladdr and dlinfo man pages define it vaguely as the = "difference between the address in the ELF file and the address in = memory." That sounds (to me at least) like l_addr should point to byte 0 = in the file (the ELF header), and that seems to be correct in all but = the non-PIE case. It makes little sense to expose a value without a = clear (documented!) interpretation, and knowing this deviation makes it = unclear to me as a user how I should be using this value (and makes me = wonder how many other users have made the same mistake). >=20 > dladdr gets its value from link_map->l_map_start instead of l_addr, so = the semantic we want is already present in a private field. It seems to = me these two fields could be swapped with little issue, if altering the = public semantic is not acceptable we could also be sated if l_map_start = was made public. >=20 > [1] = https://docs.google.com/document/d/1dVaDBdzySecxQqD6hLLzDrEF18M1UtjDna9gL5= BWWI0/edit# = > [2] https://github.com/hpc/Spindle >=20 > =3D=3D=3D Reply to [17/20]: =3D=3D=3D >=20 > > la_activity() is not called during application exit, even though > > la_objclose is. > > > > Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu. >=20 > Many thanks for the patch! >=20 > As an outsider's review, the included test does not appear to test the = new functionality noted in the commit message and fixed in the patch. It = merely tests that la_objopen/close pairs are properly generated for all = binaries, not that an la_activity(DELETE) call is made at program = termination. >=20 > Since dlclose is not called in the test body, I believe all that = should be needed is an additional check that la_activity(DELETE) is = called exactly once. >=20 > -Jonathon