-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
S390x backports for 1.1.1 #11188
S390x backports for 1.1.1 #11188
Conversation
I think you want to drop commit f993cc2f80abcac516e6b7b1ef56fc58270a0975 from that list |
The following diff is needed for make update to work (and fix failing travis):
I checked it and the changes are also missing on my ecc111 branch (make update would fail here also). I either lost them when back-fitting the commits from master, or those are needed in addition. |
for upstream yes; for ubuntu no. As I am cherrypicking things from master, that are not present on 1.1.1 branch, my license gets polluted. This is the best way I can request IBM to review what i'm trying to get into Ubuntu s390x. At the moment this is to request a public review of what I'm trying to put into Ubuntu which is basically 1_1_1-stable + a bunch of s390x backports. |
The error constant fixup is basically a fixup for 7f06dc9 : id propose to squash those. The include header fixup is because the 1.1.1 backport of 25f2138 (which is 0c994d5) does not include this change. Reason is, the original header include was introduced in f39ad8d , which is not on 1.1.1. Since c2c567e45b56d8e45466669b930b7c9f26544fd4 is the 1.1.1 backport of said commit, id propose to squash it with the fixup. I ll continue with my review later today. |
Travis is now green, but I'll do another upload into Ubuntu Autopkgtest infra, as the tests there did not look good. |
25f8dbf
to
b3a2685
Compare
Rebased, and squashed together the two commits as suggested. |
I did the following tests:
All tests tests were successful. My only finding is related to additional tests i did for the OPENSSL_s390xcap env variable: I think its the following chunks which are missing:
I propose to squash these changes with 3ed77a3d527a459726cef230d1811488e323a4ff |
for upstream yes; for ubuntu no. As I am cherrypicking things from master, that are not present on 1.1.1 branch, my license gets polluted.
You might not be able to put the 1.1.1 code under the Apache
license, we actually had to remove code that was present in 1.1.1.
And this is an upstream pull request, so we don't want to merge
that commit.
|
Ok, so how do you handle cherry-picks of Apache code from master onto the 1.1.1 branch? All of the code I'm trying to cherrypick above is not mine, and all of it is under Apache license right? Or is everything in master dual licensed? Or can only the original authors cherrypick from master and thus dual-license their contributions? |
a7e9237
to
8afbe3e
Compare
@xnox Looks good to me now and passes all tests. See also p-steuer#1 (comment) . |
For all new contributions we have a CLA, allowing us to put a
license on it. So if we merge it to master, it's under the Apache
2 license, if we merge it to older branches it's under the old
OpenSSL license.
|
afaik not the author but the copyright owner could always chose to do that. But as Kurt mentioned, this case should be covered by the cla anyway. |
I discussed with @xnox about actually trying to get this merged to 1_1_1-stable (i.e. really treating HW acceleration as fixes): backports based on a single upstream stable branch instead of many different distro packages would IMO benefit both HW vendors and distributors. See discussion here p-steuer#1, especially comments p-steuer#1 (comment) and p-steuer#1 (comment) . I would also like to hear @t8m 's point of view. |
We already treat performance fixes as bug fixes. However a completely new implementation is a little bit different beast so I am not sure. With my distro hat on, I would be fine with it, but it is stretching the openssl release policy. |
We have in the past also done such changes.
|
What a nice pun! :-) |
8afbe3e
to
1d39b9a
Compare
Rebased on top of current 1.1.1-stable branch. |
There are two separate CLA issues. Firstly cla-check is detecting that you committed these changes (even though someone else authored them), and is complaining that there isn't a CLA on file for you. The simplest solution to this is for you to just submit a CLA. The second issue is a missing CLA for Paul Yang (@InfoHunter). The reason is that you have cherry-picked a commit that was pushed under a CLA that was valid at the time it was pushed. That CLA is no longer valid for new contributions (there is a replacement CLA for Paul). The cla-bot isn't clever enough to detect that this is actually a backport of something that is legitimately in master - and just sees this as an invalid CLA. We might need an OMC member to temporarily re-enable that CLA when it comes to pushing this to workaround that problem. Or alternatively someone goes and fixes the cla bot to be a bit more intelligent. |
The only commit by paul that is included is aa65b5f895d0e9d58ee2d27796a6965793f112af which just moves the s390x man page's return value section above the example section. I think you could just drop it ? Wrt to committing changes that i authored: Im okay with that. |
1d39b9a
to
a809253
Compare
Signed-off-by: Patrick Steuer <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#10004) (cherry picked from commit b3681e2) Signed-off-by: Dimitri John Ledkov <[email protected]>
Signed-off-by: Patrick Steuer <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#10004) (cherry picked from commit ac037dc) Signed-off-by: Dimitri John Ledkov <[email protected]>
using PCC and KDSA instructions. Signed-off-by: Patrick Steuer <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from openssl#10004) Signed-off-by: Patrick Steuer <[email protected]> (cherry picked from commit 56eb364ef5cf93e2658cd45fcffee435b80857b2) Signed-off-by: Dimitri John Ledkov <[email protected]>
x25519 has such a test vector obtained from wycheproof but wycheproof does not have a corresponding x448 test vector. So add a self-generated test vector for that case. Signed-off-by: Patrick Steuer <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#10339) (cherry picked from commit fd60f8da74c68ba56f828bcc59141856503ffa0a) Signed-off-by: Dimitri John Ledkov <[email protected]>
087be09
to
bf34f39
Compare
@mattcaswell the discussion takes a very different path, to what LTS means in Ubuntu. And the wikipedia article referenced, also doesn't quite live up to what Ubuntu means by LTS which is the first distro to introduce it. For example, the current OpenSSL -stable branches fail to meet Ubuntu's LTS requirements and we do not take stable branches updates as a whole to the latest release as a letter. Yet at the some time, the changes we do cherrypick into our LTS releases do not meet OpenSSL Upstream requirements either. It seems to me that there is a significant impedance mismatch, and people mix different definitions of what stable means, and when.
There are desires for all sorts of things, from various people. For example, a lot of deployments desire absolute stability want no changes at all. Such deployments typically operate against frozen package sets, do not apply updates, and/or fork things to only change things they want. Whilst accepting the fact they have bugs and security vulnerabilities, as they value no change in behavior over anything. Some requests from such deployments are for livepatching libssl library in memory of actively running daemons, to ensure no downtime or even interruption in serving requests or active connections. There is not much to do here, as typically such deployments freeze on a particular build of OpenSSL and just keep using it. Some deployments, with good CI/CD, typically want all the bug fixes, as long as there is no ABI / API changes, such that updates may trigger rolling restarts/reboots, but not rebuilding software. Ie. something along the lines of drop-in updates. Even regressions are ok, as long as they are identified and fixed up quickly enough, and they can update to newer git commit of the stable branch. Currently OpenSSL project does not cater these deployments well, because release schedule of stable branches is not predictable, but only dictated by it seems security CRDs and regressions those cause. It would be nice if there was a stable release cut for LTS ABI every 8th Tuesday. Security-only updates. A lot of deployments are in between the above two. They don't care about bugfixes, but only care to apply security updates out of caution or compliance. Currently OpenSSL project does not cater these deployments well. It would be nice, if every point release of the stable LTS ABI had a security-only branch of fixes, released as patch only (i.e. not even tarballs). I.e. 1.1.1a-security, 1.1.1b-security, 1.1.1c-security git branches, which only contained CVE bugfixes as clean cherry-picks, as small as possible, without any other changes that are unrelated. Possibly even just High and Critical, no Mediums or Lows. Many distributions make distinction between security and non-security updates, prioritize them differently, and have longer support timeframes for security, etc. For example, it would be nice if 1.1.1[adg]-security (every third point release) had a 20 year support time-frame, New Hardware, stable ABI, cost per watt. When one runs out of capacity and needs more hardware, typically new-shiny hardware is purchased or services are rotated onto. Or for example it is not cost-effective to buy old hardware as it's simply is no longer available for sale. But because the deployment is not homogenous, one often wants to deploy software stack with the same stable LTS ABI. But at the same time, squeeze everything out of the available hardware that one possibly can because money was spent on shiny metal. To support these very large deployments, they want ABI stability yet any performance improvements they can possible get, whilst not breaking the ABI. This is especially true during the early timeframe of a stable LTS ABI, whilst no next one is available yet. One is stuck between wanting the performance that is available in v3/master, yet requiring -security availablility of 1.1.1 LTS ABI. These deployments don't even usual care about bugfixes. But really performance improvements only. Or for example if a bugfix regresses performance, they will concider it to be a savere regression and rollback and switch to the "no-changes" camp. An Ubuntu LTS release goes through multiple stages over it lifecycle. At first, it caters for all of the above use cases. That is for the first 2 years, via -security and -updates streams. Once the next LTS is out, performance backports stop (no more HWE updates). Then for the next 3 years we provide bugfixes and security updates. Then for the next 5 years we provide select security updates for high and critical only. And then only extended security maintenance is available for an even smaller subset of packages. In terms of naming, during first 2 years we push out point releases with updated HWE stacks (Hardware Enablement). Then point releases stop, and we only do USN & SRU (ubuntu security notice, and stable release updates). Then we reach end of basic support, start of extended security maintainance, meaning SRUs stop and only USN continue. And then eventually we reach end of ESM. The policy of what goes into -stable branches, does not need to be fixed in time, and stay the same and forever. A lot of hardware vendors, added a lot of performance and hardware specific features in v3/master. And that release has slipped. So a lot of people want to use v3-only performance patches with stable LTS 1.1.1 ABI. And there is nothing that can cater for them at the moment. It would be useful for example to cut 1.1.1g-security branch which will only have security patches applied without any bugfixes or perfomance fixes. And accept bugfixes + performance changes into 1.1.1-stable, until v3 is out. Once v3 is out, only accept security fixes into 1.1.1-stable branch and nothing else, no more bugfixes and no more performance improvements. I understand this is very different from what was done before, but nobody wants intermixed security patches with bugfixes with performance improvements. As each type of changes, have risks of introducing regressions which are not worth for the end user if they don't care about that type of changes. |
Separately from the 1.1.1-stable policy discussions, I'd like to request exception on how the s390x port is handled in particular. The set of maintainers for it is small, and are common between upstream and downstream distributions. And at the moment, there have been two mainframe generations with better crypto acceleration, that are supported by master/v3, but not by 1.1.1 series. I would like to request exception to the 1.1.1-stable branch policy to backport hardware acceleration patches from master to 1.1.1-stable to enable exploiting the performance features that z14 and z15 offer. I and s390x port maintainers commit to QA and support of these backports, and regressions that may arise from doing picking them up. How can I apply for such exception to the 1.1.1-stable rules, without changing the rules for other ports / features? |
It would be nice if there was a stable release cut for LTS ABI every 8th Tuesday.
We have been doing stable releases about 3 months apart, even when
there wasn't a security issue that needed to be released. We
didn't announce this, nor do we always properly follow up on this.
So you're suggesting that we actually make a policy of this?
|
Yes please. Predictable regular point release, create cadence, for everyone to align with. It will make it a lot easier to consume those releases. And make them quicker adopted. It would be nice if security CRD happens there is an extra release in between the cadence. But make it just the security patch only, as a clean patch on top of last point release only, which is merged into stable, and will be rolled up with stable fixes when the regular cadence happens.
Hopefully that makes sense. That would make it a lot easier to ship security updates. Because those that are already on (d) release, will take (e) straight away, becuase it contains security patch only. Whereas "regular cadence" (f) release will have rolled security fixes and bufixes. But is not as urgent to upgrade to if one is on (d) or (e) already, and happy with it. ps. I guess 1.1.1d.1, 1.1.1d.2 are security only, whereas 1.1.1f is regular cadence stable release with both security and bug-fixes. And eventually stop doing bug-fixes, and continue security-only. |
You folks do realize that there are only two full-time developers, and under a dozen volunteers, right? |
And 3 part-time people, maintaining this patchset on top of older snapshots of 1.1.1-stable in three distributions. Yes, we want to minimize the amount of total duplicated maintainance there is of this code for the 1.1.1-ABI globally. To free up time, to do interesting things with OpenSSL. Because backporting patches over and over again, and maintaining conflicts / rebases, is not productive. Sharing stable trees, with common features that all distros ship is the way to go. |
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
That discussion evolved and splitted in 2 parts (as mentioned here):
@mattcaswell given you put the OTC hold, will you call the OTC vote on this? Should we wait until after |
BTW, there's no reason that OTC couldn't come up with a suggested policy for the OMC's consideration if someone wants to create a strawman proposal.
I can call a vote on that if its helpful. The current policy is mentioned on the release strategy page as follows: "Letter releases, such as 1.0.2a, exclusively contain bug and security fixes and no new features" Suggested wording for the vote: "The performance improvements provided in PR11188 should be considered a bug fix and therefore acceptable for backport to 1.1.1" |
I have started the above mentioned vote and will report back here when we have a result. |
There has only one vote so far. Is there a way to reping voters? |
Votes automatically close after 2 weeks unless explicitly closed earlier. This actually closed last week but I forgot to report it back. As a reminder the vote text was: "topic: The performance improvements provided in PR11188 should be considered a bug fix and therefore acceptable for backport to 1.1.1" The vote was not accepted: for: 0, against: 8, abstained: 3, not voted: 0 So, the OTC considers this PR to not be in conformance with current policy. The only way forward would be to change the policy, or to gain an explicit exemption for this PR (or abandon the PR). |
Do we need to call an OTC vote to recommend OMC to consider an explicit exemption? Or is someone from the OMC willing to start a vote to decide on such an exemption? |
IMO, I think such a vote is unlikely to pass - although there's nothing stopping you raising it if you want to go down that path. I think a better way is to clarify and extend the existing policy. I suggest that someone creates a strawman proposal (preferably as a web PR). I'm undecided whether it is best to get OTC to vote on it first before passing it to OMC, or just go straight to OMC. Ultimately it will be an OMC decision I think. |
I think the policy amendment should be OMC vote straight out. The OTC members can voice their concerns or support on the to-be-proposed PR. |
It always seems to me to be unnecessary bureaucracy to have OTC vote to recommend something to OMC to do a vote about the matter. |
Thank you for completing the vote! In light of rejecting this PR, to reduce maintenance burden in Ubuntu, 20.04 LTS and 20.10 will thus remain on 1.1.1f point release plus cherry-picks. As I do not wish to maintain rebases of this code, together with all our other patches against multiple point releases, without these patches getting merged upstream to stable series. The next major/minor bump of OpenSSL in Ubuntu will therefore be v3.0.0 once that is released as stable upstream, as that will be the first upstream release that will contain this PR, among other cherrypicks we already carry in 1.1.1f. |
Thanks for completing the vote. I still hope we can come up with a better solution regarding HW enablement patches and LTS releases in the future. |
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
@p-steuer the @openssl/otc has discussed the possibility of having LTS+ releases accompanying LTS releases in the future, where such enhancements could find a place to go. But AFAIK the details have not been worked out yet. Also, since it would be a policy change, it would require an OMC discussion and vote. |
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
@p-steuer
this rebases existing ubuntu s390x patches on top of OpenSSL_1_1_1-stable branch
... cherrypicks more things from master
... or from your ecc111 branch, which was first rebased on top of OpenSSL_1_1_1-stable
I'll try to ship all of this into ubuntu, if it like builds and passes tests. So far I have only statically assembled this, without like even test building it.