From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf2d.google.com (mail-qv1-xf2d.google.com [IPv6:2607:f8b0:4864:20::f2d]) by sourceware.org (Postfix) with ESMTPS id 252643858405 for ; Mon, 20 Dec 2021 12:10:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 252643858405 Received: by mail-qv1-xf2d.google.com with SMTP id h5so7352525qvh.8 for ; Mon, 20 Dec 2021 04:10:30 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=Htahw+7Sz+joXjN2tcR8jLNhoYH/aBT9GBn6pZ8HRGA=; b=0ogn9nvT5rSANsWIveK4K/QSNhEYUgvZzgBv4wVMZ57LUjJgg1cLZWWDc1KCo1Bl4C sZw+FT3iLHffUCnm0oPOXAUSKMSKJ61UE/AUd5wt8LaBBGDEy9nhCN0y3uklSY4yfN+m ipD8KS4dGkM+eSy+45u5D2jMr1W7lKFsxnWFbXFt9TfyovCymul4dw3D4dAhPUcl8TLG WrznN+ZIYcwshajigXrsuK4hNAL0x0kCSBOOmJNVjPCBTfahqPAkh39eET4uD4LEF0bJ rQ2esqz4rQkByGmCbXUsKDas6GXgKYLXZ3Ji1JH460egsDPDr0lUwKbvmdTIh16Af20c LTcA== X-Gm-Message-State: AOAM531rEqwbUc/Q/4Ll/nGHh+G/inYs8AHobKBJ1oy5j3edH0yWkBtM 0xwU45QCApsVjmlb17+Xv+ATmA== X-Google-Smtp-Source: ABdhPJyfGzvKI8UBHhvZZhosTCvEt4jNMxOn6LofyWPQbklqPuED3OIdduasJsiYmW+WccteW7mWLg== X-Received: by 2002:a05:6214:2a:: with SMTP id b10mr12076654qvr.29.1640002229218; Mon, 20 Dec 2021 04:10:29 -0800 (PST) Received: from ?IPV6:2804:431:c7cb:3b1e:762b:24f5:94b:4e15? ([2804:431:c7cb:3b1e:762b:24f5:94b:4e15]) by smtp.gmail.com with ESMTPSA id d19sm14686942qtb.47.2021.12.20.04.10.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Dec 2021 04:10:29 -0800 (PST) Message-ID: <96f36ce7-4ea0-9175-965f-fd84f23b1ba4@linaro.org> Date: Mon, 20 Dec 2021 09:10:26 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH v6 10/20] elf: Avoid unnecessary slowdown from profiling with audit (BZ#15533) Content-Language: en-US To: Florian Weimer Cc: libc-alpha@sourceware.org, John Mellor-Crummey , Ben Woodard , Alexander Monakov References: <20211115183734.531155-1-adhemerval.zanella@linaro.org> <20211115183734.531155-11-adhemerval.zanella@linaro.org> <87lf0hhhuh.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella In-Reply-To: <87lf0hhhuh.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-15.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Mon, 20 Dec 2021 12:10:31 -0000 On 18/12/2021 15:45, Florian Weimer wrote: > * Adhemerval Zanella: > >> The rtld-audit interfaces introduces a slowdown due to enabling profiling >> instrumentation (as if LD_AUDIT implied LD_PROFILE). However, instrumenting >> is only necessary if one of audit libraries provides PLT callbacks ( >> la_pltenter or la_pltexit symbols). Otherwise, the slowdown can be avoided. > > Awkward linebreak after (. Ack. > >> To keep la_symbind() to work even without PLT callbacks, _dl_fixup now > >> +* The audit libraries will avoid unnecessary slowdown if it is not required >> + PLT tracking (by not implementing the la_pltenter() or la_pltexit() >> + callbacks). > > I think we don't use the () markers? (Although I said before I wouldn't > point this out anymore.) I though I have removed all of them, I will fix this as well. > >> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c >> index 0d5b727c64..64a96c36e8 100644 >> --- a/elf/dl-reloc.c >> +++ b/elf/dl-reloc.c >> @@ -205,12 +205,28 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[], >> int skip_ifunc = reloc_mode & __RTLD_NOIFUNC; > >> #ifndef PROF >> - if (__glibc_unlikely (consider_profiling) >> + if (consider_profiling | consider_symbind >> && l->l_info[DT_PLTRELSZ] != NULL) >> { >> /* Allocate the array which will contain the already found > > Please add parentheses around the | expression (which should perhaps > use || too). > Ack. >> diff --git a/elf/dl-runtime.c b/elf/dl-runtime.c >> index 03da689503..9a38eea7cc 100644 >> --- a/elf/dl-runtime.c >> +++ b/elf/dl-runtime.c >> @@ -131,6 +131,37 @@ _dl_fixup ( > >> - /* Determine whether any of the two participating DSOs is >> - interested in auditing. */ >> - if ((l->l_audit_any_plt | result->l_audit_any_plt) != 0) >> - { >> - unsigned int flags = 0; >> - struct audit_ifaces *afct = GLRO(dl_audit); >> - /* Synthesize a symbol record where the st_value field is >> - the result. */ >> - ElfW(Sym) sym = *defsym; >> - sym.st_value = DL_FIXUP_VALUE_ADDR (value); > > All this was copied over in patch 06/20 of the series to dl-audit.c. Ack, I have moved this to the 06/20 patch. > >> diff --git a/elf/tst-audit19a.c b/elf/tst-audit19a.c >> new file mode 100644 >> index 0000000000..36b781f9be >> --- /dev/null >> +++ b/elf/tst-audit19a.c > >> +#include >> +#include >> + >> +/* We interpose the profile resolver and if it is called it means profiling is >> + enabled. */ >> +void >> +_dl_runtime_profile (ElfW(Word) addr) > > I don't think this works. _dl_runtime_profile is not an exported > symbol, so it can't be interposed. > > Maybe you can check for an allocated l_reloc_result instead. If it's > not there, profiling isn't possible. It's not entirely equivalent, but > it provides at least some testing. Indeed, I am not sure why I haven't see it that removing the segfault does not make the test fail. I have added a l_reloc_result check instead. > >> diff --git a/elf/tst-auditmod19a.c b/elf/tst-auditmod19a.c >> new file mode 100644 >> index 0000000000..2296382a1c >> --- /dev/null >> +++ b/elf/tst-auditmod19a.c >> @@ -0,0 +1,23 @@ > >> +unsigned int >> +la_version (unsigned int version) >> +{ >> + return version; >> +} > > This should use LAV_CURRENT. > Ack. > >> diff --git a/elf/tst-auditmod19b.c b/elf/tst-auditmod19b.c >> new file mode 100644 >> index 0000000000..52bb88c7d7 >> --- /dev/null >> +++ b/elf/tst-auditmod19b.c > >> +unsigned int >> +la_version (unsigned int version) >> +{ >> + return version; >> +} > > Likweise. Ack. > > Thanks, > Florian >