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

[Guangshuo Li]

Hi Coiby,

Thanks for the review and for sharing the testing result.

On Thu, 25 Jun 2026 at 18:50, Coiby Xu <coiby.xu@xxxxxxxxx> wrote:
>
> 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

I have sent v2 with the commit message updated to mention the refcount
underflow/use-after-free warning, and also fixed the local variable
ordering as suggested.

Thanks,
Guangshuo