Hi Coiby, Thanks for the review and for sharing the testing result.
On Thu, 25 Jun 2026 at 18:50, Coiby Xu <[email protected]> wrote: > > 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 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
