Re: [PATCH 07/13] kexec: Implementation of new syscall kexec_file_load

[Borislav Petkov]

On Tue, Jun 03, 2014 at 09:06:56AM -0400, Vivek Goyal wrote:
> Previous patch provided the interface definition and this patch prvides
> implementation of new syscall.
> 
> Previously segment list was prepared in user space. Now user space just
> passes kernel fd, initrd fd and command line and kernel will create a
> segment list internally.
> 
> This patch contains generic part of the code. Actual segment preparation
> and loading is done by arch and image specific loader. Which comes in
> next patch.
> 
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
>  arch/x86/kernel/machine_kexec_64.c |  54 +++++
>  include/linux/kexec.h              |  53 ++++
>  include/uapi/linux/kexec.h         |   4 +
>  kernel/kexec.c                     | 483 ++++++++++++++++++++++++++++++++++++-
>  4 files changed, 589 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index 679cef0..d9c5cf0 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -22,6 +22,13 @@
>  #include <asm/mmu_context.h>
>  #include <asm/debugreg.h>
>  
> +/* arch dependent functionality related to kexec file based syscall */

  ... arch-dependent ...			... file-based ...

> +static struct kexec_file_type kexec_file_type[] = {

You could call this kexec_file_types and use ARRAY_SIZE and drop this
nr_file_types; mangled diff ontop:

Index: b/arch/x86/kernel/machine_kexec_64.c
===================================================================
--- a/arch/x86/kernel/machine_kexec_64.c        2014-06-04 17:32:31.520372283 +0200
+++ b/arch/x86/kernel/machine_kexec_64.c        2014-06-04 17:30:59.214376321 +0200
@@ -23,12 +23,10 @@
 #include <asm/debugreg.h>
 
 /* arch dependent functionality related to kexec file based syscall */
-static struct kexec_file_type kexec_file_type[] = {
+static struct kexec_file_type kexec_file_types[] = {
        {"", NULL, NULL, NULL},
 };
 
-static int nr_file_types = sizeof(kexec_file_type)/sizeof(kexec_file_type[0]);
-
 static void free_transition_pgtable(struct kimage *image)
 {
        free_page((unsigned long)image->arch.pud);
@@ -297,7 +295,7 @@ int arch_kexec_kernel_image_probe(struct
 {
        int i, ret = -ENOEXEC;
 
-       for (i = 0; i < nr_file_types; i++) {
+       for (i = 0; i < ARRAY_SIZE(kexec_file_types); i++) {
                if (!kexec_file_type[i].probe)
                        continue;
 

> +	{"", NULL, NULL, NULL},
> +};
> +
> +static int nr_file_types = sizeof(kexec_file_type)/sizeof(kexec_file_type[0]);
> +

Superfluous newline.

>  static void free_transition_pgtable(struct kimage *image)
>  {
>  	free_page((unsigned long)image->arch.pud);
> @@ -283,3 +290,50 @@ void arch_crash_save_vmcoreinfo(void)
>  			      (unsigned long)&_text - __START_KERNEL);
>  }
>  
> +/* arch dependent functionality related to kexec file based syscall */
> +
> +int arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> +					unsigned long buf_len)

Arg alignment: it is customary to put function args on new line at the
next right position after the opening brace. Ditto for the rest of the
locations where this is the case.

> +{
> +	int i, ret = -ENOEXEC;
> +
> +	for (i = 0; i < nr_file_types; i++) {
> +		if (!kexec_file_type[i].probe)
> +			continue;
> +
> +		ret = kexec_file_type[i].probe(buf, buf_len);
> +		if (!ret) {
> +			image->file_handler_idx = i;
> +			return ret;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +void *arch_kexec_kernel_image_load(struct kimage *image, char *kernel,
> +			unsigned long kernel_len, char *initrd,
> +			unsigned long initrd_len, char *cmdline,
> +			unsigned long cmdline_len)

Those are a *lot* of arguments. Maybe a helper struct encompassing them
all to pass around?

> +{
> +	int idx = image->file_handler_idx;
> +
> +	if (idx < 0)
> +		return ERR_PTR(-ENOEXEC);
> +
> +	return kexec_file_type[idx].load(image, kernel, kernel_len, initrd,
> +					initrd_len, cmdline, cmdline_len);
> +}
> +
> +int arch_kimage_file_post_load_cleanup(struct kimage *image)
> +{
> +	int idx = image->file_handler_idx;
> +
> +	/* This can be called up even before image handler has been set */
> +	if (idx < 0)
> +		return 0;

Btw, these games with the index seem not optimal to me. Why not simply
have image->fops or so which is a pointer to struct kexec_file_type
after having it renamed to kexec_file_ops and then assign the correct
one to image->fops in arch_kexec_kernel_image_probe() and then simply
call the proper handler:

	if (!image->fops)
		return;

	return image->fops->cleanup(image);

and above

	return image->fops->load(...)

and so on.

In any case, this looks cleaner to me.

> +
> +	if (kexec_file_type[idx].cleanup)
> +		return kexec_file_type[idx].cleanup(image);
> +	return 0;
> +}
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index d0285cc..3790519 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -121,13 +121,58 @@ struct kimage {
>  #define KEXEC_TYPE_DEFAULT 0
>  #define KEXEC_TYPE_CRASH   1
>  	unsigned int preserve_context : 1;
> +	/* If set, we are using file mode kexec syscall */
> +	unsigned int file_mode:1;
>  
>  #ifdef ARCH_HAS_KIMAGE_ARCH
>  	struct kimage_arch arch;
>  #endif
> +
> +	/* Additional Fields for file based kexec syscall */

Why capitalized?

> +	void *kernel_buf;
> +	unsigned long kernel_buf_len;
> +
> +	void *initrd_buf;
> +	unsigned long initrd_buf_len;
> +
> +	char *cmdline_buf;
> +	unsigned long cmdline_buf_len;
> +
> +	/* index of file handler in array */
> +	int file_handler_idx;
> +
> +	/* Image loader handling the kernel can store a pointer here */
> +	void *image_loader_data;
>  };
>  
> +/*
> + * Keeps a track of buffer parameters as provided by caller for requesting

"Keeps track"

> + * memory placement of buffer.
> + */
> +struct kexec_buf {
> +	struct kimage *image;
> +	char *buffer;
> +	unsigned long bufsz;
> +	unsigned long memsz;
> +	unsigned long buf_align;
> +	unsigned long buf_min;
> +	unsigned long buf_max;
> +	bool top_down;		/* allocate from top of memory hole */
> +};
>  
> +typedef int (kexec_probe_t)(const char *kernel_buf, unsigned long kernel_size);
> +typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf,
> +				unsigned long kernel_len, char *initrd,
> +				unsigned long initrd_len, char *cmdline,
> +				unsigned long cmdline_len);
> +typedef int (kexec_cleanup_t)(struct kimage *image);
> +
> +struct kexec_file_type {
> +	const char *name;
> +	kexec_probe_t *probe;
> +	kexec_load_t *load;
> +	kexec_cleanup_t *cleanup;
> +};
>  
>  /* kexec interface functions */
>  extern void machine_kexec(struct kimage *image);
> @@ -138,6 +183,11 @@ extern asmlinkage long sys_kexec_load(unsigned long entry,
>  					struct kexec_segment __user *segments,
>  					unsigned long flags);
>  extern int kernel_kexec(void);
> +extern int kexec_add_buffer(struct kimage *image, char *buffer,
> +			unsigned long bufsz, unsigned long memsz,
> +			unsigned long buf_align, unsigned long buf_min,
> +			unsigned long buf_max, bool top_down,
> +			unsigned long *load_addr);
>  extern struct page *kimage_alloc_control_pages(struct kimage *image,
>  						unsigned int order);
>  extern void crash_kexec(struct pt_regs *);
> @@ -188,6 +238,9 @@ extern int kexec_load_disabled;
>  #define KEXEC_FLAGS    (KEXEC_ON_CRASH | KEXEC_PRESERVE_CONTEXT)
>  #endif
>  
> +/* Listof defined/legal kexec file flags */

"List of ..."

> +#define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH)
> +
>  #define VMCOREINFO_BYTES           (4096)
>  #define VMCOREINFO_NOTE_NAME       "VMCOREINFO"
>  #define VMCOREINFO_NOTE_NAME_BYTES ALIGN(sizeof(VMCOREINFO_NOTE_NAME), 4)
> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> index d6629d4..5fddb1b 100644
> --- a/include/uapi/linux/kexec.h
> +++ b/include/uapi/linux/kexec.h
> @@ -13,6 +13,10 @@
>  #define KEXEC_PRESERVE_CONTEXT	0x00000002
>  #define KEXEC_ARCH_MASK		0xffff0000
>  
> +/* Kexec file load interface flags */
> +#define KEXEC_FILE_UNLOAD	0x00000001
> +#define KEXEC_FILE_ON_CRASH	0x00000002

Do we have those documented somewhere and what do they mean?

> +
>  /* These values match the ELF architecture values.
>   * Unless there is a good reason that should continue to be the case.
>   */
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index a3044e6..1ad4d60 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -260,6 +260,221 @@ static struct kimage *do_kimage_alloc_init(void)
>  
>  static void kimage_free_page_list(struct list_head *list);
>  
> +static int copy_file_from_fd(int fd, void **buf, unsigned long *buf_len)
> +{
> +	struct fd f = fdget(fd);
> +	int ret = 0;
> +	struct kstat stat;
> +	loff_t pos;
> +	ssize_t bytes = 0;
> +
> +	if (!f.file)
> +		return -EBADF;
> +
> +	ret = vfs_getattr(&f.file->f_path, &stat);
> +	if (ret)
> +		goto out;
> +
> +	if (stat.size > INT_MAX) {
> +		ret = -EFBIG;
> +		goto out;
> +	}
> +
> +	/* Don't hand 0 to vmalloc, it whines. */
> +	if (stat.size == 0) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	*buf = vmalloc(stat.size);
> +	if (!*buf) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	pos = 0;
> +	while (pos < stat.size) {
> +		bytes = kernel_read(f.file, pos, (char *)(*buf) + pos,
> +					stat.size - pos);
> +		if (bytes < 0) {
> +			vfree(*buf);
> +			ret = bytes;
> +			goto out;
> +		}
> +
> +		if (bytes == 0)
> +			break;
> +		pos += bytes;
> +	}
> +
> +	*buf_len = pos;
> +out:
> +	fdput(f);
> +	return ret;
> +}
> +
> +/* Architectures can provide this probe function */
> +int __attribute__ ((weak))

We have __weak for that.

> +arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
> +				unsigned long buf_len)
> +{
> +	return -ENOEXEC;
> +}
> +
> +void *__attribute__ ((weak))

ditto.

> +arch_kexec_kernel_image_load(struct kimage *image, char *kernel,
> +		unsigned long kernel_len, char *initrd,
> +		unsigned long initrd_len, char *cmdline,
> +		unsigned long cmdline_len)
> +{
> +	return ERR_PTR(-ENOEXEC);
> +}
> +
> +void __attribute__ ((weak))

ditto.

> +arch_kimage_file_post_load_cleanup(struct kimage *image)
> +{
> +	return;
> +}
> +
> +/*
> + * Free up tempory buffers allocated which are not needed after image has
> + * been loaded.
> + *
> + * Free up memory used by kernel, initrd, and comand line. This is temporary
> + * memory allocation which is not needed any more after these buffers have
> + * been loaded into separate segments and have been copied elsewhere
> + */

Why do we need that comment? It is obvious what's going on.

> +static void kimage_file_post_load_cleanup(struct kimage *image)
> +{
> +	vfree(image->kernel_buf);
> +	image->kernel_buf = NULL;
> +
> +	vfree(image->initrd_buf);
> +	image->initrd_buf = NULL;
> +
> +	vfree(image->cmdline_buf);
> +	image->cmdline_buf = NULL;
> +
> +	/* See if architcture has anything to cleanup post load */

s/architcture/architecture/

> +	arch_kimage_file_post_load_cleanup(image);
> +}
> +
> +/*
> + * In file mode list of segments is prepared by kernel. Copy relevant
> + * data from user space, do error checking, prepare segment list
> + */
> +static int kimage_file_prepare_segments(struct kimage *image, int kernel_fd,
> +		int initrd_fd, const char __user *cmdline_ptr,
> +		unsigned long cmdline_len)

arg alignment

> +{
> +	int ret = 0;
> +	void *ldata;
> +
> +	ret = copy_file_from_fd(kernel_fd, &image->kernel_buf,
> +					&image->kernel_buf_len);
> +	if (ret)
> +		return ret;
> +
> +	/* Call arch image probe handlers */
> +	ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
> +						image->kernel_buf_len);
> +
> +	if (ret)
> +		goto out;
> +
> +	ret = copy_file_from_fd(initrd_fd, &image->initrd_buf,
> +					&image->initrd_buf_len);
> +	if (ret)
> +		goto out;
> +
> +	image->cmdline_buf = vzalloc(cmdline_len);
> +	if (!image->cmdline_buf)

		ret = -ENOMEM;

> +		goto out;
> +
> +	ret = copy_from_user(image->cmdline_buf, cmdline_ptr, cmdline_len);
> +	if (ret) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	image->cmdline_buf_len = cmdline_len;
> +
> +	/* command line should be a string with last byte null */
> +	if (image->cmdline_buf[cmdline_len - 1] != '\0') {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Call arch image load handlers */
> +	ldata = arch_kexec_kernel_image_load(image,
> +			image->kernel_buf, image->kernel_buf_len,
> +			image->initrd_buf, image->initrd_buf_len,
> +			image->cmdline_buf, image->cmdline_buf_len);
> +
> +	if (IS_ERR(ldata)) {
> +		ret = PTR_ERR(ldata);
> +		goto out;
> +	}
> +
> +	image->image_loader_data = ldata;
> +out:
> +	/* In case of error, free up all allocated memory in this function */
> +	if (ret)
> +		kimage_file_post_load_cleanup(image);
> +	return ret;
> +}
> +
> +static int kimage_file_normal_alloc(struct kimage **rimage, int kernel_fd,
> +		int initrd_fd, const char __user *cmdline_ptr,
> +		unsigned long cmdline_len)

arg alignment

> +{
> +	int result;
> +	struct kimage *image;
> +
> +	/* Allocate and initialize a controlling structure */

No need for that comment IMO.

> +	image = do_kimage_alloc_init();
> +	if (!image)
> +		return -ENOMEM;
> +
> +	image->file_mode = 1;
> +	image->file_handler_idx = -1;
> +
> +	result = kimage_file_prepare_segments(image, kernel_fd, initrd_fd,
> +			cmdline_ptr, cmdline_len);

arg alignment... yeah, you know what I mean, I'm not going to point
those out further anymore.

> +	if (result)
> +		goto out_free_image;
> +
> +	result = sanity_check_segment_list(image);
> +	if (result)
> +		goto out_free_post_load_bufs;
> +
> +	result = -ENOMEM;
> +	image->control_code_page = kimage_alloc_control_pages(image,
> +					   get_order(KEXEC_CONTROL_PAGE_SIZE));
> +	if (!image->control_code_page) {
> +		pr_err("Could not allocate control_code_buffer\n");

Might wanna define pr_fmt when using the pr_* things fo the first time
in this file.

> +		goto out_free_post_load_bufs;
> +	}
> +
> +	image->swap_page = kimage_alloc_control_pages(image, 0);
> +	if (!image->swap_page) {
> +		pr_err(KERN_ERR "Could not allocate swap buffer\n");
> +		goto out_free_control_pages;
> +	}
> +
> +	*rimage = image;
> +	return 0;
> +
> +out_free_control_pages:
> +	kimage_free_page_list(&image->control_pages);
> +out_free_post_load_bufs:
> +	kimage_file_post_load_cleanup(image);
> +	kfree(image->image_loader_data);
> +out_free_image:
> +	kfree(image);
> +	return result;
> +}
> +
>  static int kimage_normal_alloc(struct kimage **rimage, unsigned long entry,
>  				unsigned long nr_segments,
>  				struct kexec_segment __user *segments)
> @@ -683,6 +898,16 @@ static void kimage_free(struct kimage *image)
>  
>  	/* Free the kexec control pages... */
>  	kimage_free_page_list(&image->control_pages);
> +
> +	kfree(image->image_loader_data);
> +
> +	/*
> +	 * Free up any temporary buffers allocated. This might hit if
> +	 * error occurred much later after buffer allocation.
> +	 */
> +	if (image->file_mode)
> +		kimage_file_post_load_cleanup(image);
> +
>  	kfree(image);
>  }
>  
> @@ -812,10 +1037,14 @@ static int kimage_load_normal_segment(struct kimage *image,
>  	unsigned long maddr;
>  	size_t ubytes, mbytes;
>  	int result;
> -	unsigned char __user *buf;
> +	unsigned char __user *buf = NULL;
> +	unsigned char *kbuf = NULL;
>  
>  	result = 0;
> -	buf = segment->buf;
> +	if (image->file_mode)
> +		kbuf = segment->kbuf;
> +	else
> +		buf = segment->buf;
>  	ubytes = segment->bufsz;
>  	mbytes = segment->memsz;
>  	maddr = segment->mem;
> @@ -847,7 +1076,11 @@ static int kimage_load_normal_segment(struct kimage *image,
>  				PAGE_SIZE - (maddr & ~PAGE_MASK));
>  		uchunk = min(ubytes, mchunk);
>  
> -		result = copy_from_user(ptr, buf, uchunk);
> +		/* For file based kexec, source pages are in kernel memory */
> +		if (image->file_mode)
> +			memcpy(ptr, kbuf, uchunk);
> +		else
> +			result = copy_from_user(ptr, buf, uchunk);
>  		kunmap(page);
>  		if (result) {
>  			result = -EFAULT;
> @@ -855,7 +1088,10 @@ static int kimage_load_normal_segment(struct kimage *image,
>  		}
>  		ubytes -= uchunk;
>  		maddr  += mchunk;
> -		buf    += mchunk;
> +		if (image->file_mode)
> +			kbuf += mchunk;
> +		else
> +			buf += mchunk;
>  		mbytes -= mchunk;
>  	}
>  out:
> @@ -1102,7 +1338,64 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  		const char __user *, cmdline_ptr, unsigned long,
>  		cmdline_len, unsigned long, flags)
>  {
> -	return -ENOSYS;
> +	int ret = 0, i;
> +	struct kimage **dest_image, *image;
> +
> +	/* We only trust the superuser with rebooting the system. */
> +	if (!capable(CAP_SYS_BOOT))
> +		return -EPERM;
> +
> +	/* Make sure we have a legal set of flags */
> +	if (flags != (flags & KEXEC_FILE_FLAGS))
> +		return -EINVAL;

This test looks strange: according to it, kexec_file_load has to always
be called with both KEXEC_FILE_UNLOAD and KEXEC_FILE_ON_CRASH set. Don't
you want to check against an allowed mask or so like KEXEC_FLAGS is
handled in kexec_load?

> +
> +	image = NULL;
> +
> +	if (!mutex_trylock(&kexec_mutex))
> +		return -EBUSY;
> +
> +	dest_image = &kexec_image;
> +	if (flags & KEXEC_FILE_ON_CRASH)
> +		dest_image = &kexec_crash_image;
> +
> +	if (flags & KEXEC_FILE_UNLOAD)
> +		goto exchange;
> +
> +	ret = kimage_file_normal_alloc(&image, kernel_fd, initrd_fd,
> +				cmdline_ptr, cmdline_len);
> +	if (ret)
> +		goto out;
> +
> +	ret = machine_kexec_prepare(image);
> +	if (ret)
> +		goto out;
> +
> +	for (i = 0; i < image->nr_segments; i++) {
> +		struct kexec_segment *ksegment;
> +
> +		ksegment = &image->segment[i];
> +		pr_debug("Loading segment %d: buf=0x%p bufsz=0x%zx mem=0x%lx memsz=0x%zx\n",
> +			 i, ksegment->buf, ksegment->bufsz, ksegment->mem,
> +			 ksegment->memsz);
> +
> +		ret = kimage_load_segment(image, &image->segment[i]);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	kimage_terminate(image);
> +
> +	/*
> +	 * Free up any temporary buffers allocated which are not needed
> +	 * after image has been loaded
> +	 */
> +	kimage_file_post_load_cleanup(image);
> +exchange:
> +	image = xchg(dest_image, image);
> +out:
> +	mutex_unlock(&kexec_mutex);
> +	kimage_free(image);
> +	return ret;
>  }
>  
>  void crash_kexec(struct pt_regs *regs)
> @@ -1659,6 +1952,186 @@ static int __init crash_save_vmcoreinfo_init(void)
>  
>  subsys_initcall(crash_save_vmcoreinfo_init);
>  
> +static int __kexec_add_segment(struct kimage *image, char *buf,
> +		unsigned long bufsz, unsigned long mem, unsigned long memsz)
> +{
> +	struct kexec_segment *ksegment;
> +
> +	ksegment = &image->segment[image->nr_segments];
> +	ksegment->kbuf = buf;
> +	ksegment->bufsz = bufsz;
> +	ksegment->mem = mem;
> +	ksegment->memsz = memsz;
> +	image->nr_segments++;
> +
> +	return 0;
> +}
> +
> +static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
> +					struct kexec_buf *kbuf)
> +{
> +	struct kimage *image = kbuf->image;
> +	unsigned long temp_start, temp_end;
> +
> +	temp_end = min(end, kbuf->buf_max);
> +	temp_start = temp_end - kbuf->memsz;
> +
> +	do {
> +		/* align down start */
> +		temp_start = temp_start & (~(kbuf->buf_align - 1));
> +
> +		if (temp_start < start || temp_start < kbuf->buf_min)
> +			return 0;
> +
> +		temp_end = temp_start + kbuf->memsz - 1;
> +
> +		/*
> +		 * Make sure this does not conflict with any of existing
> +		 * segments
> +		 */
> +		if (kimage_is_destination_range(image, temp_start, temp_end)) {
> +			temp_start = temp_start - PAGE_SIZE;
> +			continue;
> +		}
> +
> +		/* We found a suitable memory range */
> +		break;
> +	} while (1);
> +
> +	/* If we are here, we found a suitable memory range */
> +	__kexec_add_segment(image, kbuf->buffer, kbuf->bufsz, temp_start,
> +				kbuf->memsz);
> +
> +	/* Stop navigating through remaining System RAM ranges */
> +	return 1;
> +}
> +
> +static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
> +					struct kexec_buf *kbuf)
> +{
> +	struct kimage *image = kbuf->image;
> +	unsigned long temp_start, temp_end;
> +
> +	temp_start = max(start, kbuf->buf_min);
> +
> +	do {
> +		temp_start = ALIGN(temp_start, kbuf->buf_align);
> +		temp_end = temp_start + kbuf->memsz - 1;
> +
> +		if (temp_end > end || temp_end > kbuf->buf_max)
> +			return 0;
> +		/*
> +		 * Make sure this does not conflict with any of existing
> +		 * segments
> +		 */
> +		if (kimage_is_destination_range(image, temp_start, temp_end)) {
> +			temp_start = temp_start + PAGE_SIZE;
> +			continue;
> +		}
> +
> +		/* We found a suitable memory range */
> +		break;
> +	} while (1);
> +
> +	/* If we are here, we found a suitable memory range */
> +	__kexec_add_segment(image, kbuf->buffer, kbuf->bufsz, temp_start,
> +				kbuf->memsz);
> +
> +	/* Stop navigating through remaining System RAM ranges */
> +	return 1;
> +}
> +
> +static int walk_ram_range_callback(u64 start, u64 end, void *arg)
> +{
> +	struct kexec_buf *kbuf = (struct kexec_buf *)arg;
> +	unsigned long sz = end - start + 1;
> +
> +	/* Returning 0 will take to next memory range */
> +	if (sz < kbuf->memsz)
> +		return 0;
> +
> +	if (end < kbuf->buf_min || start > kbuf->buf_max)
> +		return 0;
> +
> +	/*
> +	 * Allocate memory top down with-in ram range. Otherwise bottom up
> +	 * allocation.
> +	 */
> +	if (kbuf->top_down)
> +		return locate_mem_hole_top_down(start, end, kbuf);
> +	else
> +		return locate_mem_hole_bottom_up(start, end, kbuf);
> +}
> +
> +/*
> + * Helper functions for placing a buffer in a kexec segment. This assumes

s/functions/function/

> + * that kexec_mutex is held.
> + */
> +int kexec_add_buffer(struct kimage *image, char *buffer,
> +		unsigned long bufsz, unsigned long memsz,
> +		unsigned long buf_align, unsigned long buf_min,
> +		unsigned long buf_max, bool top_down, unsigned long *load_addr)
> +{
> +
> +	unsigned long nr_segments = image->nr_segments, new_nr_segments;
> +	struct kexec_segment *ksegment;
> +	struct kexec_buf buf, *kbuf;
> +
> +	/* Currently adding segment this way is allowed only in file mode */
> +	if (!image->file_mode)
> +		return -EINVAL;
> +
> +	if (nr_segments >= KEXEC_SEGMENT_MAX)
> +		return -EINVAL;
> +
> +	/*
> +	 * Make sure we are not trying to add buffer after allocating
> +	 * control pages. All segments need to be placed first before
> +	 * any control pages are allocated. As control page allocation
> +	 * logic goes through list of segments to make sure there are
> +	 * no destination overlaps.
> +	 */
> +	if (!list_empty(&image->control_pages)) {
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	memset(&buf, 0, sizeof(struct kexec_buf));
> +	kbuf = &buf;
> +	kbuf->image = image;
> +	kbuf->buffer = buffer;
> +	kbuf->bufsz = bufsz;
> +
> +	/* Align memsz to next page boundary */

No need for that comment...

> +	kbuf->memsz = ALIGN(memsz, PAGE_SIZE);
> +
> +	/* Align to atleast page size boundary */

ditto.

> +	kbuf->buf_align = max(buf_align, PAGE_SIZE);
> +	kbuf->buf_min = buf_min;
> +	kbuf->buf_max = buf_max;
> +	kbuf->top_down = top_down;
> +
> +	/* Walk the RAM ranges and allocate a suitable range for the buffer */
> +	walk_system_ram_res(0, -1, kbuf, walk_ram_range_callback);
> +
> +	/*
> +	 * If range could be found successfully, it would have incremented
> +	 * the nr_segments value.
> +	 */
> +	new_nr_segments = image->nr_segments;
> +
> +	/* A suitable memory range could not be found for buffer */
> +	if (new_nr_segments == nr_segments)
> +		return -EADDRNOTAVAIL;

Right, why don't you check walk_system_ram_res's retval? If it is != 0,
i.e. walk_ram_range_callback gives a 1 on "success", you can drop this
way of checking whether finding a new range succeeded.

> +
> +	/* Found a suitable memory range */
> +

superfluous newline.

> +	ksegment = &image->segment[new_nr_segments - 1];
> +	*load_addr = ksegment->mem;
> +	return 0;
> +}
> +
> +
>  /*
>   * Move into place and start executing a preloaded standalone
>   * executable.  If nothing was preloaded return an error.
> -- 
> 1.9.0
> 
> 

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/