Re: [PATCH v10 5/8] powerpc: Move remove_ima_buffer() to drivers/of/kexec.c

[Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>]

On 12/5/20 12:14 PM, Thiago Jung Bauermann wrote:
> Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx> writes:
>
> > remove_ima_buffer() removes the chosen node "linux,ima-kexec-buffer"
> > from the device tree and frees the memory reserved for carrying forward
> > the IMA measurement logs on kexec. This function does not have
> > architecture specific code, but is currently limited to powerpc.
> >
> > Move remove_ima_buffer() to "drivers/of/ima_kexec.c" so that it is
>
> s/ima_kexec./kexec.c/
Will fix it.

> > accessible for other architectures as well.
> >
> > Co-developed-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
>
> Looks good. Just minor comments below. Nevertheless:
>
> Reviewed-by: Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxx>
>
> > ---
> >   arch/powerpc/include/asm/ima.h |  8 ++------
> >   arch/powerpc/kexec/ima.c       | 26 --------------------------
> >   drivers/of/kexec.c             | 29 +++++++++++++++++++++++++++++
> >   include/linux/kexec.h          |  1 +
> >   4 files changed, 32 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
> > index ead488cf3981..a2fc71bc3b23 100644
> > --- a/arch/powerpc/include/asm/ima.h
> > +++ b/arch/powerpc/include/asm/ima.h
> > @@ -2,17 +2,13 @@
> >   #ifndef _ASM_POWERPC_IMA_H
> >   #define _ASM_POWERPC_IMA_H
> >   
> > +#include <linux/kexec.h>
> > +
> >   struct kimage;
>
> When you include <linux/kexec.h>, there's no need anymore for the
> `struct kimage` forward declaration so you can remove it.

Sure - i'll remove the forward declaration.

> >   
> >   int ima_get_kexec_buffer(void **addr, size_t *size);
> >   int ima_free_kexec_buffer(void);
> >   
> > -#ifdef CONFIG_IMA
> > -void remove_ima_buffer(void *fdt, int chosen_node);
> > -#else
> > -static inline void remove_ima_buffer(void *fdt, int chosen_node) {}
> > -#endif
> > -
>
> This patch introduces a slight behaviour change on powerpc when
> CONFIG_IMA isn't set: since remove_ima_buffer() is now defined,
> kexec_file_load() will remove the IMA kexec buffer when creating the FDT
> for the next kernel. Before this patch, it will leave it there.
>
> I think it's actually an improvement since a stale IMA kexec buffer
> isn't useful and just wastes memory. This should be mentioned on the
> commit message.

Thanks Thiago.

Appreciate if you could test this change for powerpc and let me know if 
you see any issues.

> >   #ifdef CONFIG_IMA_KEXEC
> >   int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
> >   			      size_t size);
> > diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c
> > index 906e8212435d..68017123b07d 100644
> > --- a/arch/powerpc/kexec/ima.c
> > +++ b/arch/powerpc/kexec/ima.c
> > @@ -61,32 +61,6 @@ int ima_free_kexec_buffer(void)
> >   	return memblock_free(addr, size);
> >   }
> >   
> > -/**
> > - * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
> > - *
> > - * @fdt: Flattened Device Tree to update
> > - * @chosen_node: Offset to the chosen node in the device tree
> > - *
> > - * The IMA measurement buffer is of no use to a subsequent kernel, so we always
> > - * remove it from the device tree.
> > - */
> > -void remove_ima_buffer(void *fdt, int chosen_node)
> > -{
> > -	int ret;
> > -	unsigned long addr;
> > -	size_t size;
> > -
> > -	ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
> > -	if (ret)
> > -		return;
> > -
> > -	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> > -
> > -	ret = delete_fdt_mem_rsv(fdt, addr, size);
> > -	if (!ret)
> > -		pr_debug("Removed old IMA buffer reservation.\n");
> > -}
> > -
> >   #ifdef CONFIG_IMA_KEXEC
> >   /**
> >    * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
> > diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> > index 516b86f7113a..42d16dfff78d 100644
> > --- a/drivers/of/kexec.c
> > +++ b/drivers/of/kexec.c
> > @@ -129,3 +129,32 @@ int get_ima_kexec_buffer(void *fdt, int chosen_node,
> >   
> >   	return 0;
> >   }
> > +
> > +/**
> > + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
> > + *
> > + * @fdt: Flattened Device Tree to update
> > + * @chosen_node: Offset to the chosen node in the device tree
> > + *
> > + * The IMA measurement buffer is of no use to a subsequent kernel, so we always
> > + * remove it from the device tree.
> > + */
> > +void remove_ima_buffer(void *fdt, int chosen_node)
> > +{
> > +	int ret;
> > +	unsigned long addr;
> > +	size_t size;
> > +
> > +	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
> > +		return;
> > +
> > +	ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
> > +	if (ret)
> > +		return;
> > +
> > +	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> > +
> > +	ret = delete_fdt_mem_rsv(fdt, addr, size);
> > +	if (!ret)
> > +		pr_debug("Removed old IMA buffer reservation.\n");
> > +}
> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > index 10ff704ab670..52a0efff184d 100644
> > --- a/include/linux/kexec.h
> > +++ b/include/linux/kexec.h
> > @@ -411,6 +411,7 @@ static inline int kexec_crash_loaded(void) { return 0; }
> >   extern int get_root_addr_size_cells(int *addr_cells, int *size_cells);
> >   extern int get_ima_kexec_buffer(void *fdt, int chosen_node,
> >   				unsigned long *addr, size_t *size);
> > +extern void remove_ima_buffer(void *fdt, int chosen_node);
> >   extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
> >   			      unsigned long size);
> >   #endif /* CONFIG_OF_FLATTREE */
>
> Same comment as before: remove the `extern` keyword.

Will remove "extern" keyword.

 -lakshmi