This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: V3 [PATCH 03/24] x86: Support IBT and SHSTK in Intel CET [BZ #21598]
On Tue, Jul 17, 2018 at 5:22 PM, Carlos O'Donell <carlos@redhat.com> 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 <hjl.tools@gmail.com>
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.