| Message ID | 20171010063619.6303-4-takahiro.akashi@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Takahiro, On 10/10/17 07:36, AKASHI Takahiro wrote: > arch_kexec_kernel_*() and arch_kimage_file_post_load_cleanup can now be > duplicated among some architectures, so let's factor them out. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > Cc: Dave Young <dyoung@redhat.com> > Cc: Vivek Goyal <vgoyal@redhat.com> > Cc: Baoquan He <bhe@redhat.com> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/kexec.h | 2 +- > arch/powerpc/kernel/kexec_elf_64.c | 2 +- > arch/powerpc/kernel/machine_kexec_file_64.c | 36 ++---------------- > arch/x86/include/asm/kexec-bzimage64.h | 2 +- > arch/x86/kernel/kexec-bzimage64.c | 2 +- > arch/x86/kernel/machine_kexec_64.c | 45 +---------------------- > include/linux/kexec.h | 15 ++++---- > kernel/kexec_file.c | 57 +++++++++++++++++++++++++++-- > 8 files changed, 70 insertions(+), 91 deletions(-) > > diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h > index 25668bc8cb2a..23588952d8bd 100644 > --- a/arch/powerpc/include/asm/kexec.h > +++ b/arch/powerpc/include/asm/kexec.h > @@ -92,7 +92,7 @@ static inline bool kdump_in_progress(void) > } > > #ifdef CONFIG_KEXEC_FILE > -extern struct kexec_file_ops kexec_elf64_ops; > +extern const struct kexec_file_ops kexec_elf64_ops; > > #ifdef CONFIG_IMA_KEXEC > #define ARCH_HAS_KIMAGE_ARCH > diff --git a/arch/powerpc/kernel/kexec_elf_64.c b/arch/powerpc/kernel/kexec_elf_64.c > index 9a42309b091a..6c78c11c7faf 100644 > --- a/arch/powerpc/kernel/kexec_elf_64.c > +++ b/arch/powerpc/kernel/kexec_elf_64.c > @@ -657,7 +657,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, > return ret ? ERR_PTR(ret) : fdt; > } > > -struct kexec_file_ops kexec_elf64_ops = { > +const struct kexec_file_ops kexec_elf64_ops = { > .probe = elf64_probe, > .load = elf64_load, > }; > diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c b/arch/powerpc/kernel/machine_kexec_file_64.c > index 992c0d258e5d..e7ce78857f0b 100644 > --- a/arch/powerpc/kernel/machine_kexec_file_64.c > +++ b/arch/powerpc/kernel/machine_kexec_file_64.c > @@ -31,8 +31,9 @@ > > #define SLAVE_CODE_SIZE 256 > > -static struct kexec_file_ops *kexec_file_loaders[] = { > +const struct kexec_file_ops * const kexec_file_loaders[] = { > &kexec_elf64_ops, > + NULL > }; > > int arch_kexec_kernel_image_probe(struct kimage *image, void *buf, > @@ -45,38 +46,7 @@ int arch_kexec_kernel_image_probe(struct kimage *image, void *buf, > if (image->type == KEXEC_TYPE_CRASH) > return -ENOTSUPP; > > - for (i = 0; i < ARRAY_SIZE(kexec_file_loaders); i++) { > - fops = kexec_file_loaders[i]; > - if (!fops || !fops->probe) > - continue; > - > - ret = fops->probe(buf, buf_len); > - if (!ret) { > - image->fops = fops; > - return ret; > - } > - } > - > - return ret; > -} > - > -void *arch_kexec_kernel_image_load(struct kimage *image) > -{ > - if (!image->fops || !image->fops->load) > - return ERR_PTR(-ENOEXEC); > - > - return image->fops->load(image, image->kernel_buf, > - image->kernel_buf_len, image->initrd_buf, > - image->initrd_buf_len, image->cmdline_buf, > - image->cmdline_buf_len); > -} > - > -int arch_kimage_file_post_load_cleanup(struct kimage *image) > -{ > - if (!image->fops || !image->fops->cleanup) > - return 0; > - > - return image->fops->cleanup(image->image_loader_data); > + return _kexec_kernel_image_probe(image, buf, buf_len); > } > > /** > diff --git a/arch/x86/include/asm/kexec-bzimage64.h b/arch/x86/include/asm/kexec-bzimage64.h > index d1b5d194e31d..284fd23d133b 100644 > --- a/arch/x86/include/asm/kexec-bzimage64.h > +++ b/arch/x86/include/asm/kexec-bzimage64.h > @@ -1,6 +1,6 @@ > #ifndef _ASM_KEXEC_BZIMAGE64_H > #define _ASM_KEXEC_BZIMAGE64_H > > -extern struct kexec_file_ops kexec_bzImage64_ops; > +extern const struct kexec_file_ops kexec_bzImage64_ops; > > #endif /* _ASM_KEXE_BZIMAGE64_H */ > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c > index fb095ba0c02f..705654776c0c 100644 > --- a/arch/x86/kernel/kexec-bzimage64.c > +++ b/arch/x86/kernel/kexec-bzimage64.c > @@ -538,7 +538,7 @@ static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len) > } > #endif > > -struct kexec_file_ops kexec_bzImage64_ops = { > +const struct kexec_file_ops kexec_bzImage64_ops = { > .probe = bzImage64_probe, > .load = bzImage64_load, > .cleanup = bzImage64_cleanup, > diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c > index 1f790cf9d38f..2cdd29d64181 100644 > --- a/arch/x86/kernel/machine_kexec_64.c > +++ b/arch/x86/kernel/machine_kexec_64.c > @@ -30,8 +30,9 @@ > #include <asm/set_memory.h> > > #ifdef CONFIG_KEXEC_FILE > -static struct kexec_file_ops *kexec_file_loaders[] = { > +const struct kexec_file_ops * const kexec_file_loaders[] = { > &kexec_bzImage64_ops, > + NULL > }; > #endif > > @@ -363,27 +364,6 @@ void arch_crash_save_vmcoreinfo(void) > /* arch-dependent functionality related to kexec file-based syscall */ > > #ifdef CONFIG_KEXEC_FILE > -int arch_kexec_kernel_image_probe(struct kimage *image, void *buf, > - unsigned long buf_len) > -{ > - int i, ret = -ENOEXEC; > - struct kexec_file_ops *fops; > - > - for (i = 0; i < ARRAY_SIZE(kexec_file_loaders); i++) { > - fops = kexec_file_loaders[i]; > - if (!fops || !fops->probe) > - continue; > - > - ret = fops->probe(buf, buf_len); > - if (!ret) { > - image->fops = fops; > - return ret; > - } > - } > - > - return ret; > -} > - > void *arch_kexec_kernel_image_load(struct kimage *image) > { > vfree(image->arch.elf_headers); > @@ -398,27 +378,6 @@ void *arch_kexec_kernel_image_load(struct kimage *image) > image->cmdline_buf_len); > } > > -int arch_kimage_file_post_load_cleanup(struct kimage *image) > -{ > - if (!image->fops || !image->fops->cleanup) > - return 0; > - > - return image->fops->cleanup(image->image_loader_data); > -} > - > -#ifdef CONFIG_KEXEC_VERIFY_SIG > -int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel, > - unsigned long kernel_len) > -{ > - if (!image->fops || !image->fops->verify_sig) { > - pr_debug("kernel loader does not support signature verification."); > - return -EKEYREJECTED; > - } > - > - return image->fops->verify_sig(kernel, kernel_len); > -} > -#endif > - > /* > * Apply purgatory relocations. > * > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > index 2b7590f5483a..bfb37665a77f 100644 > --- a/include/linux/kexec.h > +++ b/include/linux/kexec.h > @@ -208,7 +208,7 @@ struct kimage { > unsigned long cmdline_buf_len; > > /* File operations provided by image loader */ > - struct kexec_file_ops *fops; > + const struct kexec_file_ops *fops; > > /* Image loader handling the kernel can store a pointer here */ > void *image_loader_data; > @@ -276,12 +276,13 @@ int crash_shrink_memory(unsigned long new_size); > size_t crash_get_memory_size(void); > void crash_free_reserved_phys_range(unsigned long begin, unsigned long end); > > -int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf, > - unsigned long buf_len); > -void * __weak arch_kexec_kernel_image_load(struct kimage *image); > -int __weak arch_kimage_file_post_load_cleanup(struct kimage *image); > -int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf, > - unsigned long buf_len); > +int _kexec_kernel_image_probe(struct kimage *image, void *buf, > + unsigned long buf_len); > +void *_kexec_kernel_image_load(struct kimage *image); > +int _kimage_file_post_load_cleanup(struct kimage *image); > +int _kexec_kernel_verify_sig(struct kimage *image, void *buf, > + unsigned long buf_len); > + > int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr, > Elf_Shdr *sechdrs, unsigned int relsec); > int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > index 9f48f4412297..bbb0177e20ab 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -26,30 +26,79 @@ > #include <linux/vmalloc.h> > #include "kexec_internal.h" > > +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL}; > + > static int kexec_calculate_store_digests(struct kimage *image); > > +int _kexec_kernel_image_probe(struct kimage *image, void *buf, > + unsigned long buf_len) > +{ > + const struct kexec_file_ops *fops; > + int ret = -ENOEXEC; > + > + for (fops = kexec_file_loaders[0]; fops && fops->probe; ++fops) { Hmm, that's not gonna work (and I see that what I said in the previous patch was not 100% correct either). 'fops' should be of type 'const struct kexec_file_ops **', and the loop should be: for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) With some additional dereferences in the body of the loop. Unless you prefer the previous state of the loop (with i and the break inside), but I still think this looks better. Cheers, -- Julien Thierry IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Hi Takahiro, Sorry about the previous message, confidential disclaimer shouldn't have been there. Resending comments without disclaimer to avoid confusion. On 10/10/17 07:36, AKASHI Takahiro wrote: > arch_kexec_kernel_*() and arch_kimage_file_post_load_cleanup can now be > duplicated among some architectures, so let's factor them out. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > Cc: Dave Young <dyoung@redhat.com> > Cc: Vivek Goyal <vgoyal@redhat.com> > Cc: Baoquan He <bhe@redhat.com> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/kexec.h | 2 +- > arch/powerpc/kernel/kexec_elf_64.c | 2 +- > arch/powerpc/kernel/machine_kexec_file_64.c | 36 ++---------------- > arch/x86/include/asm/kexec-bzimage64.h | 2 +- > arch/x86/kernel/kexec-bzimage64.c | 2 +- > arch/x86/kernel/machine_kexec_64.c | 45 +---------------------- > include/linux/kexec.h | 15 ++++---- > kernel/kexec_file.c | 57 +++++++++++++++++++++++++++-- > 8 files changed, 70 insertions(+), 91 deletions(-) > > diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h > index 25668bc8cb2a..23588952d8bd 100644 > --- a/arch/powerpc/include/asm/kexec.h > +++ b/arch/powerpc/include/asm/kexec.h > @@ -92,7 +92,7 @@ static inline bool kdump_in_progress(void) > } > > #ifdef CONFIG_KEXEC_FILE > -extern struct kexec_file_ops kexec_elf64_ops; > +extern const struct kexec_file_ops kexec_elf64_ops; > > #ifdef CONFIG_IMA_KEXEC > #define ARCH_HAS_KIMAGE_ARCH > diff --git a/arch/powerpc/kernel/kexec_elf_64.c b/arch/powerpc/kernel/kexec_elf_64.c > index 9a42309b091a..6c78c11c7faf 100644 > --- a/arch/powerpc/kernel/kexec_elf_64.c > +++ b/arch/powerpc/kernel/kexec_elf_64.c > @@ -657,7 +657,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, > return ret ? ERR_PTR(ret) : fdt; > } > > -struct kexec_file_ops kexec_elf64_ops = { > +const struct kexec_file_ops kexec_elf64_ops = { > .probe = elf64_probe, > .load = elf64_load, > }; > diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c b/arch/powerpc/kernel/machine_kexec_file_64.c > index 992c0d258e5d..e7ce78857f0b 100644 > --- a/arch/powerpc/kernel/machine_kexec_file_64.c > +++ b/arch/powerpc/kernel/machine_kexec_file_64.c > @@ -31,8 +31,9 @@ > > #define SLAVE_CODE_SIZE 256 > > -static struct kexec_file_ops *kexec_file_loaders[] = { > +const struct kexec_file_ops * const kexec_file_loaders[] = { > &kexec_elf64_ops, > + NULL > }; > > int arch_kexec_kernel_image_probe(struct kimage *image, void *buf, > @@ -45,38 +46,7 @@ int arch_kexec_kernel_image_probe(struct kimage *image, void *buf, > if (image->type == KEXEC_TYPE_CRASH) > return -ENOTSUPP; > > - for (i = 0; i < ARRAY_SIZE(kexec_file_loaders); i++) { > - fops = kexec_file_loaders[i]; > - if (!fops || !fops->probe) > - continue; > - > - ret = fops->probe(buf, buf_len); > - if (!ret) { > - image->fops = fops; > - return ret; > - } > - } > - > - return ret; > -} > - > -void *arch_kexec_kernel_image_load(struct kimage *image) > -{ > - if (!image->fops || !image->fops->load) > - return ERR_PTR(-ENOEXEC); > - > - return image->fops->load(image, image->kernel_buf, > - image->kernel_buf_len, image->initrd_buf, > - image->initrd_buf_len, image->cmdline_buf, > - image->cmdline_buf_len); > -} > - > -int arch_kimage_file_post_load_cleanup(struct kimage *image) > -{ > - if (!image->fops || !image->fops->cleanup) > - return 0; > - > - return image->fops->cleanup(image->image_loader_data); > + return _kexec_kernel_image_probe(image, buf, buf_len); > } > > /** > diff --git a/arch/x86/include/asm/kexec-bzimage64.h b/arch/x86/include/asm/kexec-bzimage64.h > index d1b5d194e31d..284fd23d133b 100644 > --- a/arch/x86/include/asm/kexec-bzimage64.h > +++ b/arch/x86/include/asm/kexec-bzimage64.h > @@ -1,6 +1,6 @@ > #ifndef _ASM_KEXEC_BZIMAGE64_H > #define _ASM_KEXEC_BZIMAGE64_H > > -extern struct kexec_file_ops kexec_bzImage64_ops; > +extern const struct kexec_file_ops kexec_bzImage64_ops; > > #endif /* _ASM_KEXE_BZIMAGE64_H */ > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c > index fb095ba0c02f..705654776c0c 100644 > --- a/arch/x86/kernel/kexec-bzimage64.c > +++ b/arch/x86/kernel/kexec-bzimage64.c > @@ -538,7 +538,7 @@ static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len) > } > #endif > > -struct kexec_file_ops kexec_bzImage64_ops = { > +const struct kexec_file_ops kexec_bzImage64_ops = { > .probe = bzImage64_probe, > .load = bzImage64_load, > .cleanup = bzImage64_cleanup, > diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c > index 1f790cf9d38f..2cdd29d64181 100644 > --- a/arch/x86/kernel/machine_kexec_64.c > +++ b/arch/x86/kernel/machine_kexec_64.c > @@ -30,8 +30,9 @@ > #include <asm/set_memory.h> > > #ifdef CONFIG_KEXEC_FILE > -static struct kexec_file_ops *kexec_file_loaders[] = { > +const struct kexec_file_ops * const kexec_file_loaders[] = { > &kexec_bzImage64_ops, > + NULL > }; > #endif > > @@ -363,27 +364,6 @@ void arch_crash_save_vmcoreinfo(void) > /* arch-dependent functionality related to kexec file-based syscall */ > > #ifdef CONFIG_KEXEC_FILE > -int arch_kexec_kernel_image_probe(struct kimage *image, void *buf, > - unsigned long buf_len) > -{ > - int i, ret = -ENOEXEC; > - struct kexec_file_ops *fops; > - > - for (i = 0; i < ARRAY_SIZE(kexec_file_loaders); i++) { > - fops = kexec_file_loaders[i]; > - if (!fops || !fops->probe) > - continue; > - > - ret = fops->probe(buf, buf_len); > - if (!ret) { > - image->fops = fops; > - return ret; > - } > - } > - > - return ret; > -} > - > void *arch_kexec_kernel_image_load(struct kimage *image) > { > vfree(image->arch.elf_headers); > @@ -398,27 +378,6 @@ void *arch_kexec_kernel_image_load(struct kimage *image) > image->cmdline_buf_len); > } > > -int arch_kimage_file_post_load_cleanup(struct kimage *image) > -{ > - if (!image->fops || !image->fops->cleanup) > - return 0; > - > - return image->fops->cleanup(image->image_loader_data); > -} > - > -#ifdef CONFIG_KEXEC_VERIFY_SIG > -int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel, > - unsigned long kernel_len) > -{ > - if (!image->fops || !image->fops->verify_sig) { > - pr_debug("kernel loader does not support signature verification."); > - return -EKEYREJECTED; > - } > - > - return image->fops->verify_sig(kernel, kernel_len); > -} > -#endif > - > /* > * Apply purgatory relocations. > * > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > index 2b7590f5483a..bfb37665a77f 100644 > --- a/include/linux/kexec.h > +++ b/include/linux/kexec.h > @@ -208,7 +208,7 @@ struct kimage { > unsigned long cmdline_buf_len; > > /* File operations provided by image loader */ > - struct kexec_file_ops *fops; > + const struct kexec_file_ops *fops; > > /* Image loader handling the kernel can store a pointer here */ > void *image_loader_data; > @@ -276,12 +276,13 @@ int crash_shrink_memory(unsigned long new_size); > size_t crash_get_memory_size(void); > void crash_free_reserved_phys_range(unsigned long begin, unsigned long end); > > -int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf, > - unsigned long buf_len); > -void * __weak arch_kexec_kernel_image_load(struct kimage *image); > -int __weak arch_kimage_file_post_load_cleanup(struct kimage *image); > -int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf, > - unsigned long buf_len); > +int _kexec_kernel_image_probe(struct kimage *image, void *buf, > + unsigned long buf_len); > +void *_kexec_kernel_image_load(struct kimage *image); > +int _kimage_file_post_load_cleanup(struct kimage *image); > +int _kexec_kernel_verify_sig(struct kimage *image, void *buf, > + unsigned long buf_len); > + > int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr, > Elf_Shdr *sechdrs, unsigned int relsec); > int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > index 9f48f4412297..bbb0177e20ab 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -26,30 +26,79 @@ > #include <linux/vmalloc.h> > #include "kexec_internal.h" > > +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL}; > + > static int kexec_calculate_store_digests(struct kimage *image); > > +int _kexec_kernel_image_probe(struct kimage *image, void *buf, > + unsigned long buf_len) > +{ > + const struct kexec_file_ops *fops; > + int ret = -ENOEXEC; > + > + for (fops = kexec_file_loaders[0]; fops && fops->probe; ++fops) { Hmm, that's not gonna work (and I see that what I said in the previous patch was not 100% correct either). 'fops' should be of type 'const struct kexec_file_ops **', and the loop should be: for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) With some additional dereferences in the body of the loop. Unless you prefer the previous state of the loop (with i and the break inside), but I still think this looks better. Cheers,
On Tue, Oct 10, 2017 at 12:02:01PM +0100, Julien Thierry wrote: [snip] > >--- a/kernel/kexec_file.c > >+++ b/kernel/kexec_file.c > >@@ -26,30 +26,79 @@ > > #include <linux/vmalloc.h> > > #include "kexec_internal.h" > > > >+const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL}; > >+ > > static int kexec_calculate_store_digests(struct kimage *image); > > > >+int _kexec_kernel_image_probe(struct kimage *image, void *buf, > >+ unsigned long buf_len) > >+{ > >+ const struct kexec_file_ops *fops; > >+ int ret = -ENOEXEC; > >+ > >+ for (fops = kexec_file_loaders[0]; fops && fops->probe; ++fops) { > > Hmm, that's not gonna work (and I see that what I said in the previous > patch was not 100% correct either). Can you elaborate this a bit more? I'm sure that, with my code, any member of fops, cannot be changed; "const struct kexec_file_ops *fops" means that fops is a pointer to "constant sturct kexec_file_ops," while "struct kexec_file_ops * const kexec_file_loaders[]" means that kexec_file_loaders is a "constant array" of pointers to "constant struct kexec_file_ops." Thanks, -Takahiro AKASHI > 'fops' should be of type 'const struct kexec_file_ops **', and the loop > should be: > > for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) > > With some additional dereferences in the body of the loop. > > Unless you prefer the previous state of the loop (with i and the break > inside), but I still think this looks better. > > Cheers, > > -- > Julien Thierry > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Hello Takahiro-san, > On 11 Oct 2017, at 06:07, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Tue, Oct 10, 2017 at 12:02:01PM +0100, Julien Thierry wrote: > > [snip] > >>> --- a/kernel/kexec_file.c >>> +++ b/kernel/kexec_file.c >>> @@ -26,30 +26,79 @@ >>> #include <linux/vmalloc.h> >>> #include "kexec_internal.h" >>> >>> +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL}; >>> + >>> static int kexec_calculate_store_digests(struct kimage *image); >>> >>> +int _kexec_kernel_image_probe(struct kimage *image, void *buf, >>> + unsigned long buf_len) >>> +{ >>> + const struct kexec_file_ops *fops; >>> + int ret = -ENOEXEC; >>> + >>> + for (fops = kexec_file_loaders[0]; fops && fops->probe; ++fops) { >> >> Hmm, that's not gonna work (and I see that what I said in the previous >> patch was not 100% correct either). > > Can you elaborate this a bit more? > > I'm sure that, with my code, any member of fops, cannot be changed; > "const struct kexec_file_ops *fops" means that fops is a pointer to > "constant sturct kexec_file_ops," while "struct kexec_file_ops * > const kexec_file_loaders[]" means that kexec_file_loaders is a "constant > array" of pointers to "constant struct kexec_file_ops." > No, you need 2x const for that, i.e., const struct kexec_file_ops * const kexec_file_loaders[] otherwise, the pointed-to objects may still be modified. > Thanks, > -Takahiro AKASHI > > >> 'fops' should be of type 'const struct kexec_file_ops **', and the loop >> should be: >> >> for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) >> >> With some additional dereferences in the body of the loop. >> >> Unless you prefer the previous state of the loop (with i and the break >> inside), but I still think this looks better. >> >> Cheers, >> >> -- >> Julien Thierry >> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On 11/10/17 06:07, AKASHI Takahiro wrote: > On Tue, Oct 10, 2017 at 12:02:01PM +0100, Julien Thierry wrote: > > [snip] > >>> --- a/kernel/kexec_file.c >>> +++ b/kernel/kexec_file.c >>> @@ -26,30 +26,79 @@ >>> #include <linux/vmalloc.h> >>> #include "kexec_internal.h" >>> >>> +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL}; >>> + >>> static int kexec_calculate_store_digests(struct kimage *image); >>> >>> +int _kexec_kernel_image_probe(struct kimage *image, void *buf, >>> + unsigned long buf_len) >>> +{ >>> + const struct kexec_file_ops *fops; >>> + int ret = -ENOEXEC; >>> + >>> + for (fops = kexec_file_loaders[0]; fops && fops->probe; ++fops) { >> >> Hmm, that's not gonna work (and I see that what I said in the previous >> patch was not 100% correct either). > > Can you elaborate this a bit more? > Yes. With the current state of the loop, you are going to check the first element of kexec_file_loaders[0], and what will get incremented is the pointer contained in kexec_file_loaders rather than a pointer pointer pointing at an element of kexec_file_loaders. > I'm sure that, with my code, any member of fops, cannot be changed; > "const struct kexec_file_ops *fops" means that fops is a pointer to > "constant sturct kexec_file_ops," while "struct kexec_file_ops * > const kexec_file_loaders[]" means that kexec_file_loaders is a "constant > array" of pointers to "constant struct kexec_file_ops." > Hmm, right, my suggestion below doesn't have the right constness, fops should be declared as: const struct kexec_file_ops * const * fops; This can point at elements of kexec_file_loaders. Hope this makes more sense. Cheers, > Thanks, > -Takahiro AKASHI > > >> 'fops' should be of type 'const struct kexec_file_ops **', and the loop >> should be: >> >> for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) >> >> With some additional dereferences in the body of the loop. >> >> Unless you prefer the previous state of the loop (with i and the break >> inside), but I still think this looks better. >> >> Cheers, >>
On Wed, Oct 11, 2017 at 09:24:16AM +0100, Julien Thierry wrote: > > > On 11/10/17 06:07, AKASHI Takahiro wrote: > >On Tue, Oct 10, 2017 at 12:02:01PM +0100, Julien Thierry wrote: > > > >[snip] > > > >>>--- a/kernel/kexec_file.c > >>>+++ b/kernel/kexec_file.c > >>>@@ -26,30 +26,79 @@ > >>> #include <linux/vmalloc.h> > >>> #include "kexec_internal.h" > >>> > >>>+const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL}; > >>>+ > >>> static int kexec_calculate_store_digests(struct kimage *image); > >>> > >>>+int _kexec_kernel_image_probe(struct kimage *image, void *buf, > >>>+ unsigned long buf_len) > >>>+{ > >>>+ const struct kexec_file_ops *fops; > >>>+ int ret = -ENOEXEC; > >>>+ > >>>+ for (fops = kexec_file_loaders[0]; fops && fops->probe; ++fops) { > >> > >>Hmm, that's not gonna work (and I see that what I said in the previous > >>patch was not 100% correct either). > > > >Can you elaborate this a bit more? > > > > Yes. With the current state of the loop, you are going to check the first > element of kexec_file_loaders[0], and what will get incremented is the > pointer contained in kexec_file_loaders rather than a pointer pointer > pointing at an element of kexec_file_loaders. Aha, got it. I thought that you were talking about const usage. Since I don't want to bother anybody with my repeated minor updates, I'd like to post a new version, v6, only if you don't have any more comments and if Catalin and Will are likely to accept my other patches. Thanks, -Takahiro AKASHI > > >I'm sure that, with my code, any member of fops, cannot be changed; > >"const struct kexec_file_ops *fops" means that fops is a pointer to > >"constant sturct kexec_file_ops," while "struct kexec_file_ops * > >const kexec_file_loaders[]" means that kexec_file_loaders is a "constant > >array" of pointers to "constant struct kexec_file_ops." > > > > Hmm, right, my suggestion below doesn't have the right constness, fops > should be declared as: > const struct kexec_file_ops * const * fops; > > This can point at elements of kexec_file_loaders. > > Hope this makes more sense. > > Cheers, > > >Thanks, > >-Takahiro AKASHI > > > > > >>'fops' should be of type 'const struct kexec_file_ops **', and the loop > >>should be: > >> > >>for (fops = &kexec_file_loaders[0]; *fops && (*fops)->probe; ++fops) > >> > >>With some additional dereferences in the body of the loop. > >> > >>Unless you prefer the previous state of the loop (with i and the break > >>inside), but I still think this looks better. > >> > >>Cheers, > >> > > > -- > Julien Thierry
Hi AKASHI,
[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on v4.14-rc4 next-20171013]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/AKASHI-Takahiro/arm64-kexec-add-kexec_file_load-support/20171012-003448
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: x86_64-randconfig-in0-10141752 (attached as .config)
compiler: gcc-4.6 (Debian 4.6.4-7) 4.6.4
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
>> kernel/kexec_file.o: warning: objtool: _kexec_kernel_image_load()+0x31: unsupported stack register modification
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi AKASHI,
[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.14-rc4 next-20171013]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/AKASHI-Takahiro/arm64-kexec-add-kexec_file_load-support/20171012-003448
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: powerpc-powernv_defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
arch/powerpc/kernel/machine_kexec_file_64.c: In function 'arch_kexec_kernel_image_probe':
>> arch/powerpc/kernel/machine_kexec_file_64.c:43:25: error: unused variable 'fops' [-Werror=unused-variable]
struct kexec_file_ops *fops;
^~~~
>> arch/powerpc/kernel/machine_kexec_file_64.c:42:9: error: unused variable 'ret' [-Werror=unused-variable]
int i, ret = -ENOEXEC;
^~~
>> arch/powerpc/kernel/machine_kexec_file_64.c:42:6: error: unused variable 'i' [-Werror=unused-variable]
int i, ret = -ENOEXEC;
^
cc1: all warnings being treated as errors
vim +/fops +43 arch/powerpc/kernel/machine_kexec_file_64.c
a0458284 Thiago Jung Bauermann 2016-11-29 38
a0458284 Thiago Jung Bauermann 2016-11-29 39 int arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
a0458284 Thiago Jung Bauermann 2016-11-29 40 unsigned long buf_len)
a0458284 Thiago Jung Bauermann 2016-11-29 41 {
a0458284 Thiago Jung Bauermann 2016-11-29 @42 int i, ret = -ENOEXEC;
a0458284 Thiago Jung Bauermann 2016-11-29 @43 struct kexec_file_ops *fops;
a0458284 Thiago Jung Bauermann 2016-11-29 44
a0458284 Thiago Jung Bauermann 2016-11-29 45 /* We don't support crash kernels yet. */
a0458284 Thiago Jung Bauermann 2016-11-29 46 if (image->type == KEXEC_TYPE_CRASH)
a0458284 Thiago Jung Bauermann 2016-11-29 47 return -ENOTSUPP;
a0458284 Thiago Jung Bauermann 2016-11-29 48
6b2bef33 AKASHI Takahiro 2017-10-10 49 return _kexec_kernel_image_probe(image, buf, buf_len);
a0458284 Thiago Jung Bauermann 2016-11-29 50 }
a0458284 Thiago Jung Bauermann 2016-11-29 51
:::::: The code at line 43 was first introduced by commit
:::::: a0458284f0625ade5eff2118bab89b2d4bbacc3e powerpc: Add support code for kexec_file_load()
:::::: TO: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
:::::: CC: Michael Ellerman <mpe@ellerman.id.au>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, Oct 14, 2017 at 07:37:04PM +0800, kbuild test robot wrote: > Hi AKASHI, > > [auto build test WARNING on arm64/for-next/core] > [also build test WARNING on v4.14-rc4 next-20171013] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/AKASHI-Takahiro/arm64-kexec-add-kexec_file_load-support/20171012-003448 > base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core > config: x86_64-randconfig-in0-10141752 (attached as .config) > compiler: gcc-4.6 (Debian 4.6.4-7) 4.6.4 > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > All warnings (new ones prefixed by >>): > > >> kernel/kexec_file.o: warning: objtool: _kexec_kernel_image_load()+0x31: unsupported stack register modification I talked to Josh (objtool maintainer) and confirmed this is an issue of objtool, not a bug in my patch. -Takahiro AKASHI > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h index 25668bc8cb2a..23588952d8bd 100644 --- a/arch/powerpc/include/asm/kexec.h +++ b/arch/powerpc/include/asm/kexec.h @@ -92,7 +92,7 @@ static inline bool kdump_in_progress(void) } #ifdef CONFIG_KEXEC_FILE -extern struct kexec_file_ops kexec_elf64_ops; +extern const struct kexec_file_ops kexec_elf64_ops; #ifdef CONFIG_IMA_KEXEC #define ARCH_HAS_KIMAGE_ARCH diff --git a/arch/powerpc/kernel/kexec_elf_64.c b/arch/powerpc/kernel/kexec_elf_64.c index 9a42309b091a..6c78c11c7faf 100644 --- a/arch/powerpc/kernel/kexec_elf_64.c +++ b/arch/powerpc/kernel/kexec_elf_64.c @@ -657,7 +657,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, return ret ? ERR_PTR(ret) : fdt; } -struct kexec_file_ops kexec_elf64_ops = { +const struct kexec_file_ops kexec_elf64_ops = { .probe = elf64_probe, .load = elf64_load, }; diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c b/arch/powerpc/kernel/machine_kexec_file_64.c index 992c0d258e5d..e7ce78857f0b 100644 --- a/arch/powerpc/kernel/machine_kexec_file_64.c +++ b/arch/powerpc/kernel/machine_kexec_file_64.c @@ -31,8 +31,9 @@ #define SLAVE_CODE_SIZE 256 -static struct kexec_file_ops *kexec_file_loaders[] = { +const struct kexec_file_ops * const kexec_file_loaders[] = { &kexec_elf64_ops, + NULL }; int arch_kexec_kernel_image_probe(struct kimage *image, void *buf, @@ -45,38 +46,7 @@ int arch_kexec_kernel_image_probe(struct kimage *image, void *buf, if (image->type == KEXEC_TYPE_CRASH) return -ENOTSUPP; - for (i = 0; i < ARRAY_SIZE(kexec_file_loaders); i++) { - fops = kexec_file_loaders[i]; - if (!fops || !fops->probe) - continue; - - ret = fops->probe(buf, buf_len); - if (!ret) { - image->fops = fops; - return ret; - } - } - - return ret; -} - -void *arch_kexec_kernel_image_load(struct kimage *image) -{ - if (!image->fops || !image->fops->load) - return ERR_PTR(-ENOEXEC); - - return image->fops->load(image, image->kernel_buf, - image->kernel_buf_len, image->initrd_buf, - image->initrd_buf_len, image->cmdline_buf, - image->cmdline_buf_len); -} - -int arch_kimage_file_post_load_cleanup(struct kimage *image) -{ - if (!image->fops || !image->fops->cleanup) - return 0; - - return image->fops->cleanup(image->image_loader_data); + return _kexec_kernel_image_probe(image, buf, buf_len); } /** diff --git a/arch/x86/include/asm/kexec-bzimage64.h b/arch/x86/include/asm/kexec-bzimage64.h index d1b5d194e31d..284fd23d133b 100644 --- a/arch/x86/include/asm/kexec-bzimage64.h +++ b/arch/x86/include/asm/kexec-bzimage64.h @@ -1,6 +1,6 @@ #ifndef _ASM_KEXEC_BZIMAGE64_H #define _ASM_KEXEC_BZIMAGE64_H -extern struct kexec_file_ops kexec_bzImage64_ops; +extern const struct kexec_file_ops kexec_bzImage64_ops; #endif /* _ASM_KEXE_BZIMAGE64_H */ diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c index fb095ba0c02f..705654776c0c 100644 --- a/arch/x86/kernel/kexec-bzimage64.c +++ b/arch/x86/kernel/kexec-bzimage64.c @@ -538,7 +538,7 @@ static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len) } #endif -struct kexec_file_ops kexec_bzImage64_ops = { +const struct kexec_file_ops kexec_bzImage64_ops = { .probe = bzImage64_probe, .load = bzImage64_load, .cleanup = bzImage64_cleanup, diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c index 1f790cf9d38f..2cdd29d64181 100644 --- a/arch/x86/kernel/machine_kexec_64.c +++ b/arch/x86/kernel/machine_kexec_64.c @@ -30,8 +30,9 @@ #include <asm/set_memory.h> #ifdef CONFIG_KEXEC_FILE -static struct kexec_file_ops *kexec_file_loaders[] = { +const struct kexec_file_ops * const kexec_file_loaders[] = { &kexec_bzImage64_ops, + NULL }; #endif @@ -363,27 +364,6 @@ void arch_crash_save_vmcoreinfo(void) /* arch-dependent functionality related to kexec file-based syscall */ #ifdef CONFIG_KEXEC_FILE -int arch_kexec_kernel_image_probe(struct kimage *image, void *buf, - unsigned long buf_len) -{ - int i, ret = -ENOEXEC; - struct kexec_file_ops *fops; - - for (i = 0; i < ARRAY_SIZE(kexec_file_loaders); i++) { - fops = kexec_file_loaders[i]; - if (!fops || !fops->probe) - continue; - - ret = fops->probe(buf, buf_len); - if (!ret) { - image->fops = fops; - return ret; - } - } - - return ret; -} - void *arch_kexec_kernel_image_load(struct kimage *image) { vfree(image->arch.elf_headers); @@ -398,27 +378,6 @@ void *arch_kexec_kernel_image_load(struct kimage *image) image->cmdline_buf_len); } -int arch_kimage_file_post_load_cleanup(struct kimage *image) -{ - if (!image->fops || !image->fops->cleanup) - return 0; - - return image->fops->cleanup(image->image_loader_data); -} - -#ifdef CONFIG_KEXEC_VERIFY_SIG -int arch_kexec_kernel_verify_sig(struct kimage *image, void *kernel, - unsigned long kernel_len) -{ - if (!image->fops || !image->fops->verify_sig) { - pr_debug("kernel loader does not support signature verification."); - return -EKEYREJECTED; - } - - return image->fops->verify_sig(kernel, kernel_len); -} -#endif - /* * Apply purgatory relocations. * diff --git a/include/linux/kexec.h b/include/linux/kexec.h index 2b7590f5483a..bfb37665a77f 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -208,7 +208,7 @@ struct kimage { unsigned long cmdline_buf_len; /* File operations provided by image loader */ - struct kexec_file_ops *fops; + const struct kexec_file_ops *fops; /* Image loader handling the kernel can store a pointer here */ void *image_loader_data; @@ -276,12 +276,13 @@ int crash_shrink_memory(unsigned long new_size); size_t crash_get_memory_size(void); void crash_free_reserved_phys_range(unsigned long begin, unsigned long end); -int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf, - unsigned long buf_len); -void * __weak arch_kexec_kernel_image_load(struct kimage *image); -int __weak arch_kimage_file_post_load_cleanup(struct kimage *image); -int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf, - unsigned long buf_len); +int _kexec_kernel_image_probe(struct kimage *image, void *buf, + unsigned long buf_len); +void *_kexec_kernel_image_load(struct kimage *image); +int _kimage_file_post_load_cleanup(struct kimage *image); +int _kexec_kernel_verify_sig(struct kimage *image, void *buf, + unsigned long buf_len); + int __weak arch_kexec_apply_relocations_add(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, unsigned int relsec); int __weak arch_kexec_apply_relocations(const Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c index 9f48f4412297..bbb0177e20ab 100644 --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -26,30 +26,79 @@ #include <linux/vmalloc.h> #include "kexec_internal.h" +const __weak struct kexec_file_ops * const kexec_file_loaders[] = {NULL}; + static int kexec_calculate_store_digests(struct kimage *image); +int _kexec_kernel_image_probe(struct kimage *image, void *buf, + unsigned long buf_len) +{ + const struct kexec_file_ops *fops; + int ret = -ENOEXEC; + + for (fops = kexec_file_loaders[0]; fops && fops->probe; ++fops) { + ret = fops->probe(buf, buf_len); + if (!ret) { + image->fops = fops; + return ret; + } + } + + return ret; +} + /* Architectures can provide this probe function */ int __weak arch_kexec_kernel_image_probe(struct kimage *image, void *buf, unsigned long buf_len) { - return -ENOEXEC; + return _kexec_kernel_image_probe(image, buf, buf_len); +} + +void *_kexec_kernel_image_load(struct kimage *image) +{ + if (!image->fops || !image->fops->load) + return ERR_PTR(-ENOEXEC); + + return image->fops->load(image, image->kernel_buf, + image->kernel_buf_len, image->initrd_buf, + image->initrd_buf_len, image->cmdline_buf, + image->cmdline_buf_len); } void * __weak arch_kexec_kernel_image_load(struct kimage *image) { - return ERR_PTR(-ENOEXEC); + return _kexec_kernel_image_load(image); +} + +int _kimage_file_post_load_cleanup(struct kimage *image) +{ + if (!image->fops || !image->fops->cleanup) + return 0; + + return image->fops->cleanup(image->image_loader_data); } int __weak arch_kimage_file_post_load_cleanup(struct kimage *image) { - return -EINVAL; + return _kimage_file_post_load_cleanup(image); } #ifdef CONFIG_KEXEC_VERIFY_SIG +int _kexec_kernel_verify_sig(struct kimage *image, void *buf, + unsigned long buf_len) +{ + if (!image->fops || !image->fops->verify_sig) { + pr_debug("kernel loader does not support signature verification.\n"); + return -EKEYREJECTED; + } + + return image->fops->verify_sig(buf, buf_len); +} + int __weak arch_kexec_kernel_verify_sig(struct kimage *image, void *buf, unsigned long buf_len) { - return -EKEYREJECTED; + return _kexec_kernel_verify_sig(image, buf, buf_len); } #endif
arch_kexec_kernel_*() and arch_kimage_file_post_load_cleanup can now be duplicated among some architectures, so let's factor them out. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> Cc: Dave Young <dyoung@redhat.com> Cc: Vivek Goyal <vgoyal@redhat.com> Cc: Baoquan He <bhe@redhat.com> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> --- arch/powerpc/include/asm/kexec.h | 2 +- arch/powerpc/kernel/kexec_elf_64.c | 2 +- arch/powerpc/kernel/machine_kexec_file_64.c | 36 ++---------------- arch/x86/include/asm/kexec-bzimage64.h | 2 +- arch/x86/kernel/kexec-bzimage64.c | 2 +- arch/x86/kernel/machine_kexec_64.c | 45 +---------------------- include/linux/kexec.h | 15 ++++---- kernel/kexec_file.c | 57 +++++++++++++++++++++++++++-- 8 files changed, 70 insertions(+), 91 deletions(-)