Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel

[Baoquan He <bhe@redhat.com>]

On 06/07/24 at 08:27pm, Coiby Xu wrote:
> On Tue, Jun 04, 2024 at 04:51:03PM +0800, Baoquan He wrote:
> > Hi Coiby,
> 
> Hi Baoquan,
> 
> > 
> > On 05/23/24 at 01:04pm, Coiby Xu wrote:
> > > A sysfs /sys/kernel/crash_dm_crypt_keys is provided for user space to make
> > > the dm crypt keys persist for the kdump kernel. User space can send the
> > > following commands,
> > > - "init KEY_NUM"
> > >   Initialize needed structures
> > > - "record KEY_DESC"
> > >   Record a key description. The key must be a logon key.
> > > 
> > > User space can also read this API to learn about current state.
> > 
> > From the subject, can I think the luks keys will persist forever? or
> > only for a while?
> 
> Yes, you are right. The keys need to stay in kdump reserved memory.

Hmm, there are two different concepts we may need differentiate. From
security keys's point of view, the keys need be stored for a while so
that kdump loading take action to get it, that's done through sysfs;
Froom kdump's point of view, the keys need be stored forever till kdump
kernel use it. I can't see what you are referring to from the subject,
esepcially you stress the newly added sysfs
/sys/kernel/crash_dm_crypt_keys.

> 
> > If need and can only keep it for a while, can you
> > mention it and tell why and how it will be used. Because you add a lot
> > of codes, but only simply mention the sysfs, that doesn't make sense.
> 
> Thanks for raising the concern! I've added
> Documentation/ABI/testing/crash_dm_crypt_keys and copy some text in the
> cover letter to this patch in v5.
> 
> > 
> > > 
> > > Signed-off-by: Coiby Xu <coxu@redhat.com>
> > > ---
> > >  include/linux/crash_core.h   |   5 +-
> > >  kernel/Kconfig.kexec         |   8 +++
> > >  kernel/Makefile              |   1 +
> > >  kernel/crash_dump_dm_crypt.c | 113 +++++++++++++++++++++++++++++++++++
> > >  kernel/ksysfs.c              |  22 +++++++
> > >  5 files changed, 148 insertions(+), 1 deletion(-)
> > >  create mode 100644 kernel/crash_dump_dm_crypt.c
> > > 
> > > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> > > index 44305336314e..6bff1c24efa3 100644
> > > --- a/include/linux/crash_core.h
> > > +++ b/include/linux/crash_core.h
> > > @@ -34,7 +34,10 @@ static inline void arch_kexec_protect_crashkres(void) { }
> > >  static inline void arch_kexec_unprotect_crashkres(void) { }
> > >  #endif
> [...]
> > > +static int init(const char *buf)
> >              ~~~~ A more interesting name with more description?
> 
> Thanks for the suggestion! I've added some comments for this function
> in v5. But I can't come up with a better name after looking at current
> kernel code. You are welcome to suggest any better name:)

Usually init() is for the whole driver module. Your init() here only
receive the passed total keys number, and allocate the key_header, how
can you simply name it init()? If you call it init_keys_header(), I
would think it's much more meaningful.

> 
> > > +static int process_cmd(const char *buf, size_t count)
> >                                                  ~~~~
> > If nobody use the count, why do you add it?
> 
> Good catch! Yes, this is no need to use count in v4. But v5 now needs it to avoid
> buffer overflow.

OK, did you add code comment telling what 'count' stands for?

And the name 'process_cmd()' is also ambiguous. We may need avoid this
kind of name, e.g process_cmd, do_things, handle_stuff. Can you add a
more specific name?