From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20090 invoked by alias); 18 Jul 2018 03:32:31 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 19998 invoked by uid 89); 18 Jul 2018 03:32:22 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-oi0-f66.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=AO0gt5Uap7rGHClGlwdnXGF7Dw6Mc4GJwCQSclSXbQY=; b=CXtQPJ3LBWETiFro/AubfA77hiaDe9yTV+6WMFnaz/hdLEfnY/GvHqXz6GBXFUwJ8a CPnKUF+IDP3USu3DewP5Gm3TEUwiAGWfmHRgmCwzLwPrcnv586jfiVai4AGe8k0JLv7r sx9simUTe5Y/Buia3NTpOihgCdbyrfPf1VcY73r0hOVAY9Yoho4SMsnwbUY5wT331qsH lqlj8muFyVcm3AZgugMne01WUFIbKG5xubTYn3Qbafg4yid4VzuqvQnPFM+xsZnuSosP mIM7m9UGBY74oJfbTGsvnzKOAhJ3LgfXC8E7+2JGJ/SFQ/6hULIAN88WGpMFWlkkCKkE 7VaQ== MIME-Version: 1.0 In-Reply-To: <21dc4ff1-4729-d448-d2d8-0a58f51546e7@redhat.com> References: <21dc4ff1-4729-d448-d2d8-0a58f51546e7@redhat.com> From: "H.J. Lu" Date: Wed, 18 Jul 2018 03:32:00 -0000 Message-ID: Subject: Re: V3 [PATCH 03/24] x86: Support IBT and SHSTK in Intel CET [BZ #21598] To: "Carlos O'Donell" Cc: Joseph Myers , GNU C Library Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2018-07/txt/msg00531.txt.bz2 On Tue, Jul 17, 2018 at 5:22 PM, Carlos O'Donell wrote: > On 07/17/2018 06:53 PM, Joseph Myers wrote: >> On Mon, 16 Jul 2018, Carlos O'Donell wrote: >> >>> This version is OK. >> >> The documentation omissions need to be fixed for the release. I see no >> install.texi update / INSTALL regeneration for the new configure option >> and no NEWS update for the new feature and associated configure option. Hi Joseph, can you review https://sourceware.org/ml/libc-alpha/2018-06/msg00328.html Thanks. >> It looks like there are new tunables added, at least according to the >> commit message, which also need documenting in the glibc manual. In Sorry for that. Here is the patch to document tunables: https://sourceware.org/ml/libc-alpha/2018-07/msg00525.html >> general, any information from the commit message that is relevant to users >> or builders of versions of glibc with this feature (as opposed to being >> only information for developers of glibc, or ABI information that belongs >> in an external ABI document, or only about the change rather than about >> the feature as it exists after the change) needs to go in an appropriate >> place in the manual, not just in the commit message. >> >> At least the omission of documentation from some recent function additions >> has the justification that none of the documentation for *at functions, >> wherein it would need to go, exists at present. There is no such reason >> for omitting documentation for configure options or tunables. I don't >> think we want changes going in without documentation updates (in the >> manual, NEWS etc., as appropriate) or a rationale for the absence of such >> updates clearly specified in the commit message - and being in the freeze >> period should make our standards for such things *higher*, not lower. > > I agree, and I will make sure this is covered for Intel CET. > >> I'm also concerned that a large amount of complicated code added by this >> change (or earlier CET changes) appears to have no execution tests (the >> patch adds just one test, one that inspects object properties rather than >> executing any of the affected code), so making it effectively >> unmaintainable. I realise some would be covered by the normal testsuite, >> at least for a --enable-cet build running on a CET-enabled processor and >> kernel, and that some tests may only be effective on CET-enabled >> processors and kernels that most people don't have yet. But I'd expect >> there to be some execution tests for the various cases of mixed CET >> properties in different executables and shared libraries, for example - >> generically, each piece of CET support in glibc needs to be considered for >> testability, and either have tests added, or be explained as tested >> automatically for certain glibc build configurations, or be justified as >> excessively hard to test. Here is a patch to test mixing CET executables and non-CET libraries: https://sourceware.org/ml/libc-alpha/2018-07/msg00526.html > My opinion is that the testing can grow organically from the initial support > as Intel and the community invest in CET support. > >> In short, the code *present* in the patch may have been thoroughly >> reviewed, but the unjustified *omissions* of documentation and testcases >> mean I do not think it was ready to include in glibc or is appropriate to >> include in a release unless those documentation and tests are added soon. > > I think the documentation is important and should go in immediately, for > any tunables or new configuration options. > > My opinion is that the testing can be added incrementally and organically > as we continue to support the new feature. Lack of full coverage testing > should not block inclusion into the release. There are 2 kinds of CET tests: correctness and effectiveness. I am focusing on correctness for now. That is all glibc tests should pass on CET machines when glibc is configured with --enable-cet. Also mixing CET executables and non-CET libraries should work on CET machines. I think I have all covered. Please let me know if I missed anything. BTW, I am holding off the final set of patches on: https://github.com/hjl-tools/glibc/tree/hjl/cet/master starting at commit 1edb466caec037bdf9c714bbd38ceef11ba07971 Author: H.J. Lu Date: Sat Jul 7 07:33:04 2018 -0700 x86/CET: Extend arch_prctl syscall for CET control We are disusing with kernel community for the final syscall decision. Thanks. -- H.J.