[PATCH V2] x86/kexec: fix a kexec_file_load failure

[PATCH V2] x86/kexec: fix a kexec_file_load failure

[Dave Young]

The code cleanup mentioned in Fixes tag changed the behavior of
kexec_locate_mem_hole.  The kexec_locate_mem_hole will try to
allocate free memory only when kbuf.mem is initialized as zero.

But in x86 kexec_file_load implementation there are a few places
the kbuf.mem is reused like below:
  /* kbuf initialized, kbuf.mem = 0 */
  ...
  kexec_add_buffer()
  ...
  kexec_add_buffer()

  The second kexec_add_buffer will reuse previous kbuf but not
  reinitialize the kbuf.mem.

Thus kexec_file_load failed because the sanity check failed.

So explictily reset kbuf.mem to fix the issue.

Fixes: b6664ba42f14 ("s390, kexec_file: drop arch_kexec_mem_walk()")
Signed-off-by: Dave Young <dyoung@redhat.com>
Cc: <stable@vger.kernel.org>
---
V1 -> V2: use KEXEC_BUF_MEM_UNKNOWN in code.
 arch/x86/kernel/crash.c           | 1 +
 arch/x86/kernel/kexec-bzimage64.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index f631a3f15587..6b7890c7889b 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -469,6 +469,7 @@ int crash_load_segments(struct kimage *image)
 
 	kbuf.memsz = kbuf.bufsz;
 	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
+	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
 	ret = kexec_add_buffer(&kbuf);
 	if (ret) {
 		vfree((void *)image->arch.elf_headers);
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 278cd07228dd..0d5efa34f359 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -434,6 +434,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 	kbuf.memsz = PAGE_ALIGN(header->init_size);
 	kbuf.buf_align = header->kernel_alignment;
 	kbuf.buf_min = MIN_KERNEL_LOAD_ADDR;
+	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
 	ret = kexec_add_buffer(&kbuf);
 	if (ret)
 		goto out_free_params;
@@ -448,6 +449,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 		kbuf.bufsz = kbuf.memsz = initrd_len;
 		kbuf.buf_align = PAGE_SIZE;
 		kbuf.buf_min = MIN_INITRD_LOAD_ADDR;
+		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
 		ret = kexec_add_buffer(&kbuf);
 		if (ret)
 			goto out_free_params;
-- 
2.17.0


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2] x86/kexec: fix a kexec_file_load failure

[Dave Young]

On 12/28/18 at 09:12am, Dave Young wrote:
> The code cleanup mentioned in Fixes tag changed the behavior of
> kexec_locate_mem_hole.  The kexec_locate_mem_hole will try to
> allocate free memory only when kbuf.mem is initialized as zero.
> 
> But in x86 kexec_file_load implementation there are a few places
> the kbuf.mem is reused like below:
>   /* kbuf initialized, kbuf.mem = 0 */
>   ...
>   kexec_add_buffer()
>   ...
>   kexec_add_buffer()
> 
>   The second kexec_add_buffer will reuse previous kbuf but not
>   reinitialize the kbuf.mem.
> 
> Thus kexec_file_load failed because the sanity check failed.
> 
> So explictily reset kbuf.mem to fix the issue.
> 
> Fixes: b6664ba42f14 ("s390, kexec_file: drop arch_kexec_mem_walk()")
> Signed-off-by: Dave Young <dyoung@redhat.com>
> Cc: <stable@vger.kernel.org>
> ---
> V1 -> V2: use KEXEC_BUF_MEM_UNKNOWN in code.
>  arch/x86/kernel/crash.c           | 1 +
>  arch/x86/kernel/kexec-bzimage64.c | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index f631a3f15587..6b7890c7889b 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -469,6 +469,7 @@ int crash_load_segments(struct kimage *image)
>  
>  	kbuf.memsz = kbuf.bufsz;
>  	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>  	ret = kexec_add_buffer(&kbuf);
>  	if (ret) {
>  		vfree((void *)image->arch.elf_headers);
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 278cd07228dd..0d5efa34f359 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -434,6 +434,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>  	kbuf.memsz = PAGE_ALIGN(header->init_size);
>  	kbuf.buf_align = header->kernel_alignment;
>  	kbuf.buf_min = MIN_KERNEL_LOAD_ADDR;
> +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>  	ret = kexec_add_buffer(&kbuf);
>  	if (ret)
>  		goto out_free_params;
> @@ -448,6 +449,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>  		kbuf.bufsz = kbuf.memsz = initrd_len;
>  		kbuf.buf_align = PAGE_SIZE;
>  		kbuf.buf_min = MIN_INITRD_LOAD_ADDR;
> +		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>  		ret = kexec_add_buffer(&kbuf);
>  		if (ret)
>  			goto out_free_params;
> -- 
> 2.17.0
> 


Ping, this is a regression issue, can we get this fixed?

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2] x86/kexec: fix a kexec_file_load failure

[Baoquan He]

On 12/28/18 at 09:12am, Dave Young wrote:
> The code cleanup mentioned in Fixes tag changed the behavior of
> kexec_locate_mem_hole.  The kexec_locate_mem_hole will try to
> allocate free memory only when kbuf.mem is initialized as zero.
> 
> But in x86 kexec_file_load implementation there are a few places
> the kbuf.mem is reused like below:
>   /* kbuf initialized, kbuf.mem = 0 */
>   ...
>   kexec_add_buffer()
>   ...
>   kexec_add_buffer()
> 
>   The second kexec_add_buffer will reuse previous kbuf but not
>   reinitialize the kbuf.mem.
> 
> Thus kexec_file_load failed because the sanity check failed.
> 
> So explictily reset kbuf.mem to fix the issue.
> 
> Fixes: b6664ba42f14 ("s390, kexec_file: drop arch_kexec_mem_walk()")
> Signed-off-by: Dave Young <dyoung@redhat.com>
> Cc: <stable@vger.kernel.org>
> ---
> V1 -> V2: use KEXEC_BUF_MEM_UNKNOWN in code.
>  arch/x86/kernel/crash.c           | 1 +
>  arch/x86/kernel/kexec-bzimage64.c | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index f631a3f15587..6b7890c7889b 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -469,6 +469,7 @@ int crash_load_segments(struct kimage *image)
>  

Wondering why this place doesn't need the initialization assignment.
Isn't it to assign in all places before kexec_add_buffer() calling?

	/* Add backup segment. */
        if (image->arch.backup_src_sz) { 
	}

>  	kbuf.memsz = kbuf.bufsz;
>  	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>  	ret = kexec_add_buffer(&kbuf);
>  	if (ret) {
>  		vfree((void *)image->arch.elf_headers);
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 278cd07228dd..0d5efa34f359 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -434,6 +434,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>  	kbuf.memsz = PAGE_ALIGN(header->init_size);
>  	kbuf.buf_align = header->kernel_alignment;
>  	kbuf.buf_min = MIN_KERNEL_LOAD_ADDR;

Same question for bzImage64_load(), there are three kexec_add_buffer()
calling, I only saw two initialization in this patch.

> +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>  	ret = kexec_add_buffer(&kbuf);
>  	if (ret)
>  		goto out_free_params;
> @@ -448,6 +449,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>  		kbuf.bufsz = kbuf.memsz = initrd_len;
>  		kbuf.buf_align = PAGE_SIZE;
>  		kbuf.buf_min = MIN_INITRD_LOAD_ADDR;
> +		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>  		ret = kexec_add_buffer(&kbuf);
>  		if (ret)
>  			goto out_free_params;
> -- 
> 2.17.0
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2] x86/kexec: fix a kexec_file_load failure

[Dave Young]

On 01/08/19 at 01:24pm, Baoquan He wrote:
> On 12/28/18 at 09:12am, Dave Young wrote:
> > The code cleanup mentioned in Fixes tag changed the behavior of
> > kexec_locate_mem_hole.  The kexec_locate_mem_hole will try to
> > allocate free memory only when kbuf.mem is initialized as zero.
> > 
> > But in x86 kexec_file_load implementation there are a few places
> > the kbuf.mem is reused like below:
> >   /* kbuf initialized, kbuf.mem = 0 */
> >   ...
> >   kexec_add_buffer()
> >   ...
> >   kexec_add_buffer()
> > 
> >   The second kexec_add_buffer will reuse previous kbuf but not
> >   reinitialize the kbuf.mem.
> > 
> > Thus kexec_file_load failed because the sanity check failed.
> > 
> > So explictily reset kbuf.mem to fix the issue.
> > 
> > Fixes: b6664ba42f14 ("s390, kexec_file: drop arch_kexec_mem_walk()")
> > Signed-off-by: Dave Young <dyoung@redhat.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> > V1 -> V2: use KEXEC_BUF_MEM_UNKNOWN in code.
> >  arch/x86/kernel/crash.c           | 1 +
> >  arch/x86/kernel/kexec-bzimage64.c | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index f631a3f15587..6b7890c7889b 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -469,6 +469,7 @@ int crash_load_segments(struct kimage *image)
> >  
> 
> Wondering why this place doesn't need the initialization assignment.
> Isn't it to assign in all places before kexec_add_buffer() calling?

C designated initializers will make sure to initialize it as zero.
We set KEXEC_BUF_MEM_UNKNOWN as 0 so it just works.

> 
> 	/* Add backup segment. */
>         if (image->arch.backup_src_sz) { 
> 	}
> 
> >  	kbuf.memsz = kbuf.bufsz;
> >  	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> > +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> >  	ret = kexec_add_buffer(&kbuf);
> >  	if (ret) {
> >  		vfree((void *)image->arch.elf_headers);
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > index 278cd07228dd..0d5efa34f359 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -434,6 +434,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
> >  	kbuf.memsz = PAGE_ALIGN(header->init_size);
> >  	kbuf.buf_align = header->kernel_alignment;
> >  	kbuf.buf_min = MIN_KERNEL_LOAD_ADDR;
> 
> Same question for bzImage64_load(), there are three kexec_add_buffer()
> calling, I only saw two initialization in this patch.
> 
> > +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> >  	ret = kexec_add_buffer(&kbuf);
> >  	if (ret)
> >  		goto out_free_params;
> > @@ -448,6 +449,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
> >  		kbuf.bufsz = kbuf.memsz = initrd_len;
> >  		kbuf.buf_align = PAGE_SIZE;
> >  		kbuf.buf_min = MIN_INITRD_LOAD_ADDR;
> > +		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> >  		ret = kexec_add_buffer(&kbuf);
> >  		if (ret)
> >  			goto out_free_params;
> > -- 
> > 2.17.0
> > 

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2] x86/kexec: fix a kexec_file_load failure

[Baoquan He]

On 01/08/19 at 04:46pm, Dave Young wrote:
> > Wondering why this place doesn't need the initialization assignment.
> > Isn't it to assign in all places before kexec_add_buffer() calling?
> 
> C designated initializers will make sure to initialize it as zero.
> We set KEXEC_BUF_MEM_UNKNOWN as 0 so it just works.

Got it, it works, thanks. People may need check code to find out
KEXEC_BUF_MEM_UNKNOWN is 0, then realize this fact.

Other than this, it looks good to me, ack it.

Acked-by: Baoquan He <bhe@redhat.com>

Thanks
Baoquan

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2] x86/kexec: fix a kexec_file_load failure

[Dave Young]

On 01/08/19 at 04:51pm, Baoquan He wrote:
> On 01/08/19 at 04:46pm, Dave Young wrote:
> > > Wondering why this place doesn't need the initialization assignment.
> > > Isn't it to assign in all places before kexec_add_buffer() calling?
> > 
> > C designated initializers will make sure to initialize it as zero.
> > We set KEXEC_BUF_MEM_UNKNOWN as 0 so it just works.
> 
> Got it, it works, thanks. People may need check code to find out
> KEXEC_BUF_MEM_UNKNOWN is 0, then realize this fact.

Agreed,  it is not very clear now. It's better to improve it with some explict
initial value since we have the macro.  But since this is a regression
I suggest to fix the bug first, I can send a patch later for the
improvement. 

Thanks!
> 
> Other than this, it looks good to me, ack it.
> 
> Acked-by: Baoquan He <bhe@redhat.com>
> 
> Thanks
> Baoquan

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH V2] x86/kexec: fix a kexec_file_load failure

[Dave Young]

On 12/28/18 at 09:12am, Dave Young wrote:
> The code cleanup mentioned in Fixes tag changed the behavior of
> kexec_locate_mem_hole.  The kexec_locate_mem_hole will try to
> allocate free memory only when kbuf.mem is initialized as zero.
> 
> But in x86 kexec_file_load implementation there are a few places
> the kbuf.mem is reused like below:
>   /* kbuf initialized, kbuf.mem = 0 */
>   ...
>   kexec_add_buffer()
>   ...
>   kexec_add_buffer()
> 
>   The second kexec_add_buffer will reuse previous kbuf but not
>   reinitialize the kbuf.mem.
> 
> Thus kexec_file_load failed because the sanity check failed.
> 
> So explictily reset kbuf.mem to fix the issue.
> 
> Fixes: b6664ba42f14 ("s390, kexec_file: drop arch_kexec_mem_walk()")
> Signed-off-by: Dave Young <dyoung@redhat.com>
> Cc: <stable@vger.kernel.org>
> ---
> V1 -> V2: use KEXEC_BUF_MEM_UNKNOWN in code.
>  arch/x86/kernel/crash.c           | 1 +
>  arch/x86/kernel/kexec-bzimage64.c | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index f631a3f15587..6b7890c7889b 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -469,6 +469,7 @@ int crash_load_segments(struct kimage *image)
>  
>  	kbuf.memsz = kbuf.bufsz;
>  	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>  	ret = kexec_add_buffer(&kbuf);
>  	if (ret) {
>  		vfree((void *)image->arch.elf_headers);
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 278cd07228dd..0d5efa34f359 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -434,6 +434,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>  	kbuf.memsz = PAGE_ALIGN(header->init_size);
>  	kbuf.buf_align = header->kernel_alignment;
>  	kbuf.buf_min = MIN_KERNEL_LOAD_ADDR;
> +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>  	ret = kexec_add_buffer(&kbuf);
>  	if (ret)
>  		goto out_free_params;
> @@ -448,6 +449,7 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>  		kbuf.bufsz = kbuf.memsz = initrd_len;
>  		kbuf.buf_align = PAGE_SIZE;
>  		kbuf.buf_min = MIN_INITRD_LOAD_ADDR;
> +		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>  		ret = kexec_add_buffer(&kbuf);
>  		if (ret)
>  			goto out_free_params;
> -- 
> 2.17.0
> 

Andrew, Boris,  can any of you take this patch? Without this fix we have a regression.

Thanks
Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec