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

util: Limit cryptsetup PBKDF memory usage (backport #3781) #3784

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Apr 27, 2023

This is an automatic backport of pull request #3781 done by Mergify.


Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

@Madhu-1 Madhu-1 added ready-to-merge This PR is ready to be merged and it doesn't need second review (backports only) ci/retry/e2e Label to retry e2e retesting on approved PR's ok-to-test Label to trigger E2E tests labels Apr 27, 2023
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.23

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.26

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.26

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@github-actions github-actions bot removed the ok-to-test Label to trigger E2E tests label Apr 27, 2023
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Apr 27, 2023

@Mergifyio rebase

@mergify
Copy link
Contributor Author

mergify bot commented Apr 27, 2023

rebase

✅ Branch has been successfully rebased

@Madhu-1 Madhu-1 force-pushed the mergify/bp/release-v3.8/pr-3781 branch from 82459ea to bd4c896 Compare April 27, 2023 12:13
@Madhu-1 Madhu-1 added the ok-to-test Label to trigger E2E tests label Apr 27, 2023
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.26

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.27

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

@mergify[bot] "ci/centos/k8s-e2e-external-storage/1.27" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

@mergify[bot] "ci/centos/mini-e2e-helm/k8s-1.27" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

@Mergifyio requeue

@mergify
Copy link
Contributor Author

mergify bot commented Apr 27, 2023

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@Madhu-1 Madhu-1 removed the ci/retry/e2e Label to retry e2e retesting on approved PR's label Apr 27, 2023
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Apr 27, 2023

@Mergifyio rebase

By default, `cryptsetup luksFormat` uses Argon2i as Password-Based Key
Derivation Function (PBKDF), which not only has a CPU cost, but also a memory
cost (to make brute-force attacks harder).

The memory cost is based on the available system memory by default, which in
the context of Ceph CSI can be a problem for two reasons:

1. Pods can have a memory limit (much lower that the memory available on the
   node, usually) which isn't taken into account by `cryptsetup`, so it can get
   OOM-killed when formating a new volume;
2. The amount of memory that was used during `cryptsetup luksFormat` will then
   be needed for `cryptsetup luksOpen`, so if the volume was formated on a node
   with a lot of memory, but then needs to be opened on a different node with
   less memory, `cryptsetup` will get OOM-killed.

This commit sets the PBKDF memory limit to a fixed value to ensure consistent
memory usage regardless of the specifications of the nodes where the volume
happens to be formatted in the first place.

The limit is set to a relatively low value (32 MiB) so that the `csi-rbdplugin`
container in the `nodeplugin` pod doesn't require an extravagantly high memory
limit in order to format/open volumes (particularly with operations happening
in parallel), while at the same time not being so low as to render it
completely pointless.

Signed-off-by: Benoît Knecht <bknecht@protonmail.ch>
(cherry picked from commit 1852e97)
@mergify
Copy link
Contributor Author

mergify bot commented Apr 27, 2023

rebase

✅ Branch has been successfully rebased

@Madhu-1 Madhu-1 force-pushed the mergify/bp/release-v3.8/pr-3781 branch from bd4c896 to 10384ab Compare April 27, 2023 14:19
@Madhu-1 Madhu-1 added the ok-to-test Label to trigger E2E tests label Apr 27, 2023
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.26

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.27

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.27

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.27

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@github-actions github-actions bot removed the ok-to-test Label to trigger E2E tests label Apr 27, 2023
@Madhu-1 Madhu-1 added the ci/retry/e2e Label to retry e2e retesting on approved PR's label Apr 27, 2023
@mergify mergify bot merged commit 0ec6e10 into release-v3.8 Apr 27, 2023
@mergify mergify bot deleted the mergify/bp/release-v3.8/pr-3781 branch April 27, 2023 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/retry/e2e Label to retry e2e retesting on approved PR's ready-to-merge This PR is ready to be merged and it doesn't need second review (backports only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants