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

[Joseph Myers <joseph at codesourcery dot com>]

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'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.

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.

-- 
Joseph S. Myers
joseph@codesourcery.com