Refine proc mounts entries traversal

Bug #2054390 reported by Chengen Du
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
cryptsetup (Ubuntu)
Status tracked in Plucky
Focal
In Progress
Undecided
Chengen Du
Jammy
Fix Committed
Undecided
Chengen Du
Mantic
Won't Fix
Undecided
Chengen Du
Noble
Fix Committed
Undecided
Chengen Du
Oracular
Fix Released
Undecided
Unassigned
Plucky
Fix Released
Undecided
Unassigned

Bug Description

[Impact]
The shell's read builtin iterates through /proc/mounts one line at a time. This becomes problematic when LDAP automount maps generate a large number of entries in /proc/mounts. It can lead to timeout issues, especially when iterating through the entries twice in the cryptroot hook.

[Fix]
Applying the following upstream commit [1] can resolve this issue.

95fd4be9b4c6 d/functions: get_mnt_devno(): Speed up execution time on large /proc/mounts.
    Use awk rather than a `while read; do done` loop here as the /proc/mounts
    pseudo-file can be many thousands lines long and the shell's `read` builtin
    traverses it one read(2) at the time which cruelly slows down execution time.

    See https://salsa.debian.org/cryptsetup-team/cryptsetup/-/merge_requests/36 .

[Test Plan]
1. Prepare a VM with its root partition encrypted using LUKS.
2. Save the content of /etc/crypttab for reference.
3. Install the patched package and execute the binary located at /usr/share/initramfs-tools/hooks/cryptroot.
4. Verify that the /etc/crypttab content remains unchanged and ensure the output does not include the following warning message:
cryptsetup: WARNING: Couldn't determine root device

For ZFS, which does not have a major/minor device number, the hook function will skip it.
To reproduce this scenario, it is necessary to configure the root partition on ZFS.
You can refer to the documentation [2] for the setup process.
Alternatively, we can prepare a simple script to manually trigger this scenario for testing purposes.
===
#!/bin/sh

. /lib/cryptsetup/functions

if devnos="$(get_mnt_devno $1)"; then
 echo "devnos: ${devnos}"
else
 echo "WARNING: Couldn't determine device"
fi
===

The logic can be easily verified by following these steps:
# mount | grep zfs
/zfs on /zfs type zfs (rw,xattr,noacl)
zfs/dataset on /mnt type zfs (rw,xattr,noacl)
# ./test /mnt
devnos:
# echo $?
0

The devnos should be empty, and no errors should occur.

The performance improvement test is outlined as follows:
root@jammy-ptp:~# mkdir src dst
root@jammy-ptp:~# for i in {1..5000}; do touch src/test_${i} dst/test_${i}; mount --bind src/test_${i} dst/test_${i}; done
root@jammy-ptp:~# wc -l /proc/mounts
5028 /proc/mounts

[Before]
root@jammy-ptp:~# time /usr/share/initramfs-tools/hooks/cryptroot
real 0m1.415s
user 0m0.975s
sys 0m0.529s

[After]
root@jammy-ptp:~/cryptsetup# time /usr/share/initramfs-tools/hooks/cryptroot
real 0m0.129s
user 0m0.098s
sys 0m0.037s

[Where problems could occur]
The patch exclusively modifies the method of extracting information without altering the underlying hook logic.
It's crucial to note that the successful generation of the crypttab is contingent upon the accuracy of the information provided by the patch.
Any inaccuracies may impede the crypttab generation process.

[Other Info]
The proposed change [1] is already applied in Oracular and Plucky.

[1] https://salsa.debian.org/cryptsetup-team/cryptsetup/-/commit/95fd4be9b4c6471e94c418101e7acfae7e1aa4fc
[2] https://openzfs.github.io/openzfs-docs/Getting%20Started/Ubuntu/Ubuntu%2022.04%20Root%20on%20ZFS.html

Chengen Du (chengendu)
description: updated
Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Focal

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Jammy

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Mantic

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Noble

Changed in cryptsetup (Ubuntu Focal):
assignee: nobody → Chengen Du (chengendu)
Changed in cryptsetup (Ubuntu Jammy):
assignee: nobody → Chengen Du (chengendu)
Changed in cryptsetup (Ubuntu Mantic):
assignee: nobody → Chengen Du (chengendu)
Changed in cryptsetup (Ubuntu Noble):
assignee: nobody → Chengen Du (chengendu)
Changed in cryptsetup (Ubuntu Focal):
status: New → In Progress
Changed in cryptsetup (Ubuntu Jammy):
status: New → In Progress
Changed in cryptsetup (Ubuntu Mantic):
status: New → In Progress
Changed in cryptsetup (Ubuntu Noble):
status: New → In Progress
Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "lp2054390_focal_refine_proc_mounts_entries_traversal.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

tags: added: patch
Revision history for this message
Chengen Du (chengendu) wrote :

There is some feedback from upstream regarding this implementation [1]. Before proceeding with the SRU process, I will reach out to them to confirm that the patch won't introduce any regressions. I'll keep the bug updated with their response.

[1] https://salsa.debian.org/cryptsetup-team/cryptsetup/-/merge_requests/36

Revision history for this message
Steve Langasek (vorlon) wrote :

blocked per last comment.

Changed in cryptsetup (Ubuntu Noble):
status: In Progress → Incomplete
Revision history for this message
Benjamin Drung (bdrung) wrote :

Unsubscribing ~ubuntu-sponsors for now. Please re-subscribe once ready.

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Focal

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Jammy

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Mantic

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Noble

Changed in cryptsetup (Ubuntu Noble):
status: Incomplete → In Progress
Revision history for this message
Chengen Du (chengendu) wrote :

Upstream has merged the patch [1]. I have backported the new patch for SRU.

[1] https://salsa.debian.org/cryptsetup-team/cryptsetup/-/merge_requests/36

Chengen Du (chengendu)
description: updated
Revision history for this message
Chengen Du (chengendu) wrote :

Following the discussion with the SRU sponsor (i.e., @mfo/@halves), the SRU will be put on hold until the Noble release. The patch is not considered critical for the release, as it focuses on optimization rather than bug fixes. It would be advantageous to opt for a safer approach, allowing for additional time for thorough testing.

Revision history for this message
Simon Chopin (schopin) wrote :

Is this ready for review/sponsoring?

Revision history for this message
Mauricio Faria de Oliveira (mfo) wrote :

No, please ignore it for now; there's an issue/regression identified in the patch.

Changed in cryptsetup (Ubuntu Noble):
status: In Progress → Incomplete
Changed in cryptsetup (Ubuntu Mantic):
status: In Progress → Incomplete
Changed in cryptsetup (Ubuntu Jammy):
status: In Progress → Incomplete
Changed in cryptsetup (Ubuntu Focal):
status: In Progress → Incomplete
Changed in cryptsetup (Ubuntu):
status: In Progress → Incomplete
Revision history for this message
Mauricio Faria de Oliveira (mfo) wrote :

Removed subscription of ubuntu-sponsors.

Revision history for this message
Brian Murray (brian-murray) wrote :

Ubuntu 23.10 (Mantic Minotaur) has reached end of life, so this bug will not be fixed for that specific release.

Changed in cryptsetup (Ubuntu Mantic):
status: Incomplete → Won't Fix
Revision history for this message
Chengen Du (chengendu) wrote :

There are no issues with the patch; it turned out to be a misunderstanding. We are currently awaiting the SE sponsor's review.

Changed in cryptsetup (Ubuntu Focal):
status: Incomplete → In Progress
Changed in cryptsetup (Ubuntu Jammy):
status: Incomplete → In Progress
Changed in cryptsetup (Ubuntu Noble):
status: Incomplete → In Progress
Revision history for this message
Chengen Du (chengendu) wrote :

@mfo Could you please find some time to verify this patch? The patch set is lp2054390-XXX-d-functions-get_mnt_devno-Speed-up-execution-time-on.debdiff. Your assistance would be greatly appreciated.

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Focal

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Jammy

Revision history for this message
Chengen Du (chengendu) wrote :

Debdiff for Noble

Revision history for this message
Mauricio Faria de Oliveira (mfo) wrote :

The proposed change is already applied in Oracular and Plucky.

$ rmadison -a source cryptsetup
...
 cryptsetup | 2:2.7.2-2ubuntu1 | oracular | source
 cryptsetup | 2:2.7.2-2ubuntu1 | plucky | source

$ pull-lp-source cryptsetup
Found cryptsetup 2:2.7.2-2ubuntu1 in plucky

$ grep -A4 '^get_mnt_devno()' cryptsetup-2.7.2/debian/functions
get_mnt_devno() {
    local wantmount="$1" devnos="" uuid dev
    local out spec fstype DEV MAJ MIN

    # use awk rather than a `while read; do done` loop here as /proc/mounts

description: updated
description: updated
Changed in cryptsetup (Ubuntu Plucky):
status: Incomplete → Fix Released
Changed in cryptsetup (Ubuntu Oracular):
status: New → Fix Released
Revision history for this message
Mauricio Faria de Oliveira (mfo) wrote :

Hi Chengen,

I'm resuming the reviews for these SRUs.

At the moment, the Test Plan section is a bit vague:
Can you please clarify what is the expected output of running cryptroot?
And IMHO it could be improved with a specific test for regressions (the key point in SRUs is to maintain stability/avoid regressions, even more so in boot-related SRUs :),
e.g., compare the results with the old/new packages, and ensure they remain the same.

Thanks!

Changed in cryptsetup (Ubuntu Plucky):
assignee: Chengen Du (chengendu) → nobody
Changed in cryptsetup (Ubuntu Noble):
status: In Progress → Incomplete
description: updated
Steve Langasek (vorlon)
description: updated
Revision history for this message
Mauricio Faria de Oliveira (mfo) wrote (last edit ):

Also, it would be nice to measure/validate the performance improvement with a fake, synthetic, very large /proc/mounts file (e.g., mount -o bind /tmp/fake-mounts /proc/mounts), in the order of the problematic LDAP file mentioned in the [Impact] section, since this is what the SRU is about: i.e., old package takes X seconds and/or timeout, new package takes Y (much less than X) seconds.

Revision history for this message
Chengen Du (chengendu) wrote :

Hi Mauricio,

Thank you for the update. The test plan has been revised accordingly.

description: updated
Chengen Du (chengendu)
Changed in cryptsetup (Ubuntu Noble):
status: Incomplete → In Progress
Revision history for this message
Heitor Alves de Siqueira (halves) wrote :

Hi Chengen,

thanks for the additional details! The performance improvements are considerable enough that I think we should attempt to SRU this. Some comments before we proceed:

For Jammy/Noble, the patch has a ZFS-specific section. Is there a reason why this isn't needed in Focal as well? AFAIK, ZFS on Focal also supports encryption, so I'm wondering why that section of the patch didn't make it there.

Related to that, considering that we have ZFS-specific bits in the patch that are not upstream, I'd like to include it in the Test Plan. Maybe do an additional test with encrypted ZFS-on-root setup, or something similar? Other scenarios seem to be covered well enough between the current Test Plan and existing autopkgtests, but ZFS is notably missing.

Changed in cryptsetup (Ubuntu Focal):
status: In Progress → Incomplete
Chengen Du (chengendu)
description: updated
Revision history for this message
Chengen Du (chengendu) wrote :

Hi Heitor,

As referenced in the corresponding bug report [1], the ZFS-related logic should ideally be backported to Focal.
However, the reason for not doing so remains unclear.
Since the issue is unrelated to the reported bug, addressing it as part of this fix might not be appropriate.

[1] https://bugs.launchpad.net/ubuntu/+source/cryptsetup/+bug/1830110

Chengen Du (chengendu)
Changed in cryptsetup (Ubuntu Focal):
status: Incomplete → In Progress
Revision history for this message
Heitor Alves de Siqueira (halves) wrote :

Hi Chengen,

Apologies for taking long; I wanted to make sure this was well tested before uploading. I've been running tests with encrypted rootfs using a PPA with your patches [0], and didn't spot any regressions so far. I've also confirmed the performance improvements according to the description, so this should help with the original report of failing LDAP automounts.

One note about Focal: there's a typo in your debdiff due to the function name in this release (_device_uuid vs device_uuid). I've fixed this in my test package, but please be mindful of these in the future. I'm also holding back on uploading Focal for now, as I'd like to stage it together with bug 1830110.

Sponsored for Noble/Jammy, thanks!

[0] https://launchpad.net/~halves/+archive/ubuntu/2054390-test/

Revision history for this message
Timo Aaltonen (tjaalton) wrote : Please test proposed package

Hello Chengen, or anyone else affected,

Accepted cryptsetup into noble-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/cryptsetup/2:2.7.0-1ubuntu4.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-noble to verification-done-noble. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-noble. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in cryptsetup (Ubuntu Noble):
status: In Progress → Fix Committed
tags: added: verification-needed verification-needed-noble
Changed in cryptsetup (Ubuntu Jammy):
status: In Progress → Fix Committed
tags: added: verification-needed-jammy
Revision history for this message
Timo Aaltonen (tjaalton) wrote :

Hello Chengen, or anyone else affected,

Accepted cryptsetup into jammy-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/cryptsetup/2:2.4.3-1ubuntu1.3 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-jammy to verification-done-jammy. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-jammy. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Revision history for this message
Ubuntu SRU Bot (ubuntu-sru-bot) wrote : Autopkgtest regression report (cryptsetup/2:2.7.0-1ubuntu4.2)

All autopkgtests for the newly accepted cryptsetup (2:2.7.0-1ubuntu4.2) for noble have finished running.
The following regressions have been reported in tests triggered by the package:

clevis/20-1 (armhf)
mandos/1.8.16-1ubuntu4 (s390x)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/noble/update_excuses.html#cryptsetup

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

Revision history for this message
Chengen Du (chengendu) wrote :

The reported regression in Noble involves an error occurring while processing the `mandos-client` package, which does not appear to be related to this issue.

161s Setting up mandos-client (1.8.16-1ubuntu4) ...
170s gpg: key 5F3A09F160B7A22B was created 77 seconds in the future (time warp or clock problem)
170s gpg: key 5F3A09F160B7A22B was created 77 seconds in the future (time warp or clock problem)
170s gpg: key 5F3A09F160B7A22B was created 77 seconds in the future (time warp or clock problem)
170s gpg: make_keysig_packet failed: Time conflict
170s shred: /tmp/mandos-keygen-privkey.A3zKnKrm06: failed to open for writing: No such file or directory
170s dpkg: error processing package mandos-client (--configure):
170s installed mandos-client package post-installation script subprocess returned error exit status 2

Both proposed packages in Jammy and Noble have been verified according to the test plan, and the results meet expectations.

[Jammy]
root@jammy:~# apt policy cryptsetup
cryptsetup:
  Installed: 2:2.4.3-1ubuntu1.3
  Candidate: 2:2.4.3-1ubuntu1.3
  Version table:
 *** 2:2.4.3-1ubuntu1.3 500
        500 http://archive.ubuntu.com/ubuntu jammy-proposed/main amd64 Packages
        100 /var/lib/dpkg/status
     2:2.4.3-1ubuntu1.2 500
        500 http://tw.archive.ubuntu.com/ubuntu jammy-updates/main amd64 Packages
     2:2.4.3-1ubuntu1 500
        500 http://tw.archive.ubuntu.com/ubuntu jammy/main amd64 Packages

root@jammy:~# mount | grep zfs
zfs on /zfs type zfs (rw,xattr,noacl)
zfs/ds on /zfs/ds type zfs (rw,xattr,noacl)
root@jammy:~# ./test /zfs/ds
devnos:
root@jammy:~# echo $?
0

root@jammy:~# wc -l /proc/mounts
5028 /proc/mounts
<-Before->
root@jammy:~# time /usr/share/initramfs-tools/hooks/cryptroot
real 0m1.538s
user 0m1.032s
sys 0m0.582s
<-After->
root@jammy:~# time /usr/share/initramfs-tools/hooks/cryptroot
real 0m0.097s
user 0m0.074s
sys 0m0.025s

[Noble]
root@noble:~# apt policy cryptsetup
cryptsetup:
  Installed: 2:2.7.0-1ubuntu4.2
  Candidate: 2:2.7.0-1ubuntu4.2
  Version table:
 *** 2:2.7.0-1ubuntu4.2 100
        100 http://tw.archive.ubuntu.com/ubuntu noble-proposed/main amd64 Packages
        100 /var/lib/dpkg/status
     2:2.7.0-1ubuntu4.1 500
        500 http://tw.archive.ubuntu.com/ubuntu noble-updates/main amd64 Packages
     2:2.7.0-1ubuntu4 500
        500 http://tw.archive.ubuntu.com/ubuntu noble/main amd64 Packages

root@noble:~# mount | grep zfs
zfs on /zfs type zfs (rw,relatime,xattr,noacl,casesensitive)
zfs/ds on /zfs/ds type zfs (rw,relatime,xattr,noacl,casesensitive)
root@noble:~# ./test /zfs/ds
devnos:
root@noble:~# echo $?
0

root@noble:~# wc -l /proc/mounts
5022 /proc/mounts
<-Before->
root@noble:~# time /usr/share/initramfs-tools/hooks/cryptroot
real 0m1.654s
user 0m0.995s
sys 0m0.726s
<-After->
root@noble:~# time /usr/share/initramfs-tools/hooks/cryptroot
real 0m0.058s
user 0m0.038s
sys 0m0.024s

tags: added: verification-done verification-done-jammy verification-done-noble
removed: verification-needed verification-needed-jammy verification-needed-noble
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.