Re: V3 [PATCH 03/24] x86: Support IBT and SHSTK in Intel CET [BZ #21598]

[Carlos O'Donell <carlos at redhat dot com>]

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.  
> It looks like there are new tunables added, at least according to the 
> commit message, which also need documenting in the glibc manual.  In 
> 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.

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.

-- 
Cheers,
Carlos.