This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]