[RFC,3/3] kexec: extend kexec_file_load system call

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 <takahiro.akashi@linaro.org>

---
 include/linux/fs.h         |  1 +
 include/linux/kexec.h      |  2 +-
 include/linux/syscalls.h   |  4 +++-
 include/uapi/linux/kexec.h | 17 ++++++++++++++
 kernel/kexec_file.c        | 57 ++++++++++++++++++++++++++++++++++++++++------
 5 files changed, 72 insertions(+), 9 deletions(-)

-- 
2.9.0

On Fri, Jul 15, 2016 at 09:09:55AM -0400, Vivek Goyal wrote:
> On Tue, Jul 12, 2016 at 10:42:01AM +0900, AKASHI Takahiro wrote:

> 

> [..]

> > -SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,

> > +SYSCALL_DEFINE6(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)

> 

> Can one add more parameters to existing syscall. Can it break existing

> programs with new kernel? I was of the impression that one can't do that.

> But may be I am missing something.


I think the idea was that we would only look at the new params if a new
flags was set, and otherwise it would behave as the old syscall.

Regardless, I think it makes far more sense to add a kexec_file_load2
syscall if we're going to modify the prototype at all. It's a rather
different proposition to the existing syscall, and needs to be treated
as such.

Thanks,
Mark.

On Mon, Jul 18, 2016 at 10:30:24AM +0800, Dave Young wrote:
> On 07/15/16 at 02:19pm, Mark Rutland wrote:

> > On Fri, Jul 15, 2016 at 09:09:55AM -0400, Vivek Goyal wrote:

> > > On Tue, Jul 12, 2016 at 10:42:01AM +0900, AKASHI Takahiro wrote:

> > > 

> > > [..]

> > > > -SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,

> > > > +SYSCALL_DEFINE6(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)

> > > 

> > > Can one add more parameters to existing syscall. Can it break existing

> > > programs with new kernel? I was of the impression that one can't do that.

> > > But may be I am missing something.

> > 

> > I think the idea was that we would only look at the new params if a new

> > flags was set, and otherwise it would behave as the old syscall.

> > 

> > Regardless, I think it makes far more sense to add a kexec_file_load2

> > syscall if we're going to modify the prototype at all. It's a rather

> > different proposition to the existing syscall, and needs to be treated

> > as such.

> 

> I do not think it is worth to add another syscall for extra fds.

> We have open(2) as an example for different numbers of arguments

> already.


Did we change the syscall interface for that?

I was under the impression that there was always one underlying syscall,
and the C library did the right thing to pass the expected information
to the underlying syscall.

That's rather different to changing the underlying syscall.

Regardless of how this is wrapped in userspace, I do not think modifying
the existing prototype is a good idea, and I think this kind of
extension needs to be a new syscall.

Thanks,
Mark.

On Tue, Jul 19, 2016 at 08:55:56AM +0800, Dave Young wrote:
> On 07/18/16 at 11:07am, Mark Rutland wrote:

> > On Mon, Jul 18, 2016 at 10:30:24AM +0800, Dave Young wrote:

> > > I do not think it is worth to add another syscall for extra fds.

> > > We have open(2) as an example for different numbers of arguments

> > > already.

> > 

> > Did we change the syscall interface for that?

> > 

> > I was under the impression that there was always one underlying syscall,

> > and the C library did the right thing to pass the expected information

> > to the underlying syscall.

> 

> I'm not sure kexec_load and kexec_file_load were included in glibc, we use

> syscall directly in kexec-tools.

> 

> kexec_load man pages says there are no wrappers for both kexec_load and

> kexec_file_load in glibc.


For the above, I was talking about how open() was handled.

If there are no userspace wrappers, then the two cases aren't comparable
in the first place...

> > That's rather different to changing the underlying syscall.

> > 

> > Regardless of how this is wrapped in userspace, I do not think modifying

> > the existing prototype is a good idea, and I think this kind of

> > extension needs to be a new syscall.

> 

> Hmm, as I replied to Vivek, there is one case about the flags, previously

> the new flag will be regarded as invalid, but not we extend it it will be

> valid, this maybe the only potential bad case.


It's true that adding suport for new flags will change the behaviour of
what used to be error cases. We generally expect real users to not be
making pointless calls for which they rely on an error being returned in
all cases.

Regardless, this extended syscall changes some underlying assumptions
made with the development of kexec_file_load, and I think treating this
as an extension is not a great idea. From a user's perspective there is
little difference between passing an additional flag or using a
different syscall number, so I don't think that we gain much by altering
the existing prototype relative to allocating a new syscall number.

Thus, I think that if this is necessary it should be treated as a new
syscall.

Thanks,
Mark.

On Tue, Jul 19, 2016 at 08:24:06AM -0400, Vivek Goyal wrote:
> On Tue, Jul 19, 2016 at 11:52:00AM +0100, Mark Rutland wrote:

> > Regardless, this extended syscall changes some underlying assumptions

> > made with the development of kexec_file_load, and I think treating this

> > as an extension is not a great idea. From a user's perspective there is

> > little difference between passing an additional flag or using a

> > different syscall number, so I don't think that we gain much by altering

> > the existing prototype relative to allocating a new syscall number.

> 

> If we are providing/opening up additional flags, I can't think what will

> it break. Same flag was invalid in old kernel but new kernel supports 

> it and will accept it. So it sounds reasonable to me to add new flags.

> 

> If existing users are not broken, then I think it might be a good idea

> to extend existing syscall. Otherwise userspace will have to be modified

> to understand a 3rd syscall also and an additional option will show up

> which asks users to specify which syscall to use. So extending existing

> syscall might keep it little simple for users.


I don't follow.

To use the new feature, you have to modify userspace anyway, as you
require userspace to pass information which it did not previously pass
(in the new arguments added to the syscall).

The presence of a new syscall does not imply the absence of the old
syscall, so you can always use that be default unless the user asks for
asomething only the new syscall provides. Regardless of the
syscall/flags difference, you still have to detect whether the new
functionality is present somehow.

> BTW, does kexec_load() needs to be modified too to handle DT?


No, at least for arm64. In the kexec_load case userspace provides the
DTB as a raw segment, and the user-provided purgatory sets up registers
to pass that to the new kernel.

Thanks,
Mark.