Re: [RFC v1 0/8] RFC v1: Kernel handling of CPU and memory hot un/plug for crash

[Eric DeVolder]

See below, thanks, eric

On 12/1/21 06:59, Baoquan He wrote:
> + akpm
>
> On 11/29/21 at 01:42pm, Eric DeVolder wrote:
> > Hi, see below.
> > eric
> >
> > On 11/24/21 03:02, Baoquan He wrote:
> > > Hi,
> > >
> > > On 11/18/21 at 12:49pm, Eric DeVolder wrote:
> > > ......
> > > > This patchset introduces a generic crash hot un/plug handler that
> > > > registers with the CPU and memory notifiers. Upon CPU or memory
> > > > changes, this generic handler is invoked and performs important
> > > > housekeeping, for example obtaining the appropriate lock, and then
> > > > invokes an architecture specific handler to do the appropriate
> > > > updates.
> > > >
> > > > In the case of x86_64, the arch specific handler generates a new
> > > > elfcorehdr, which reflects the current CPUs and memory regions, into a
> > > > buffer. Since purgatory also does an integrity check via hash digests
> > > > of the loaded segments, purgatory must also be updated with the new
> > >
> > > When I tried to address this with a draft patch, I started with a
> > > different way in which udev rule triggers reloading and only elfcorehdr
> > > segment is updated. The update should be less time consuming. Seems
> > > internal notifier is better in your way. But I didn't update purgatory
> > > since I just skipped the elfcorehdr part when calculate the digest of
> > > segments. The reason from my mind is kernel text, initrd must contribute
> > > most part of the digest, elfcorehdr is much less, and it will simplify
> > > code change more. Doing so let us have no need to touch purgatory at
> > > all. What do you think?
> >
> > Well certainly if purgatory did not need to be updated, then that simplifies
> > matters quite a bit!
> >
> > I do not have any context on the history of including elfcorehdr in the purgatory
> > integrity check. I do agree with you that checking kernel, initrd, boot_params
> > is most important. Perhaps allowing the elfcorehdr data structure to change
> > isn't too bad without including in the integrity check is ok as there is some
> > sanity checking of it by the capture kernel as it reads it for /proc/vmcore setup.
>
> Well, I think the check has included elfcorehdr since user space
> kexec-tools added the check. We can do the skipping in kexec_file load
> in kernel for the time being, see if anyone has concern about the
> safety or security. Since agreement has been reached, can you split out
> the purgatory update and repost a new patchset with the current
> frame work to only update elfcorehdr?

I reworked the patchset as you suggested and removed the reload of purgatory.
It simplified things considerably.

> Any by the way, I think you have written a very great cover letter which
> tells almost all details about the change. However, pity that they are
> not put in patch log. In your patch log, you just tell what change is
> made in the patch, but the why we need it which is the most important part
> is not seen. Most of time, we can get what change has been made from the
> code, surely it's very helpful if patch log has told it and can save
> reviewers much time, but it's not easy to get why it's needed or
> introduced if not being involved in earlier discussion or any context.
> And as you know, cover letter will be stripped away whem maintainers
> merge patch, only patch log is kept.

I've tried to place more information in the individual patch commit messages,
or the code itself.

I just posted v2!

Thanks for your interest and review!
eric

> Thanks
> Baoquan
>
> > > Still reviewing.
> >
> > Thank you!
> >
> > > > digests. The arch handler also generates a new purgatory into a
> > > > buffer, performs the hash digests of the new memory segments, and then
> > > > patches purgatory with the new digests.  If all succeeds, then the
> > > > elfcorehdr and purgatory buffers over write the existing buffers and
> > > > the new kdump image is live and ready to go. No involvement with
> > > > userspace at all.