Skip to content
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

vendor: move to snapshot-4c814e1 branch and set fixed KDF options #10591

Merged
merged 3 commits into from
Aug 9, 2021

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Aug 5, 2021

This commit moves our secboot code to the snapshot-4c814e1 branch
that contains fixes around the KDF benchmarking. This will improve
the install performance.

This commit moves our secboot code to the `snapshot-4c814e1` branch
that contains fixes around the KDF benchmarking. This will improve
the install performance.
@mvo5 mvo5 added this to the 2.51 milestone Aug 5, 2021
@mardy
Copy link
Contributor

mardy commented Aug 6, 2021

Looks like we need to adapt our code, too. Let me know if you are busy and would like me to do it.

@pedronis pedronis self-requested a review August 6, 2021 07:25
@mvo5 mvo5 changed the title vendor: move to snapshot-4c814e1 branch with KDF fixes vendor: move to snapshot-4c814e1 branch and set fixed KDF options Aug 6, 2021
@mvo5 mvo5 requested a review from chrisccoulson August 6, 2021 08:31
Copy link
Contributor

@chrisccoulson chrisccoulson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok to me

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but one doubt

@@ -53,6 +53,10 @@ func (s *encryptSuite) TestFormatEncryptedDevice(c *C) {
c.Assert(opts, DeepEquals, &sb.InitializeLUKS2ContainerOptions{
MetadataKiBSize: 2048,
KeyslotsAreaKiBSize: 2560,
KDFOptions: &sb.KDFOptions{
MemoryKiB: 32768, TargetDuration: 0,
ForceIterations: 4, Parallel: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the cryptsetup docs is not super clear if Parallel must be set as well when setting ForceIterations or not. I suppose we need to test the effect of this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to be set - it defaults to the maximum (4) if it's unset. In all cases, it's then adjusted down if there are fewer CPUs. The only requirement is that you can't try to set it to zero.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for context:
which probably translates to 4 or 1 in real world I'd imagine. Apart of intel there are not many under 4 cores devices. On arm we either see 4+ or 1 in very low power devices.

Copy link
Contributor

@kubiko kubiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@@ -53,6 +53,10 @@ func (s *encryptSuite) TestFormatEncryptedDevice(c *C) {
c.Assert(opts, DeepEquals, &sb.InitializeLUKS2ContainerOptions{
MetadataKiBSize: 2048,
KeyslotsAreaKiBSize: 2560,
KDFOptions: &sb.KDFOptions{
MemoryKiB: 32768, TargetDuration: 0,
ForceIterations: 4, Parallel: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for context:
which probably translates to 4 or 1 in real world I'd imagine. Apart of intel there are not many under 4 cores devices. On arm we either see 4+ or 1 in very low power devices.

@mvo5 mvo5 added the Squash-merge Please squash this PR when merging. label Aug 9, 2021
Copy link
Member

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mvo5 mvo5 merged commit 7e4934a into snapcore:master Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked Squash-merge Please squash this PR when merging.
Projects
None yet
6 participants