[PATCH 1/2] Choosing the most memory-consuming key slot when estimating the memory requirement for LUKS-encrypted target

[Coiby Xu]

When there are multiple key slots, "kdumpctl estimate" uses the least
memory-consuming key slot. For example, when there are two memory slots
created with --pbkdf-memory=1048576 (1G) and --pbkdf-memory=524288 (512M),
"kdumpctl estimate" thinks the extra memory requirement is only 512M.
This will of course lead to OOM if the user uses the more
memory-consuming key slot. Fix it by sorting in reverse order.

Fixes: e9e6a2c ("kdumpctl: Add kdumpctl estimate")
Signed-off-by: Coiby Xu 
---
 kdumpctl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kdumpctl b/kdumpctl
index 126ecb9..ee40d33 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1241,7 +1241,7 @@ do_estimate()
    for _dev in $(get_all_kdump_crypt_dev); do
    	_crypt_info=$(cryptsetup luksDump "/dev/block/$_dev")
    	[[ $(echo "$_crypt_info" | sed -n "s/^Version:\s*(.*)/\1/p") == "2" ]] || continue
-		for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*(.*)/\1/p" | sort -n); do
+		for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*(.*)/\1/p" | sort -n -r); do
    		crypt_size=$((crypt_size + _mem * 1024))
    		break
    	done

[PATCH 2/2] Include the memory overhead cost of cryptsetup when estimating the memory requirement for LUKS-encrypted target

[Coiby Xu]

Currently, "kdumpctl estimate" neglects the memory overhead cost of
cryptsetup itself. Unfortunately, there is no golden formula to
calculate the overhead cost [1]. So estimate the overhead cost as 50M
for aarch64 and 20M for other architectures based on the following
empirical data,

| Overhead (M) | OS                                        | arch    |
| ------------ | ----------------------------------------- | ------- |
| 14.1         | RHEL-9.2.0-20220829.d.1                   | ppc64le |
| 14           | Fedora-37-20220830.n.0 Everything ppc64le | ppc64le |
| 17           | Fedora 36                                 | ppc64le |
| 8.8          | Fedora 35                                 | s390x   |
| 10.1         | Fedora-Rawhide-20220829.n.0, fc38         | s390x   |
| 42           | Fedora-Rawhide-20220829.n.0, fc38         | arch64  |
| 40           | F35                                       | arch64  |
| 42           | F36                                       | arch64  |
| 42           | Fedora-Rawhide-20220901.n.0               | arch64  |
| 10           | F35                                       | x86_64  |
| 10           | Fedora-Rawhide-20220901.n.0               | x86_64  |
| 11           | Fedora-Rawhide-20220901.n.0               | x86_64  |

[1] https://lore.kernel.org/cryptsetup/20220616044339.376qlipk5h2omhx2@Rk/T/#u

Fixes: e9e6a2c ("kdumpctl: Add kdumpctl estimate")
Signed-off-by: Coiby Xu 
---
 kdumpctl | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/kdumpctl b/kdumpctl
index ee40d33..18abdd7 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -1192,7 +1192,7 @@ do_estimate()
    local kdump_mods
    local -A large_mods
    local baseline
-	local kernel_size mod_size initrd_size baseline_size runtime_size reserved_size estimated_size recommended_size
+	local kernel_size mod_size initrd_size baseline_size runtime_size reserved_size estimated_size recommended_size _cryptsetup_overhead
    local size_mb=$((1024 * 1024))

setup_initrd
@@ -1246,7 +1246,17 @@ do_estimate()
    		break
    	done
    done
-	[[ $crypt_size -ne 0 ]] && echo -e "Encrypted kdump target requires extra memory, assuming using the keyslot with minimun memory requirement\n"
+
+	if [[ $crypt_size -ne 0 ]]; then
+		if [[ $(uname -m) == aarch64 ]]; then
+			_cryptsetup_overhead=50
+		else
+			_cryptsetup_overhead=20
+		fi
+
+		crypt_size=$((crypt_size + _cryptsetup_overhead * size_mb))
+		echo -e "Encrypted kdump target requires extra memory, assuming using the keyslot with minimun memory requirement\n"
+	fi

estimated_size=$((kernel_size + mod_size + initrd_size + runtime_size + crypt_size))
    if [[ $baseline_size -gt $estimated_size ]]; then

[PATCH 2/2] Include the memory overhead cost of cryptsetup when estimating the memory requirement for LUKS-encrypted target

[Philipp Rudo]

Hi Coiby,

On Mon,  5 Sep 2022 18:24:37 +0800
Coiby Xu  wrote:
> Currently, "kdumpctl estimate" neglects the memory overhead cost of
> cryptsetup itself. Unfortunately, there is no golden formula to
> calculate the overhead cost [1]. So estimate the overhead cost as 50M
> for aarch64 and 20M for other architectures based on the following
> empirical data,
>
> | Overhead (M) | OS                                        | arch    |
> | ------------ | ----------------------------------------- | ------- |
> | 14.1         | RHEL-9.2.0-20220829.d.1                   | ppc64le |
> | 14           | Fedora-37-20220830.n.0 Everything ppc64le | ppc64le |
> | 17           | Fedora 36                                 | ppc64le |
> | 8.8          | Fedora 35                                 | s390x   |
> | 10.1         | Fedora-Rawhide-20220829.n.0, fc38         | s390x   |
> | 42           | Fedora-Rawhide-20220829.n.0, fc38         | arch64  |
> | 40           | F35                                       | arch64  |
> | 42           | F36                                       | arch64  |
> | 42           | Fedora-Rawhide-20220901.n.0               | arch64  |
> | 10           | F35                                       | x86_64  |
> | 10           | Fedora-Rawhide-20220901.n.0               | x86_64  |
> | 11           | Fedora-Rawhide-20220901.n.0               | x86_64  |

this approach isn't very elegant but looks like it's the best we can
do. At least it's better than before ;-)

Reviewed-by: Philipp Rudo

P.S. do_estimate is slowly getting pretty long and hard to read. Maybe
it's worth to split it up next time we work on it.
> [1] https://lore.kernel.org/cryptsetup/20220616044339.376qlipk5h2omhx2@Rk/T/#u
>
> Fixes: e9e6a2c ("kdumpctl: Add kdumpctl estimate")
> Signed-off-by: Coiby Xu
>
> kdumpctl | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index ee40d33..18abdd7 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1192,7 +1192,7 @@ do_estimate()
>    local kdump_mods
>    local -A large_mods
>    local baseline
> local kernel_size mod_size initrd_size baseline_size runtime_size reserved_size estimated_size recommended_size
>
> local kernel_size mod_size initrd_size baseline_size runtime_size reserved_size estimated_size recommended_size _cryptsetup_overhead
> local size_mb=$((1024 * 1024))
>
> setup_initrd
>
> @@ -1246,7 +1246,17 @@ do_estimate()
>    		break
>    	done
>    done
> [[ $crypt_size -ne 0 ]] && echo -e "Encrypted kdump target requires extra memory, assuming using the keyslot with minimun memory requirement\n"
>
> if [[ $crypt_size -ne 0 ]]; then
> if [[ $(uname -m) == aarch64 ]]; then
> 	_cryptsetup_overhead=50
> else
> 	_cryptsetup_overhead=20
> fi
> crypt_size=$((crypt_size + _cryptsetup_overhead * size_mb))
> echo -e "Encrypted kdump target requires extra memory, assuming using the keyslot with minimun memory requirement\n"
>
> fi
>
> estimated_size=$((kernel_size + mod_size + initrd_size + runtime_size + crypt_size))
> if [[ $baseline_size -gt $estimated_size ]]; then

[PATCH 2/2] Include the memory overhead cost of cryptsetup when estimating the memory requirement for LUKS-encrypted target

[Coiby Xu]

On Tue, Oct 25, 2022 at 04:46:19PM +0200, Philipp Rudo wrote:
> Hi Coiby,
>
> On Mon,  5 Sep 2022 18:24:37 +0800
> Coiby Xu  wrote:
> > Currently, "kdumpctl estimate" neglects the memory overhead cost of
> > cryptsetup itself. Unfortunately, there is no golden formula to
> > calculate the overhead cost [1]. So estimate the overhead cost as 50M
> > for aarch64 and 20M for other architectures based on the following
> > empirical data,
> >
> > | Overhead (M) | OS                                        | arch    |
> > | ------------ | ----------------------------------------- | ------- |
> > | 14.1         | RHEL-9.2.0-20220829.d.1                   | ppc64le |
> > | 14           | Fedora-37-20220830.n.0 Everything ppc64le | ppc64le |
> > | 17           | Fedora 36                                 | ppc64le |
> > | 8.8          | Fedora 35                                 | s390x   |
> > | 10.1         | Fedora-Rawhide-20220829.n.0, fc38         | s390x   |
> > | 42           | Fedora-Rawhide-20220829.n.0, fc38         | arch64  |
> > | 40           | F35                                       | arch64  |
> > | 42           | F36                                       | arch64  |
> > | 42           | Fedora-Rawhide-20220901.n.0               | arch64  |
> > | 10           | F35                                       | x86_64  |
> > | 10           | Fedora-Rawhide-20220901.n.0               | x86_64  |
> > | 11           | Fedora-Rawhide-20220901.n.0               | x86_64  |
>
> this approach isn't very elegant but looks like it's the best we can
> do. At least it's better than before ;-)
>
> Reviewed-by: Philipp Rudo

Yeah, thanks for the review!
> P.S. do_estimate is slowly getting pretty long and hard to read. Maybe
> it's worth to split it up next time we work on it.

Sure, thanks for the reminder!
> > [1] https://lore.kernel.org/cryptsetup/20220616044339.376qlipk5h2omhx2@Rk/T/#u
> >
> > Fixes: e9e6a2c ("kdumpctl: Add kdumpctl estimate")
> > Signed-off-by: Coiby Xu
> >
> > kdumpctl | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index ee40d33..18abdd7 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -1192,7 +1192,7 @@ do_estimate()
> >    local kdump_mods
> >    local -A large_mods
> >    local baseline
> > local kernel_size mod_size initrd_size baseline_size runtime_size reserved_size estimated_size recommended_size
> >
> > local kernel_size mod_size initrd_size baseline_size runtime_size reserved_size estimated_size recommended_size _cryptsetup_overhead
> > local size_mb=$((1024 * 1024))
> >
> > setup_initrd
> >
> > @@ -1246,7 +1246,17 @@ do_estimate()
> >    		break
> >    	done
> >    done
> > [[ $crypt_size -ne 0 ]] && echo -e "Encrypted kdump target requires extra memory, assuming using the keyslot with minimun memory requirement\n"
> >
> > if [[ $crypt_size -ne 0 ]]; then
> > if [[ $(uname -m) == aarch64 ]]; then
> > 	_cryptsetup_overhead=50
> > else
> > 	_cryptsetup_overhead=20
> > fi
> > crypt_size=$((crypt_size + _cryptsetup_overhead * size_mb))
> > echo -e "Encrypted kdump target requires extra memory, assuming using the keyslot with minimun memory requirement\n"
> >
> > fi
> >
> > estimated_size=$((kernel_size + mod_size + initrd_size + runtime_size + crypt_size))
> > if [[ $baseline_size -gt $estimated_size ]]; then

[Lichen Liu]

Reviewed-by: Lichen Liu

On Mon, Sep 5, 2022 at 6:24 PM Coiby Xu  wrote:
> When there are multiple key slots, "kdumpctl estimate" uses the least
> memory-consuming key slot. For example, when there are two memory slots
> created with --pbkdf-memory=1048576 (1G) and --pbkdf-memory=524288 (512M),
> "kdumpctl estimate" thinks the extra memory requirement is only 512M.
> This will of course lead to OOM if the user uses the more
> memory-consuming key slot. Fix it by sorting in reverse order.
>
> Fixes: e9e6a2c ("kdumpctl: Add kdumpctl estimate")
> Signed-off-by: Coiby Xu
>
> kdumpctl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 126ecb9..ee40d33 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1241,7 +1241,7 @@ do_estimate()
>         for _dev in $(get_all_kdump_crypt_dev); do
>                 _crypt_info=$(cryptsetup luksDump "/dev/block/$_dev")
>                 [[ $(echo "$_crypt_info" | sed -n "s/^Version:\s*(.*)/\1/p") == "2" ]] || continue
>           for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*\(.*\)/\1/p" | sort -n); do
>           for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*\(.*\)/\1/p" | sort -n -r); do
>                   crypt_size=$((crypt_size + _mem * 1024))
>                   break
>           done
>
> --
> 2.37.1
> _______________________________________________
> kexec mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]
> Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> List Archives: https://lists.fedoraproject.org/archives/list/[e-mail]
> Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue

[Coiby Xu]

On Thu, Oct 20, 2022 at 02:55:10PM +0800, Lichen Liu wrote:
> Reviewed-by: Lichen Liu

Thanks for the review, patch merged!
> On Mon, Sep 5, 2022 at 6:24 PM Coiby Xu  wrote:
> > When there are multiple key slots, "kdumpctl estimate" uses the least
> > memory-consuming key slot. For example, when there are two memory slots
> > created with --pbkdf-memory=1048576 (1G) and --pbkdf-memory=524288 (512M),
> > "kdumpctl estimate" thinks the extra memory requirement is only 512M.
> > This will of course lead to OOM if the user uses the more
> > memory-consuming key slot. Fix it by sorting in reverse order.
> >
> > Fixes: e9e6a2c ("kdumpctl: Add kdumpctl estimate")
> > Signed-off-by: Coiby Xu
> >
> > kdumpctl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index 126ecb9..ee40d33 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -1241,7 +1241,7 @@ do_estimate()
> >         for _dev in $(get_all_kdump_crypt_dev); do
> >                 _crypt_info=$(cryptsetup luksDump "/dev/block/$_dev")
> >                 [[ $(echo "$_crypt_info" | sed -n "s/^Version:\s*(.*)/\1/p") == "2" ]] || continue
> >           for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*\(.*\)/\1/p" | sort -n); do
> >           for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*\(.*\)/\1/p" | sort -n -r); do
> >                   crypt_size=$((crypt_size + _mem * 1024))
> >                   break
> >           done
> >
> > --
> > 2.37.1
> > _______________________________________________
> > kexec mailing list -- [e-mail]
> > To unsubscribe send an email to [e-mail]
> > Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives: https://lists.fedoraproject.org/archives/list/[e-mail]
> > Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue

[Coiby Xu]

On Wed, Oct 26, 2022 at 03:24:43PM +0800, Coiby Xu wrote:
> On Thu, Oct 20, 2022 at 02:55:10PM +0800, Lichen Liu wrote:
> > Reviewed-by: Lichen Liu
>
> Thanks for the review, patch merged!

Btw, I also changed the information regarding which keyslot will be used when
merging the patch,

-       [[ $crypt_size -ne 0 ]] && echo -e "Encrypted kdump target requires extra memory, assuming using the keyslot with minimun memory requirement\n"
+       [[ $crypt_size -ne 0 ]] && echo -e "Encrypted kdump target requires extra memory, assuming using the keyslot with maximum memory requirement\n"
> > On Mon, Sep 5, 2022 at 6:24 PM Coiby Xu  wrote:
> > > When there are multiple key slots, "kdumpctl estimate" uses the least
> > > memory-consuming key slot. For example, when there are two memory slots
> > > created with --pbkdf-memory=1048576 (1G) and --pbkdf-memory=524288 (512M),
> > > "kdumpctl estimate" thinks the extra memory requirement is only 512M.
> > > This will of course lead to OOM if the user uses the more
> > > memory-consuming key slot. Fix it by sorting in reverse order.
> > >
> > > Fixes: e9e6a2c ("kdumpctl: Add kdumpctl estimate")
> > > Signed-off-by: Coiby Xu
> > >
> > > kdumpctl | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kdumpctl b/kdumpctl
> > > index 126ecb9..ee40d33 100755
> > > --- a/kdumpctl
> > > +++ b/kdumpctl
> > > @@ -1241,7 +1241,7 @@ do_estimate()
> > >        for _dev in $(get_all_kdump_crypt_dev); do
> > >                _crypt_info=$(cryptsetup luksDump "/dev/block/$_dev")
> > >                [[ $(echo "$_crypt_info" | sed -n "s/^Version:\s*(.*)/\1/p") == "2" ]] || continue
> > >           for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*\(.*\)/\1/p" | sort -n); do
> > >           for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*\(.*\)/\1/p" | sort -n -r); do
> > >                  crypt_size=$((crypt_size + _mem * 1024))
> > >                  break
> > >          done
> > >
> > > --
> > > 2.37.1
> > > _______________________________________________
> > > kexec mailing list -- [e-mail]
> > > To unsubscribe send an email to [e-mail]
> > > Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> > > List Archives: https://lists.fedoraproject.org/archives/list/[e-mail]
> > > Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
>
> -- 
> Best regards,
> Coiby

[Philipp Rudo]

Hi Coiby,

this one doesn't seem to be merged yet.

Looks good to me

Reviewed-by: Philipp Rudo

On Mon,  5 Sep 2022 18:24:36 +0800
Coiby Xu  wrote:
> When there are multiple key slots, "kdumpctl estimate" uses the least
> memory-consuming key slot. For example, when there are two memory slots
> created with --pbkdf-memory=1048576 (1G) and --pbkdf-memory=524288 (512M),
> "kdumpctl estimate" thinks the extra memory requirement is only 512M.
> This will of course lead to OOM if the user uses the more
> memory-consuming key slot. Fix it by sorting in reverse order.
>
> Fixes: e9e6a2c ("kdumpctl: Add kdumpctl estimate")
> Signed-off-by: Coiby Xu
>
> kdumpctl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 126ecb9..ee40d33 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1241,7 +1241,7 @@ do_estimate()
>    for _dev in $(get_all_kdump_crypt_dev); do
>    	_crypt_info=$(cryptsetup luksDump "/dev/block/$_dev")
>    	[[ $(echo "$_crypt_info" | sed -n "s/^Version:\s*(.*)/\1/p") == "2" ]] || continue
> for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*\(.*\)/\1/p" | sort -n); do
> for _mem in $(echo "$_crypt_info" | sed -n "s/\sMemory:\s*\(.*\)/\1/p" | sort -n -r); do
> crypt_size=$((crypt_size + _mem * 1024))
> break
> done