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

Hi Guangshuo,

Thanks for sending this patch! Your fix is more complete than my version
https://lore.kernel.org/kexec/[email protected]/
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 <[email protected]>
>---
> 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