Re: [PATCH v4 1/7] ima: define and call ima_alloc_kexec_file_buf

[Tushar Sugandhi <tusharsu@linux.microsoft.com>]

Thanks Mimi.


On 1/24/24 05:33, Mimi Zohar wrote:
> Hi Tushar,
> 
> On Mon, 2024-01-22 at 10:37 -0800, Tushar Sugandhi wrote:
> 
> Missing from this and the other patch descriptions is the problem
> description.  Please refer to the section titled  "Describe your changes" in
> https://docs.kernel.org/process/submitting-patches.html.
> 
> "Describe your problem.  Whether your patch is a one-line bug fix or 5000 lines
> of a new feature, there must be an underlying problem that motivated you to do
> this work.  Convince the reviewer that there is a problem worth fixing and that
> it makes sense for them to read past the first paragraph."
> 
> In this case, "why" you need to refactor ima_dump_measurement_list() is the
> problem.
> 
Thanks.  I will revisit all the patch descriptions in this series to 
take into account the 'why' specific to that particular patch.

> For example:
> 
> Carrying the IMA measurement list across kexec requires allocating a buffer and
> copying the measurement records.  Separate allocating the buffer and copying the
> measurement records into separate functions in order to allocate the buffer at
> kexec "load" and copy the measurements at kexec "execute".
> 
Appreciate you giving an example in this case.
I will try to follow it in other patches too.

> "Once the problem is established, describe what you are actually doing about it
> in technical detail.  It's important to describe the change in plain English for
> the reviewer to verify that the code is behaving as you intend it to."
> 
>> Refactor ima_dump_measurement_list() to move the memory allocation part
>> to a separate function ima_alloc_kexec_file_buf() which allocates buffer
>> of size 'kexec_segment_size' at kexec 'load'.  Make the local variable
>> ima_kexec_file in function ima_dump_measurement_list() as local static to
>> the file, so that it can be accessed from ima_alloc_kexec_file_buf().
>> Make necessary changes to the function ima_add_kexec_buffer() to call the
>> above two functions.
> 
> Please make this into an unordered list.
> 
Will do.

Thanks again.

~Tushar