Re: [PATCH] crash_dump: release keyring reference at the correct time

[Coiby Xu]

Hi Guangshuo,

Thanks for sending this patch! Your fix is more complete than my version
https://lore.kernel.org/kexec/20260501234342.2518281-2-coiby.xu@xxxxxxxxx/
So I plan to drop mine from the patch set. I only have some nitpicking
for this patch. Please check inline comments.

On Wed, Jun 03, 2026 at 09:50:56PM +0800, Guangshuo Li wrote:
> restore_dm_crypt_keys_to_thread_keyring() gets a reference to the user
> keyring before restoring the saved dm-crypt keys.
>
> The same keyring reference is then passed to add_key_to_keyring() for each
> saved key, but add_key_to_keyring() drops that reference on every call.
> This is only balanced when exactly one key is restored. With multiple
> keys, the keyring reference is dropped too many times and may trigger a
> refcount underflow or use-after-free.

My testing shows when there are more than five keys to be added, this
"refcount_t: underflow; use-after" error can occur. Maybe you can
include this info in your commit msg.

> The early error paths after lookup_user_key() also return without dropping
> the keyring reference.
>
> Keep ownership of the keyring reference in
> restore_dm_crypt_keys_to_thread_keyring(), drop it once on all exit paths,
> and make add_key_to_keyring() only use the reference without consuming it.
>
> Fixes: 62f17d9df692 ("crash_dump: retrieve dm crypt keys in kdump kernel")
> Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> ---
> kernel/crash_dump_dm_crypt.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index a20d4097744a..641c290f1270 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -80,7 +80,6 @@ static int add_key_to_keyring(struct dm_crypt_key *dm_key,
> 		kexec_dprintk("Error when adding key");
> 	}
>
> -	key_ref_put(keyring_ref);
> 	return r;
> }
>
> @@ -104,6 +103,7 @@ static int restore_dm_crypt_keys_to_thread_keyring(void)
> 	size_t keys_header_size;
> 	key_ref_t keyring_ref;

I think ordering local variables from longest line length to shortest line
length a.k.a Reverse Christmas Tree style is preferred i.e.
    int ret = 0;
    u64 addr;


--
Best regards,
Coiby