From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: "H.J. Lu" Newsgroups: gmane.comp.lib.glibc.alpha Subject: Re: [PATCH 0/2] nptl: Update struct pthread_unwind_buf Date: Thu, 8 Feb 2018 05:27:43 -0800 Message-ID: References: <20180201205757.51911-1-hjl.tools@gmail.com> <4abf9786-1879-f16c-5a01-3261cd718d63@redhat.com> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" X-Trace: blaine.gmane.org 1518096360 4673 195.159.176.226 (8 Feb 2018 13:26:00 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 8 Feb 2018 13:26:00 +0000 (UTC) Cc: GNU C Library To: "Carlos O'Donell" Original-X-From: libc-alpha-return-90142-glibc-alpha=m.gmane.org@sourceware.org Thu Feb 08 14:25:56 2018 Return-path: Envelope-to: glibc-alpha@blaine.gmane.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; q=dns; s=default; b=KZm0 ZBKcYgVbOkBclrKgCRAdmZrtv1WbQt3DnO119wyZd9qo2ZALs4EeRkgf0WGD3eSZ 6gxT6K6J773+pwUpezM7Jiany9a4nVW3M/8DLTN2a8ETPVjEELgroV9YBD2JI+i4 hCZEGR0hPLWCnQT05g7fo7qXMwtFC5FidsYmYqE= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; s=default; bh=5/EP3jc6Yo QZvNZSjtMNPiW2SOM=; b=dZM4ySkGM13gIxklGgBCrZR7i07oaPbNmKeY1CPUjF CDENI2n+99hvPLHHWtdzFw7IF6f3Nuyr9nJPxPO/3omxpFgRZ+8T6of1/L57nMeI 1TMLLW8fd9vHAOa7K20PjDAric1c8tPxBEBDOyAqZNDgz1V0skPyy51nX+qHn+kQ Y= Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Original-Sender: libc-alpha-owner@sourceware.org Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:6314 X-HELO: mail-ot0-f174.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=qqSMBdu16TjV66Z2Y9h/zIUcubUNvqr8y3JRUUQoeN0=; b=PkJ/ClNtw9V3gJpZPVO/cH6MDM9RgD1bwA+p+9G5cFh50DwLlbq+NP91QGF901NDGs uj73IAmDKt4SmmjOofIpAKWsPmDALxQsVfGPn8OYP/yDbb0JXFv+mWh4CH9bu7ujdB+9 14VrVpR7xIqb+61FKYKV9rOHfv4KSOINg5FMhKSKyKAf57vKvlF4Yh3HDfSpojt42hc8 Xl1FkOLj7/uT5K+B5Tte4RdhonJ9MV5yvS1qBBtPEPT0LUpI4W3CNAJC/f1S3HCnFkdx TK9WcxN5G6N7ej8oL4q30XGlLo8CDt6bGKWNUyKEcGvwNt4/PBPJwiwH7Kzy2ezFZu9z LeLQ== X-Gm-Message-State: APf1xPB58N+3EYBRRelmj6EtvaXg4r+naUFykPdcQ6K2u7GdY3Q4eMIx hOFdAl0xkSS/UUlytJ+A5eU9BZEbEx+QcD+wtDY= X-Google-Smtp-Source: AH8x227CimZ4Z5PJ7Kavp0uH5zRtdl6S8J4kEZEUi5yINmf/Tph2Ij920Hq95X2f1SSDy/yuf1Y7yMKgzkbHsUIxBiw= X-Received: by 10.157.46.90 with SMTP id c26mr556016otd.310.1518096463842; Thu, 08 Feb 2018 05:27:43 -0800 (PST) In-Reply-To: <4abf9786-1879-f16c-5a01-3261cd718d63@redhat.com> Xref: news.gmane.org gmane.comp.lib.glibc.alpha:82490 Archived-At: Received: from server1.sourceware.org ([209.132.180.131] helo=sourceware.org) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ejmCw-0000KO-QM for glibc-alpha@blaine.gmane.org; Thu, 08 Feb 2018 14:25:47 +0100 Received: (qmail 81612 invoked by alias); 8 Feb 2018 13:27:48 -0000 Received: (qmail 81596 invoked by uid 89); 8 Feb 2018 13:27:48 -0000 On Thu, Feb 8, 2018 at 1:25 AM, Carlos O'Donell wrote: > On 02/01/2018 12:57 PM, H.J. Lu wrote: >> struct pthread_unwind_buf is updated to save and restore shadow stack >> register with backward binary compatibility. >> >> H.J. Lu (2): >> Revert "Revert Intel CET changes to __jmp_buf_tag (Bug 22743)" >> nptl: Update struct pthread_unwind_buf [BZ #22743] >> > > High Level: > > To enable Intel CET we need to make sure that all places that change > execution context abruptly also adjust the shadow stack such that the > subsequent return matches what is presently on the stack. The current > setjmp/longjmp are just such functions that change execution context > abruptly, and to use longjmp to return to a setjmp location we must > also restore the shadow stack on the return jump. > > The cancellation push/pop sequences also use a jump buffer, and at a > high-level your solution is to expand those buffers with enough space > (an ABI change) to store the shadow stack pointer. The ABI transition > is managed using the CET note markup, and safety is ensured in that way > as a all-or-nothing ABI change. > > Note that it depends heavily CET markup rolling out smoothly with glibc > 2.28. If any distro enables CET markup without glibc 2.28 then it might > be possible to have objects that use the old-ABI jump buffers in a CET > enabled application and crash. Similarly if users build and use their > own runtime. > > Why not version __pthread_register_cancel instead? The versioned symbol > would seem to me a more robust indicator of the larger jump buffer than > the requirement on coordinating markup + glibc 2.28 in all distros and > for all users? > > I would like to see an argument made for CET markup against versioning > __pthread_register_cleanup. > > Design: > > Commit f33632ccd1dec3217583fcfdd965afb62954203c finds space in the > existing sigset_t by reducing the number of possible signals down to > 513, the kernel is only using 64, and the remaining space is used to > store the 64-bit shadow stack pointer. > > However, this isn't the only place that jump buffers like those used > in setjmp/longjmp are used. Internally in pthread_cleanup_push and > pthread_clenaup_pop the cancellation is handled by a special > structure that is almost identical to the jump buffers used by > setjmp/longjmp. However, because they are not identical, and were > designed to avoid needing to save/restore the process signal mask > during cancellation (as an optimization), these buffers also do > not have enough space to store the shadow stack pointer. > > Therefore there needs to be an expansion in the size of the jump > buffer used for cancellation to accomodate the shadow stack pointer. > > Your patches present the most logical next step here, which is to > make the cancellation jump buffer the same size as the normal jump > buffer used for setjmp/longjmp. > > Changing the cancellation jump buffer is an ABI change though since > the jump buffer is allocated in the application stack via the the > pthread_cleanup_push macro. Some way must be found to determine which > size of jump buffer the caller is using to know if the newer larger > version can be used. > > It is not sufficient to version pthread_create to make this distinction > because an application may have been compiled with a new glibc, get > the new version of pthread_create, and then call a DSO which used the > old pthread_cleanup_push, and so expecte the old-ABI jump buffer. > > It is not sufficient to use priv.pad[3] because on ABIs with 32-bit > pointers like x32 there is insufficient space to store the 64-bit > shadow stack pointer. > > It *is* sufficient to version __pthread_register_cleanup in theory > because on a caller-by-caller basis this would indicate if the caller > had been compiled with the old or new-ABI jump buffer and read > accordingly. > > Your patches take another approach, which is to use the binutils CET > markup to indicate an all-or-nothing ABI transition to the new larger > sized jump buffer. If CET is enabled, then the application would have > been compiled with a CET enabled gcc, CET enabled glibc, and CET > enabled binutils, and the result is a CET enabled application which > is then assumed to have the newer larger sized cancellation jump buffer. > Your solution avoids adding another symbol version and uses the existing > CET markup as the deciding factor. > > Why might we reject versioning __pthread_register_cleanup? It would seem > that since glibc internally controls the pthread_unwind_buf it will never > be passed by pointer to another TU to be used in ways we don't control > (the problem s390x faced when trying to increase the size of jmpbuf > for setjmp/longjmp) e.g. loaded by a newer version of __pthread_register_cleanup > that expects a different sized object. The internals and their use > as expected from pthread_cleanup_push/pop are all in the same translation > unit. > > Again, I would like to see an argument made for CET markup against versioning > __pthread_register_cleanup. Symbol versioning works well when only opaque data pointers are used. A pointer of struct pthread_unwind_buf is passed to libpthread from user programs. Within libpthread, we need to know the exact layout of struct pthread_unwind_buf. It is next to impossible to use symbol versioning here. > Details: > > The name NEED_SAVED_MASK_IN_CANCEL_JMP_BUF while true is slightly > misleading IMO. While it is true that you need the saved mask there, the > actual logical goal is to make the structure match in layout with the > non-cancel jmp_buf. I would rename this to NEED_SETJMP_LAYOUT or something Will do. > like that to indicate the intent is to make the structure match the layout > used by setjmp jump buffers. > > Both UNWIND_BUF_PRIV and THREAD_COPY_ADDITONAL_INFO are exmaples of typo-prone > macro APIs. We should work hard to define defaults and have unconditional > users of these macros such that -Wundef can warn us if we make mistakes > e.g. https://sourceware.org/glibc/wiki/Wundef Will do. > Notes: > - This current patch set passes the ABI tests I ran last time so it is good > to note that your patches do not appear to introduce any other ABI regressions. > Thanks. -- H.J.