Re: [PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

Hello Dave,

Sorry for the delay, I was trying to get the other patch series ready as I 
mentioned in the other email.

Am Donnerstag, 18 August 2016, 16:19:46 schrieb Dave Young:
> Since Eric was objecting the extension, I think you should convince him,
> but I will review from code point of view.

Thank you very much for your review!
It looks like this series went as far as it can go, though...

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


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

Re: [PATCH v2 2/2] kexec: extend kexec_file_load system call

[Dave Young]

Since Eric was objecting the extension, I think you should convince him,
but I will review from code point of view.

On 08/11/16 at 08:03pm, Thiago Jung Bauermann wrote:
> From: AKASHI Takahiro 
> 
> Device tree blob must be passed to a second kernel on DTB-capable
> archs, like powerpc and arm64, but the current kernel interface
> lacks this support.
> 
> This patch extends kexec_file_load system call by adding an extra
> argument to this syscall so that an arbitrary number of file descriptors
> can be handed out from user space to the kernel.
> 
>   long sys_kexec_file_load(int kernel_fd, int initrd_fd,
>unsigned long cmdline_len,
>const char __user *cmdline_ptr,
>unsigned long flags,
>const struct kexec_fdset __user *ufdset);
> 
> If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
> argument points to the following struct buffer:
> 
>   struct kexec_fdset {
>   int nr_fds;
>   struct kexec_file_fd fds[0];
>   }
> 
> Signed-off-by: AKASHI Takahiro 
> Signed-off-by: Thiago Jung Bauermann 
> ---
>  include/linux/fs.h |  1 +
>  include/linux/kexec.h  |  7 ++--
>  include/linux/syscalls.h   |  4 ++-
>  include/uapi/linux/kexec.h | 22 
>  kernel/kexec_file.c| 83 
> ++
>  5 files changed, 108 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3523bf62f328..847d9c31f428 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
>   id(MODULE, kernel-module)   \
>   id(KEXEC_IMAGE, kexec-image)\
>   id(KEXEC_INITRAMFS, kexec-initramfs)\
> + id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)\
>   id(POLICY, security-policy) \
>   id(MAX_ID, )
>  
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 4f85d284ed0b..29202935055d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -148,7 +148,10 @@ struct kexec_file_ops {
>   kexec_verify_sig_t *verify_sig;
>  #endif
>  };
> -#endif
> +
> +int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void 
> *buf,
> + unsigned long size);
> +#endif /* CONFIG_KEXEC_FILE */
>  
>  struct kimage {
>   kimage_entry_t head;
> @@ -280,7 +283,7 @@ extern int kexec_load_disabled;
>  
>  /* List of defined/legal kexec file flags */
>  #define KEXEC_FILE_FLAGS (KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
> -  KEXEC_FILE_NO_INITRAMFS)
> +  KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
>  
>  #define VMCOREINFO_BYTES   (4096)
>  #define VMCOREINFO_NOTE_NAME   "VMCOREINFO"
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d02239022bd0..fc072bdb74e3 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -66,6 +66,7 @@ struct perf_event_attr;
>  struct file_handle;
>  struct sigaltstack;
>  union bpf_attr;
> +struct kexec_fdset;
>  
>  #include 
>  #include 
> @@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, 
> unsigned long nr_segments,
>  asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
>   unsigned long cmdline_len,
>   const char __user *cmdline_ptr,
> - unsigned long flags);
> + unsigned long flags,
> + const struct kexec_fdset __user *ufdset);
>  
>  asmlinkage long sys_exit(int error_code);
>  asmlinkage long sys_exit_group(int error_code);
> diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
> index aae5ebf2022b..6279be79efba 100644
> --- a/include/uapi/linux/kexec.h
> +++ b/include/uapi/linux/kexec.h
> @@ -23,6 +23,28 @@
>  #define KEXEC_FILE_UNLOAD0x0001
>  #define KEXEC_FILE_ON_CRASH  0x0002
>  #define KEXEC_FILE_NO_INITRAMFS  0x0004
> +#define KEXEC_FILE_EXTRA_FDS 0x0008
> +
> +enum kexec_file_type {
> + KEXEC_FILE_TYPE_KERNEL,
> + KEXEC_FILE_TYPE_INITRAMFS,
> +
> + /*
> +  * Device Tree Blob containing just the nodes and properties that
> +  * the kexec_file_load caller wants to add or modify.
> +  */
> + KEXEC_FILE_TYPE_PARTIAL_DTB,
> +};
> +
> +struct kexec_file_fd {
> + enum kexec_file_type type;
> + int fd;
> +};
> +
> +struct kexec_fdset {
> + int nr_fds;
> + struct kexec_file_fd fds[0];
> +};
>  
>  /* 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_file.c 

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

Here is a new version implementing your suggestions.
I also changed it to kmalloc fdset instead of using the stack.

What do you think?

From: AKASHI Takahiro 

Device tree blob must be passed to a second kernel on DTB-capable
archs, like powerpc and arm64, but the current kernel interface
lacks this support.

This patch extends kexec_file_load system call by adding an extra
argument to this syscall so that an arbitrary number of file descriptors
can be handed out from user space to the kernel.

long sys_kexec_file_load(int kernel_fd, int initrd_fd,
 unsigned long cmdline_len,
 const char __user *cmdline_ptr,
 unsigned long flags,
 const struct kexec_fdset __user *ufdset);

If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
argument points to the following struct buffer:

struct kexec_fdset {
int nr_fds;
struct kexec_file_fd fds[0];
}

Signed-off-by: AKASHI Takahiro 
Signed-off-by: Thiago Jung Bauermann 
---
 include/linux/fs.h |  1 +
 include/linux/kexec.h  |  7 +++-
 include/linux/syscalls.h   |  4 +-
 include/uapi/linux/kexec.h | 22 +++
 kernel/kexec_file.c| 92 +++---
 5 files changed, 117 insertions(+), 9 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3523bf62f328..2eb0674392d1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
id(MODULE, kernel-module)   \
id(KEXEC_IMAGE, kexec-image)\
id(KEXEC_INITRAMFS, kexec-initramfs)\
+   id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)\
id(POLICY, security-policy) \
id(MAX_ID, )
 
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 4f85d284ed0b..29202935055d 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -148,7 +148,10 @@ struct kexec_file_ops {
kexec_verify_sig_t *verify_sig;
 #endif
 };
-#endif
+
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+   unsigned long size);
+#endif /* CONFIG_KEXEC_FILE */
 
 struct kimage {
kimage_entry_t head;
@@ -280,7 +283,7 @@ extern int kexec_load_disabled;
 
 /* List of defined/legal kexec file flags */
 #define KEXEC_FILE_FLAGS   (KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
-KEXEC_FILE_NO_INITRAMFS)
+KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
 
 #define VMCOREINFO_BYTES   (4096)
 #define VMCOREINFO_NOTE_NAME   "VMCOREINFO"
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d02239022bd0..fc072bdb74e3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -66,6 +66,7 @@ struct perf_event_attr;
 struct file_handle;
 struct sigaltstack;
 union bpf_attr;
+struct kexec_fdset;
 
 #include 
 #include 
@@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, 
unsigned long nr_segments,
 asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
unsigned long cmdline_len,
const char __user *cmdline_ptr,
-   unsigned long flags);
+   unsigned long flags,
+   const struct kexec_fdset __user *ufdset);
 
 asmlinkage long sys_exit(int error_code);
 asmlinkage long sys_exit_group(int error_code);
diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index aae5ebf2022b..6279be79efba 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -23,6 +23,28 @@
 #define KEXEC_FILE_UNLOAD  0x0001
 #define KEXEC_FILE_ON_CRASH0x0002
 #define KEXEC_FILE_NO_INITRAMFS0x0004
+#define KEXEC_FILE_EXTRA_FDS   0x0008
+
+enum kexec_file_type {
+   KEXEC_FILE_TYPE_KERNEL,
+   KEXEC_FILE_TYPE_INITRAMFS,
+
+   /*
+* Device Tree Blob containing just the nodes and properties that
+* the kexec_file_load caller wants to add or modify.
+*/
+   KEXEC_FILE_TYPE_PARTIAL_DTB,
+};
+
+struct kexec_file_fd {
+   enum kexec_file_type type;
+   int fd;
+};
+
+struct kexec_fdset {
+   int nr_fds;
+   struct kexec_file_fd fds[0];
+};
 
 /* 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_file.c b/kernel/kexec_file.c
index 113af2f219b9..302427e5ee71 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -116,6 +116,22 @@ void kimage_file_post_load_cleanup(struct kimage *image)
image->image_loader_data = NULL;
 }
 
+/**
+ 

Re: [PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

Hello Balbir,

Thank you for the review!

Am Freitag, 12 August 2016, 18:17:39 schrieb Balbir Singh:
> On Thu, Aug 11, 2016 at 08:03:58PM -0300, Thiago Jung Bauermann wrote:
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3523bf62f328..847d9c31f428 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
> > 
> > id(MODULE, kernel-module)   \
> > id(KEXEC_IMAGE, kexec-image)\
> > id(KEXEC_INITRAMFS, kexec-initramfs)\
> > 
> > +   id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)\
> 
> The backspace is over-indented?

Indeed, I'll fix that. But to keep it aligned with the other backslashes, 
there would be no spaces between it and the final closing parenthesis. 
Either that, or reindent the other backslashes one more level. I think I 
prefer the former.
 
> > @@ -160,6 +180,55 @@ kimage_file_prepare_segments(struct kimage *image,
> > int kernel_fd, int initrd_fd,> 
> > image->initrd_buf_len = size;
> > 
> > }
> > 
> > +   if (flags & KEXEC_FILE_EXTRA_FDS) {
> > +   int nr_fds, i;
> > +   size_t fdset_size;
> > +   char fdset_buf[MAX_FDSET_SIZE];
> 
> Do we really want this on the stack?  I presume the size is not large

It has 132 bytes. Would it be better to use kmalloc instead?

> > +   struct kexec_fdset *fdset = (struct kexec_fdset *) 
fdset_buf;
> > +
> > +   ret = copy_from_user(_fds, ufdset, sizeof(int));
> > +   if (ret) {
> > +   ret = -EFAULT;
> > +   goto out;
> > +   }
> > +
> > +   if (nr_fds > KEXEC_SEGMENT_MAX) {
> 
> We need an nr_fds < 0 check as well

Indeed, I forgot to do that. I will add the check.

> > +   ret = -E2BIG;
> > +   goto out;
> > +   }
> > +
> > +   fdset_size = sizeof(struct kexec_fdset)
> > +   + nr_fds * sizeof(struct kexec_file_fd);
> > +
> > +   ret = copy_from_user(fdset, ufdset, fdset_size);
> 
> Can the user change nr_fds between the two copy_from_users, ideally not,
> but we should validate it.

Good catch. I'll check if nr_fds == fdset->nr_fds and return with an error 
if they're different.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


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

[PATCH v2 2/2] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

From: AKASHI Takahiro 

Device tree blob must be passed to a second kernel on DTB-capable
archs, like powerpc and arm64, but the current kernel interface
lacks this support.

This patch extends kexec_file_load system call by adding an extra
argument to this syscall so that an arbitrary number of file descriptors
can be handed out from user space to the kernel.

long sys_kexec_file_load(int kernel_fd, int initrd_fd,
 unsigned long cmdline_len,
 const char __user *cmdline_ptr,
 unsigned long flags,
 const struct kexec_fdset __user *ufdset);

If KEXEC_FILE_EXTRA_FDS is set to the "flags" argument, the "ufdset"
argument points to the following struct buffer:

struct kexec_fdset {
int nr_fds;
struct kexec_file_fd fds[0];
}

Signed-off-by: AKASHI Takahiro 
Signed-off-by: Thiago Jung Bauermann 
---
 include/linux/fs.h |  1 +
 include/linux/kexec.h  |  7 ++--
 include/linux/syscalls.h   |  4 ++-
 include/uapi/linux/kexec.h | 22 
 kernel/kexec_file.c| 83 ++
 5 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3523bf62f328..847d9c31f428 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2656,6 +2656,7 @@ extern int do_pipe_flags(int *, int);
id(MODULE, kernel-module)   \
id(KEXEC_IMAGE, kexec-image)\
id(KEXEC_INITRAMFS, kexec-initramfs)\
+   id(KEXEC_PARTIAL_DTB, kexec-partial-dtb)\
id(POLICY, security-policy) \
id(MAX_ID, )
 
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 4f85d284ed0b..29202935055d 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -148,7 +148,10 @@ struct kexec_file_ops {
kexec_verify_sig_t *verify_sig;
 #endif
 };
-#endif
+
+int __weak arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf,
+   unsigned long size);
+#endif /* CONFIG_KEXEC_FILE */
 
 struct kimage {
kimage_entry_t head;
@@ -280,7 +283,7 @@ extern int kexec_load_disabled;
 
 /* List of defined/legal kexec file flags */
 #define KEXEC_FILE_FLAGS   (KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
-KEXEC_FILE_NO_INITRAMFS)
+KEXEC_FILE_NO_INITRAMFS | KEXEC_FILE_EXTRA_FDS)
 
 #define VMCOREINFO_BYTES   (4096)
 #define VMCOREINFO_NOTE_NAME   "VMCOREINFO"
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d02239022bd0..fc072bdb74e3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -66,6 +66,7 @@ struct perf_event_attr;
 struct file_handle;
 struct sigaltstack;
 union bpf_attr;
+struct kexec_fdset;
 
 #include 
 #include 
@@ -321,7 +322,8 @@ asmlinkage long sys_kexec_load(unsigned long entry, 
unsigned long nr_segments,
 asmlinkage long sys_kexec_file_load(int kernel_fd, int initrd_fd,
unsigned long cmdline_len,
const char __user *cmdline_ptr,
-   unsigned long flags);
+   unsigned long flags,
+   const struct kexec_fdset __user *ufdset);
 
 asmlinkage long sys_exit(int error_code);
 asmlinkage long sys_exit_group(int error_code);
diff --git a/include/uapi/linux/kexec.h b/include/uapi/linux/kexec.h
index aae5ebf2022b..6279be79efba 100644
--- a/include/uapi/linux/kexec.h
+++ b/include/uapi/linux/kexec.h
@@ -23,6 +23,28 @@
 #define KEXEC_FILE_UNLOAD  0x0001
 #define KEXEC_FILE_ON_CRASH0x0002
 #define KEXEC_FILE_NO_INITRAMFS0x0004
+#define KEXEC_FILE_EXTRA_FDS   0x0008
+
+enum kexec_file_type {
+   KEXEC_FILE_TYPE_KERNEL,
+   KEXEC_FILE_TYPE_INITRAMFS,
+
+   /*
+* Device Tree Blob containing just the nodes and properties that
+* the kexec_file_load caller wants to add or modify.
+*/
+   KEXEC_FILE_TYPE_PARTIAL_DTB,
+};
+
+struct kexec_file_fd {
+   enum kexec_file_type type;
+   int fd;
+};
+
+struct kexec_fdset {
+   int nr_fds;
+   struct kexec_file_fd fds[0];
+};
 
 /* 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_file.c b/kernel/kexec_file.c
index 113af2f219b9..d6803dd884e2 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -25,6 +25,9 @@
 #include 
 #include "kexec_internal.h"
 
+#define MAX_FDSET_SIZE (sizeof(struct kexec_fdset) + \
+   KEXEC_SEGMENT_MAX * sizeof(struct 
kexec_file_fd))
+
 /*
  * Declare these symbols weak so that if