How do we review the code nobody understands?
https://github.com/openssl/openssl/pull/28990 is an example of such PR. It's huge, it requires specific ASM knowledge, and the only thing that can be done by me is a check that the tests pass on the relevant platform (they do).
As the patch closes a real-world issue, I think it needs to be dealt with, we can't just ignore it. So what is the way forward in this case?
This is a community effort (in this case driven by IBM folks, if I understand correctly), and I think we have to be able to trust what they're doing, and be able to trust other people that do verify the PR for us, as is the case here.
What remains for us to do is to verify that nothing particularly nefarious slips in (remember the xz issue from not so long ago). I just eyed through the PR and couldn't find any such thing.
If we cannot trust the community that's willing to put in the work, then I think we've lost something.
I'm sure the NSA would love a more trusting world.
The diametrically opposite option would be to encourage the IBM folks to make their own provider for PPC. With appropriate properties, that should be workable, no?
(if we had had the attitude that we must understand everything, then the VMS work I used to do would probably not have gone in very smoothly at all)
@Shane Lontis we have an overall problem with reviews (and I'd like to nominate more people to make reviews) but this is a specific case for which we have the situation even worse.
I think @Richard Levitte is right here. OpenSSL is sufficiently large and complex that it is impractical to expect that core staff (which I would define as paid members of the Foundation and Corporation) understand "everything" to the point where quality reviews on the contents of every patch can be reasonably expected.
My past kernel experience suggests this is manageable by a community trust and participation mechanism. Specifically they use a MAINTAINERS file, which records an identity and set of file regexs for which that identity is considered a responsible expert. When a patch is posted to LKML, the responsible tree maintainer optionally looks to the owner in the MAINTAINERS file for input/guidance/confirmation prior to accepting the patch.
I think we could do something similar:
1) If a contributor makes contributions regularly to an area of the project, they can be nominated via a PR to augment the MAINTAINERS file (or can self nominate via the same method)
2) When a new PR is opened that affects code overlapping with an entry in the MAINTAINERS file, that identity is added to the reviewers list for the PR.
3) People in the MAINTAINERS file are expected to participate by reviewing and approving those PR's to which they are assigned. Acceptance of the PR is gated on at least one approval from a MAINTAINER listed identity, in addition to our usual 2 committers reviews.
The hope would be that we would build up a list of trusted community members recognized as subject matter experts in an area of code that the rest of the project can trust their expertise. In the event that the listed maintainer is the person submitting the PR for which there are no other maintainers, we at least have a baseline level of trust that this person knows what they're doing. If there is no maintainer for a block of code...well, we're no worse off than we are know, but we can clearly identify that lack of subject matter expertise and go soliciting to find said expert.
@nhorman, isn't that what github calls CODEOWNERS? It sounds like a different beast, but in practice, it sounds like it supports approximately the same functionality MAINTAINERS does with the Linux kernel.
@Richard Levitte I think thats roughly it, yes, though I'm not sure of minutiae of the implementation differences, or if it matters. Assuming CODEOWNERS is sufficiently flexible, I'd be ok with using it
@Neil Horman It seems this is exactly what we have already done in practice for https://github.com/openssl/openssl/pull/29137 - 3 developers who have contributed RISC-V code are asked for help reviewing this PR. They should be put into a MAINTAINERS file of RISC-V realm. Add their approvals should count in addition to another 2 committers. So the trust model of this is: committers --trust-> maintainers --review--> code (that committers aren't familiar with)
@Paul Yang yes, I agree, we seem to be doing this somewhat organically (if not consistently) already. I'd just be looking for a level of formalization, if for no other reason than to give new people the project a way to find/identify who those codeowners/maintainers are. In so far as approvals counting goes, that probably needs some discussion as to the actual mechanism (i.e. we would need some tooling changes so that people in code owners could be treated like comitters for the purposes of approval without giving them write access to the tree), but in general, I agree with the approach.
@Neil Horman Absolutely, this way is a very effective collaboration method of a 'community' for involving more people in
Has AI review been considered (i.e. copilot in GitHub)?
@Todd Short I highly agree with this
@Paul Yang I'm not opposed to adding copilot or codeQL or some such to our review process, but I would personally have to see some effective reviews from an AI before I placed anything close to the same trust that I would place in a maintainers file set of people to it.
I wasnt suggesting that OpenSSL resources need to know 'everything', it could be divided per platform for example.
I would suggest that if we are relying on code coming in from external sources for assembly that we should be a bit more strict when reviewing in terms of asking for more documentation/comments explaining the code and techniques used. A fair proportion of the contributors in this space seem to disappear over time.
It's possible to formally prove various things about the assembler. This will require a large effort at the start. But the result is going to be that we have a higher trust that the code does the right thing in corner cases.
Any conclusion on the maintainers file? For instance, how could a new trusted maintainer be added to the list or so
This kind of problem was raised during the recent Corporation BAC and TAC meeting. Unless OpenSSL dedicates resources towards this I dont see the reviews getting better. It requires not just knowing different assembler, it also involves delving into algorithms that are quite often heavily optimized that are reasonably hard to understand. Combine this with a general lack of comments and references/motivations, and you end up with a black box. This is further complicated by edges cases, platform version specific problems, and other problems such as restoring correct registers and alignment issues. It can take considerable time for a reviewer to do this properly, unless they are working actively in this space.