* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Josh Triplett @ 2015-08-01 4:32 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Kees Cook, David Drysdale, Ingo Molnar, Linux API,
Michael Kerrisk, Andrew Morton, Arnd Bergmann, Shuah Khan,
Jonathan Corbet, Eric B Munson, Randy Dunlap, Andrea Arcangeli,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <CALCETrXwrf--FG+KdbG-oBixhG+odB7ukW_F7CkBGb0KQ7ZSVA@mail.gmail.com>
On Fri, Jul 31, 2015 at 03:54:56PM -0700, Andy Lutomirski wrote:
> On Fri, Jul 31, 2015 at 3:08 PM, <josh@joshtriplett.org> wrote:
> > On Fri, Jul 31, 2015 at 02:19:29PM -0700, Andy Lutomirski wrote:
> >> On Fri, Jul 31, 2015 at 1:59 PM, <josh@joshtriplett.org> wrote:
> >> > Agreed. I think the proposal above would be a net improvement, but
> >> > ideally you'd want something that's annotated and generates automatic
> >> > marshalling code.
> >> >
> >>
> >> I assume this is idle musing. If, however, we were to actually do
> >> this, I'd suggest we seriously consider speaking the Cap'n Proto
> >> serialization format. It's quite nice, it encodes and decodes *very*
> >> quickly and, unlike TLV schemes, you don't have to read it in order,
> >> making the read-side code less awkward.
> >
> > That seems like *massive* overkill for a kernel<->userspace syscall
> > interface. I was more thinking about having a few standardized marshal
> > types, and incrementally adding more when more patterns show up. For a
> > first pass, just automatically running copy_from_user and
> > copy_param_struct on appropriate sets of __user parameters identified as
> > such in a structured text file seems quite sufficient. (Plus
> > automatically generating syscalls.h from that.)
>
> If a param struct does the trick, then I agree. It's when you start
> having lists and other variable-size stuff that it gets messier.
Sure, agreed. But I really hope we don't create new kernel ABIs that
involve constructs like that.
- Josh Triplett
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Andy Lutomirski @ 2015-07-31 22:54 UTC (permalink / raw)
To: Josh Triplett
Cc: Kees Cook, David Drysdale, Ingo Molnar, Linux API,
Michael Kerrisk, Andrew Morton, Arnd Bergmann, Shuah Khan,
Jonathan Corbet, Eric B Munson, Randy Dunlap, Andrea Arcangeli,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <20150731220828.GA30890@cloud>
On Fri, Jul 31, 2015 at 3:08 PM, <josh@joshtriplett.org> wrote:
> On Fri, Jul 31, 2015 at 02:19:29PM -0700, Andy Lutomirski wrote:
>> On Fri, Jul 31, 2015 at 1:59 PM, <josh@joshtriplett.org> wrote:
>> > Agreed. I think the proposal above would be a net improvement, but
>> > ideally you'd want something that's annotated and generates automatic
>> > marshalling code.
>> >
>>
>> I assume this is idle musing. If, however, we were to actually do
>> this, I'd suggest we seriously consider speaking the Cap'n Proto
>> serialization format. It's quite nice, it encodes and decodes *very*
>> quickly and, unlike TLV schemes, you don't have to read it in order,
>> making the read-side code less awkward.
>
> That seems like *massive* overkill for a kernel<->userspace syscall
> interface. I was more thinking about having a few standardized marshal
> types, and incrementally adding more when more patterns show up. For a
> first pass, just automatically running copy_from_user and
> copy_param_struct on appropriate sets of __user parameters identified as
> such in a structured text file seems quite sufficient. (Plus
> automatically generating syscalls.h from that.)
If a param struct does the trick, then I agree. It's when you start
having lists and other variable-size stuff that it gets messier.
For reference, my Cap'n Proto parser (just the basic structure of
messages, not the actual schema bits, although the latter is more or
less just some structs) is about 300 lines of code. It's arguably
simpler than nlattr, once you throw nesting in.
--Andy
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: josh-iaAMLnmF4UmaiuxdJuQwMA @ 2015-07-31 22:08 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Kees Cook, David Drysdale, Ingo Molnar, Linux API,
Michael Kerrisk, Andrew Morton, Arnd Bergmann, Shuah Khan,
Jonathan Corbet, Eric B Munson, Randy Dunlap, Andrea Arcangeli,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <CALCETrUkMXvFRKdTH7ekY7FyGvbKDDJbf7L0shgs5R-Hep6bVA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Jul 31, 2015 at 02:19:29PM -0700, Andy Lutomirski wrote:
> On Fri, Jul 31, 2015 at 1:59 PM, <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> > Agreed. I think the proposal above would be a net improvement, but
> > ideally you'd want something that's annotated and generates automatic
> > marshalling code.
> >
>
> I assume this is idle musing. If, however, we were to actually do
> this, I'd suggest we seriously consider speaking the Cap'n Proto
> serialization format. It's quite nice, it encodes and decodes *very*
> quickly and, unlike TLV schemes, you don't have to read it in order,
> making the read-side code less awkward.
That seems like *massive* overkill for a kernel<->userspace syscall
interface. I was more thinking about having a few standardized marshal
types, and incrementally adding more when more patterns show up. For a
first pass, just automatically running copy_from_user and
copy_param_struct on appropriate sets of __user parameters identified as
such in a structured text file seems quite sufficient. (Plus
automatically generating syscalls.h from that.)
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Andy Lutomirski @ 2015-07-31 21:19 UTC (permalink / raw)
To: Josh Triplett
Cc: Kees Cook, David Drysdale, Ingo Molnar, Linux API,
Michael Kerrisk, Andrew Morton, Arnd Bergmann, Shuah Khan,
Jonathan Corbet, Eric B Munson, Randy Dunlap, Andrea Arcangeli,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <20150731205941.GA30362@cloud>
On Fri, Jul 31, 2015 at 1:59 PM, <josh@joshtriplett.org> wrote:
> On Fri, Jul 31, 2015 at 11:56:06AM -0700, Kees Cook wrote:
>> On Thu, Jul 30, 2015 at 6:02 PM, Josh Triplett <josh@joshtriplett.org> wrote:
>> > On Thu, Jul 30, 2015 at 01:03:43PM -0700, Kees Cook wrote:
>> >> On Thu, Jul 30, 2015 at 12:04 PM, Josh Triplett <josh@joshtriplett.org> wrote:
>> >> > On Thu, Jul 30, 2015 at 11:21:54AM -0700, Kees Cook wrote:
>> >> >> I like this, it's a good description of both options. I'm still biased
>> >> >> about the approach: I prefer flags, since pointers to user structures
>> >> >> complicate syscall filtering. ;)
>> >> >
>> >> > Seems like we should do two things to make that easier:
>> >> >
>> >> > 1) Create a standardized kernel mechanism for parameter-struct handling,
>> >> > implementing the recommendations mentioned here.
>> >>
>> >> It's been suggested in the past that nlmsg is appropriate for such a
>> >> thing, but I remain suspicious. :)
>> >
>> > Likewise. :)
>> >
>> >> > 2) Integrate into that mechanism a way to filter the resulting parameter
>> >> > struct with BPF *after* it has been copied to kernel space (and thus
>> >> > can no longer be tampered with).
>> >>
>> >> Yeah, this is a irritating part: the structures operated on are copied
>> >> from userspace adhoc in each syscall. Doing argument checking would
>> >> mean double copies initially, and perhaps teaching syscalls about
>> >> optional "already copied" arguments or something as an optimization.
>> >
>> > No, double copies can't work for security reasons. Because otherwise
>> > you could race the kernel from another thread, substituting different
>> > values after the check and before the use.
>>
>> Right, the double copy method would require setting up a per-thread
>> userspace memory mapping that was read-only from userspace but
>> writable from kernel space.
>
> Which seems like a lot more trouble than just copying it once.
>
>> > I think the right API looks *roughly* like this:
>> >
>> > int _copy_param_struct(size_t kernel_len, void *kernel_struct, size_t user_len, void __user *user_struct)
>> > {
>> > if (user_len > kernel_len)
>> > return -EINVAL;
>> > if (user_len && copy_from_user(kernel_struct, user_struct, user_len))
>> > return -EFAULT;
>> > if (user_len < kernel_len)
>> > memset(kernel_struct + user_len, 0, kernel_len - user_len);
>> > return 0;
>> > }
>> >
>> > #define copy_param_struct(kernel_struct, user_len, user_struct) _copy_param_struct( \
>> > sizeof(*kernel_struct) + BUILD_BUG_ON_ZERO(!__same_type(*kernel_struct, *user_struct)), \
>> > kernel_struct, user_len, user_struct)
>> >
>> >
>> > Then the syscall looks like this:
>> >
>> > SYSCALL_DEFINEn(xyzzy, ..., ..., size_t user_params_len, struct xyzzy_params __user *user_params)
>> > {
>> > int ret;
>> > struct xyzzy_params params;
>> >
>> > ret = copy_param_struct(¶ms, user_params_len, user_params);
>> > if (ret)
>> > return ret;
>> > ...
>> >
>> >
>> > And you could then hook copy_params_struct to add arbitrary additional
>> > syscall parameter validation. Bonus if there's some way to make the
>> > copy and validation occur before the syscall is ever invoked, rather
>> > than inside the syscall, but that would require adding fancier syscall
>> > definition mechanisms that autogenerate such code.
>>
>> The trouble is that the hook for the syscall (both seccomp and ptrace)
>> happens before the sys_* function executes. So the param extract
>> suddenly becomes optional. As in, did ptrace/seccomp already extract
>> the args? If so, use that copy, else copy them out myself now that I
>> need them, etc.
>>
>> It's entirely doable, but it's going to require some careful design.
>
> Agreed. I think the proposal above would be a net improvement, but
> ideally you'd want something that's annotated and generates automatic
> marshalling code.
>
I assume this is idle musing. If, however, we were to actually do
this, I'd suggest we seriously consider speaking the Cap'n Proto
serialization format. It's quite nice, it encodes and decodes *very*
quickly and, unlike TLV schemes, you don't have to read it in order,
making the read-side code less awkward.
(I wouldn't suggest that we use the official C++11 library, but the
format might be a good idea.)
--Andy
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: josh @ 2015-07-31 20:59 UTC (permalink / raw)
To: Kees Cook
Cc: David Drysdale, Ingo Molnar, Linux API, Michael Kerrisk,
Andrew Morton, Arnd Bergmann, Shuah Khan, Jonathan Corbet,
Eric B Munson, Randy Dunlap, Andrea Arcangeli, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, Oleg Nesterov, Linus Torvalds,
Greg Kroah-Hartman, Andy Lutomirski, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <CAGXu5jLethUkiq1QEZg-uRkOzofa6M8WCT8T7qAByWy5L4xHBw@mail.gmail.com>
On Fri, Jul 31, 2015 at 11:56:06AM -0700, Kees Cook wrote:
> On Thu, Jul 30, 2015 at 6:02 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Thu, Jul 30, 2015 at 01:03:43PM -0700, Kees Cook wrote:
> >> On Thu, Jul 30, 2015 at 12:04 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> >> > On Thu, Jul 30, 2015 at 11:21:54AM -0700, Kees Cook wrote:
> >> >> I like this, it's a good description of both options. I'm still biased
> >> >> about the approach: I prefer flags, since pointers to user structures
> >> >> complicate syscall filtering. ;)
> >> >
> >> > Seems like we should do two things to make that easier:
> >> >
> >> > 1) Create a standardized kernel mechanism for parameter-struct handling,
> >> > implementing the recommendations mentioned here.
> >>
> >> It's been suggested in the past that nlmsg is appropriate for such a
> >> thing, but I remain suspicious. :)
> >
> > Likewise. :)
> >
> >> > 2) Integrate into that mechanism a way to filter the resulting parameter
> >> > struct with BPF *after* it has been copied to kernel space (and thus
> >> > can no longer be tampered with).
> >>
> >> Yeah, this is a irritating part: the structures operated on are copied
> >> from userspace adhoc in each syscall. Doing argument checking would
> >> mean double copies initially, and perhaps teaching syscalls about
> >> optional "already copied" arguments or something as an optimization.
> >
> > No, double copies can't work for security reasons. Because otherwise
> > you could race the kernel from another thread, substituting different
> > values after the check and before the use.
>
> Right, the double copy method would require setting up a per-thread
> userspace memory mapping that was read-only from userspace but
> writable from kernel space.
Which seems like a lot more trouble than just copying it once.
> > I think the right API looks *roughly* like this:
> >
> > int _copy_param_struct(size_t kernel_len, void *kernel_struct, size_t user_len, void __user *user_struct)
> > {
> > if (user_len > kernel_len)
> > return -EINVAL;
> > if (user_len && copy_from_user(kernel_struct, user_struct, user_len))
> > return -EFAULT;
> > if (user_len < kernel_len)
> > memset(kernel_struct + user_len, 0, kernel_len - user_len);
> > return 0;
> > }
> >
> > #define copy_param_struct(kernel_struct, user_len, user_struct) _copy_param_struct( \
> > sizeof(*kernel_struct) + BUILD_BUG_ON_ZERO(!__same_type(*kernel_struct, *user_struct)), \
> > kernel_struct, user_len, user_struct)
> >
> >
> > Then the syscall looks like this:
> >
> > SYSCALL_DEFINEn(xyzzy, ..., ..., size_t user_params_len, struct xyzzy_params __user *user_params)
> > {
> > int ret;
> > struct xyzzy_params params;
> >
> > ret = copy_param_struct(¶ms, user_params_len, user_params);
> > if (ret)
> > return ret;
> > ...
> >
> >
> > And you could then hook copy_params_struct to add arbitrary additional
> > syscall parameter validation. Bonus if there's some way to make the
> > copy and validation occur before the syscall is ever invoked, rather
> > than inside the syscall, but that would require adding fancier syscall
> > definition mechanisms that autogenerate such code.
>
> The trouble is that the hook for the syscall (both seccomp and ptrace)
> happens before the sys_* function executes. So the param extract
> suddenly becomes optional. As in, did ptrace/seccomp already extract
> the args? If so, use that copy, else copy them out myself now that I
> need them, etc.
>
> It's entirely doable, but it's going to require some careful design.
Agreed. I think the proposal above would be a net improvement, but
ideally you'd want something that's annotated and generates automatic
marshalling code.
- Josh Triplett
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Kees Cook @ 2015-07-31 18:56 UTC (permalink / raw)
To: Josh Triplett
Cc: David Drysdale, Ingo Molnar, Linux API, Michael Kerrisk,
Andrew Morton, Arnd Bergmann, Shuah Khan, Jonathan Corbet,
Eric B Munson, Randy Dunlap, Andrea Arcangeli, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, Oleg Nesterov, Linus Torvalds,
Greg Kroah-Hartman, Andy Lutomirski, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <20150731010234.GA7265@x>
On Thu, Jul 30, 2015 at 6:02 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Thu, Jul 30, 2015 at 01:03:43PM -0700, Kees Cook wrote:
>> On Thu, Jul 30, 2015 at 12:04 PM, Josh Triplett <josh@joshtriplett.org> wrote:
>> > On Thu, Jul 30, 2015 at 11:21:54AM -0700, Kees Cook wrote:
>> >> I like this, it's a good description of both options. I'm still biased
>> >> about the approach: I prefer flags, since pointers to user structures
>> >> complicate syscall filtering. ;)
>> >
>> > Seems like we should do two things to make that easier:
>> >
>> > 1) Create a standardized kernel mechanism for parameter-struct handling,
>> > implementing the recommendations mentioned here.
>>
>> It's been suggested in the past that nlmsg is appropriate for such a
>> thing, but I remain suspicious. :)
>
> Likewise. :)
>
>> > 2) Integrate into that mechanism a way to filter the resulting parameter
>> > struct with BPF *after* it has been copied to kernel space (and thus
>> > can no longer be tampered with).
>>
>> Yeah, this is a irritating part: the structures operated on are copied
>> from userspace adhoc in each syscall. Doing argument checking would
>> mean double copies initially, and perhaps teaching syscalls about
>> optional "already copied" arguments or something as an optimization.
>
> No, double copies can't work for security reasons. Because otherwise
> you could race the kernel from another thread, substituting different
> values after the check and before the use.
Right, the double copy method would require setting up a per-thread
userspace memory mapping that was read-only from userspace but
writable from kernel space.
> I think the right API looks *roughly* like this:
>
> int _copy_param_struct(size_t kernel_len, void *kernel_struct, size_t user_len, void __user *user_struct)
> {
> if (user_len > kernel_len)
> return -EINVAL;
> if (user_len && copy_from_user(kernel_struct, user_struct, user_len))
> return -EFAULT;
> if (user_len < kernel_len)
> memset(kernel_struct + user_len, 0, kernel_len - user_len);
> return 0;
> }
>
> #define copy_param_struct(kernel_struct, user_len, user_struct) _copy_param_struct( \
> sizeof(*kernel_struct) + BUILD_BUG_ON_ZERO(!__same_type(*kernel_struct, *user_struct)), \
> kernel_struct, user_len, user_struct)
>
>
> Then the syscall looks like this:
>
> SYSCALL_DEFINEn(xyzzy, ..., ..., size_t user_params_len, struct xyzzy_params __user *user_params)
> {
> int ret;
> struct xyzzy_params params;
>
> ret = copy_param_struct(¶ms, user_params_len, user_params);
> if (ret)
> return ret;
> ...
>
>
> And you could then hook copy_params_struct to add arbitrary additional
> syscall parameter validation. Bonus if there's some way to make the
> copy and validation occur before the syscall is ever invoked, rather
> than inside the syscall, but that would require adding fancier syscall
> definition mechanisms that autogenerate such code.
The trouble is that the hook for the syscall (both seccomp and ptrace)
happens before the sys_* function executes. So the param extract
suddenly becomes optional. As in, did ptrace/seccomp already extract
the args? If so, use that copy, else copy them out myself now that I
need them, etc.
It's entirely doable, but it's going to require some careful design.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCHv3 1/1] Documentation: describe how to add a system call
From: Josh Triplett @ 2015-07-31 15:15 UTC (permalink / raw)
To: David Drysdale
Cc: linux-api, Michael Kerrisk, Andrew Morton, Arnd Bergmann,
Shuah Khan, Jonathan Corbet, Eric B Munson, Randy Dunlap,
Cyril Hrubis, Andrea Arcangeli, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Oleg Nesterov, Linus Torvalds, Greg Kroah-Hartman,
Andy Lutomirski, Al Viro, Rusty Russell, Peter Zijlstra,
Vivek Goyal, Alexei Starovoitov, David Herrmann
In-Reply-To: <1438353346-67177-2-git-send-email-drysdale@google.com>
On Fri, Jul 31, 2015 at 03:35:46PM +0100, David Drysdale wrote:
> Add a document describing the process of adding a new system call,
> including the need for a flags argument for future compatibility, and
> covering 32-bit/64-bit concerns (albeit in an x86-centric way).
>
> Signed-off-by: David Drysdale <drysdale@google.com>
> Reviewed-by: Michael Kerrisk <mtk.manpages@gmail.com>
> Reviewed-by: Eric B Munson <emunson@akamai.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
Other than the change to the mention of capabilities, this now looks
ready to me. With that item fixed:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> Documentation/adding-syscalls.txt | 531 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 531 insertions(+)
> create mode 100644 Documentation/adding-syscalls.txt
>
> diff --git a/Documentation/adding-syscalls.txt b/Documentation/adding-syscalls.txt
> new file mode 100644
> index 000000000000..633691c5fb45
> --- /dev/null
> +++ b/Documentation/adding-syscalls.txt
> @@ -0,0 +1,531 @@
> +Adding a New System Call
> +========================
> +
> +This document describes what's involved in adding a new system call to the
> +Linux kernel, over and above the normal submission advice in
> +Documentation/SubmittingPatches.
> +
> +
> +System Call Alternatives
> +------------------------
> +
> +The first thing to consider when adding a new system call is whether one of
> +the alternatives might be suitable instead. Although system calls are the
> +most traditional and most obvious interaction points between userspace and the
> +kernel, there are other possibilities -- choose what fits best for your
> +interface.
> +
> + - If the operations involved can be made to look like a filesystem-like
> + object, it may make more sense to create a new filesystem or device. This
> + also makes it easier to encapsulate the new functionality in a kernel module
> + rather than requiring it to be built into the main kernel.
> + - If the new functionality involves operations where the kernel notifies
> + userspace that something has happened, then returning a new file
> + descriptor for the relevant object allows userspace to use
> + poll/select/epoll to receive that notification.
> + - However, operations that don't map to read(2)/write(2)-like operations
> + have to be implemented as ioctl(2) requests, which can lead to a
> + somewhat opaque API.
> + - If you're just exposing runtime system information, a new node in sysfs
> + (see Documentation/filesystems/sysfs.txt) or the /proc filesystem may be
> + more appropriate. However, access to these mechanisms requires that the
> + relevant filesystem is mounted, which might not always be the case (e.g.
> + in a namespaced/sandboxed/chrooted environment). Avoid adding any API to
> + debugfs, as this is not considered a 'production' interface to userspace.
> + - If the operation is specific to a particular file or file descriptor, then
> + an additional fcntl(2) command option may be more appropriate. However,
> + fcntl(2) is a multiplexing system call that hides a lot of complexity, so
> + this option is best for when the new function is closely analogous to
> + existing fcntl(2) functionality, or the new functionality is very simple
> + (for example, getting/setting a simple flag related to a file descriptor).
> + - If the operation is specific to a particular task or process, then an
> + additional prctl(2) command option may be more appropriate. As with
> + fcntl(2), this system call is a complicated multiplexor so is best reserved
> + for near-analogs of existing prctl() commands or getting/setting a simple
> + flag related to a process.
> +
> +
> +Designing the API: Planning for Extension
> +-----------------------------------------
> +
> +A new system call forms part of the API of the kernel, and has to be supported
> +indefinitely. As such, it's a very good idea to explicitly discuss the
> +interface on the kernel mailing list, and it's important to plan for future
> +extensions of the interface.
> +
> +(The syscall table is littered with historical examples where this wasn't done,
> +together with the corresponding follow-up system calls -- eventfd/eventfd2,
> +dup2/dup3, inotify_init/inotify_init1, pipe/pipe2, renameat/renameat2 -- so
> +learn from the history of the kernel and plan for extensions from the start.)
> +
> +For simpler system calls that only take a couple of arguments, the preferred way
> +to allow for future extensibility is to include a flags argument to the system
> +call. To make sure that userspace programs can safely use flags between kernel
> +versions, check whether the flags value holds any unknown flags, and reject the
> +sycall (with EINVAL) if it does:
> +
> + if (flags & ~(THING_FLAG1 | THING_FLAG2 | THING_FLAG3))
> + return -EINVAL;
> +
> +(If no flags values are used yet, check that the flags argument is zero.)
> +
> +For more sophisticated system calls that involve a larger number of arguments,
> +it's preferred to encapsulate the majority of the arguments into a structure
> +that is passed in by pointer. Such a structure can cope with future extension
> +by including a size argument in the structure:
> +
> + struct xyzzy_params {
> + u32 size; /* userspace sets p->size = sizeof(struct xyzzy_params) */
> + u32 param_1;
> + u64 param_2;
> + u64 param_3;
> + };
> +
> +As long as any subsequently added field, say param_4, is designed so that a zero
> +value gives the previous behaviour, then this allows both directions of version
> +mismatch:
> +
> + - To cope with a later userspace program calling an older kernel, the kernel
> + code should check that any memory beyond the size of the structure that it
> + expects is zero (effectively checking that param_4 == 0).
> + - To cope with an older userspace program calling a newer kernel, the kernel
> + code can zero-extend a smaller instance of the structure (effectively setting
> + param_4 = 0).
> +
> +See perf_event_open(2) and the perf_copy_attr() function (in
> +kernel/events/core.c) for an example of this approach.
> +
> +
> +Designing the API: Other Considerations
> +---------------------------------------
> +
> +If your new system call allows userspace to refer to a kernel object, it
> +should use a file descriptor as the handle for that object -- don't invent a
> +new type of userspace object handle when the kernel already has mechanisms and
> +well-defined semantics for using file descriptors.
> +
> +If your new xyzzy(2) system call does return a new file descriptor, then the
> +flags argument should include a value that is equivalent to setting O_CLOEXEC
> +on the new FD. This makes it possible for userspace to close the timing
> +window between xyzzy() and calling fcntl(fd, F_SETFD, FD_CLOEXEC), where an
> +unexpected fork() and execve() in another thread could leak a descriptor to
> +the exec'ed program. (However, resist the temptation to re-use the actual value
> +of the O_CLOEXEC constant, as it is architecture-specific and is part of a
> +numbering space of O_* flags that is fairly full.)
> +
> +If your system call returns a new file descriptor, you should also consider
> +what it means to use the poll(2) family of system calls on that file
> +descriptor. Making a file descriptor ready for reading or writing is the
> +normal way for the kernel to indicate to userspace that an event has
> +occurred on the corresponding kernel object.
> +
> +If your new xyzzy(2) system call involves a filename argument:
> +
> + int sys_xyzzy(const char __user *path, ..., unsigned int flags);
> +
> +you should also consider whether an xyzzyat(2) version is more appropriate:
> +
> + int sys_xyzzyat(int dfd, const char __user *path, ..., unsigned int flags);
> +
> +This allows more flexibility for how userspace specifies the file in question;
> +in particular it allows userspace to request the functionality for an
> +already-opened file descriptor using the AT_EMPTY_PATH flag, effectively giving
> +an fxyzzy(3) operation for free:
> +
> + - xyzzyat(AT_FDCWD, path, ..., 0) is equivalent to xyzzy(path,...)
> + - xyzzyat(fd, "", ..., AT_EMPTY_PATH) is equivalent to fxyzzy(fd, ...)
> +
> +(For more details on the rationale of the *at() calls, see the openat(2) man
> +page; for an example of AT_EMPTY_PATH, see the statat(2) man page.)
> +
> +If your new xyzzy(2) system call involves a parameter describing an offset
> +within a file, make its type loff_t so that 64-bit offsets can be supported
> +even on 32-bit architectures.
> +
> +If your new xyzzy(2) system call involves privileged functionality, it
> +needs to be governed by the appropriate Linux capability bit (checked with a
> +call to capable()), as described in the capabilities(7) man page.
> +
> + - If there is an existing capability that governs related functionality, then
> + use that. However, avoid combining lots of only vaguely related functions
> + together under the same bit, as this goes against capabilities' purpose of
> + splitting the power of root. In particular, avoid adding new uses of the
> + already overly-general CAP_SYS_ADMIN capability.
> + - If there is no related capability, then consider adding a new capability
> + bit -- but bear in mind that the numbering space is limited, and each new
> + bit needs to be understood and administered by sysadmins.
> +
> +If your new xyzzy(2) system call manipulates a process other than the calling
> +process, it should be restricted (using a call to ptrace_may_access()) so that
> +only a calling process with the same permissions as the target process, or
> +with the necessary capabilities, can manipulate the target process.
> +
> +Finally, be aware that some non-x86 architectures have an easier time if
> +system call parameters that are explicitly 64-bit fall on odd-numbered
> +arguments (i.e. parameter 1, 3, 5), to allow use of contiguous pairs of 32-bit
> +registers. (This concern does not apply if the arguments are part of a
> +structure that's passed in by pointer.)
> +
> +
> +Proposing the API
> +-----------------
> +
> +To make new system calls easy to review, it's best to divide up the patchset
> +into separate chunks. These should include at least the following items as
> +distinct commits (each of which is described further below):
> +
> + - The core implementation of the system call, together with prototypes,
> + generic numbering, Kconfig changes and fallback stub implementation.
> + - Wiring up of the new system call for one particular architecture, usually
> + x86 (including all of x86_64, x86_32 and x32).
> + - A demonstration of the use of the new system call in userspace via a
> + selftest in tools/testing/selftests/.
> + - A draft man-page for the new system call, either as plain text in the
> + cover letter, or as a patch to the (separate) man-pages repository.
> +
> +New system call proposals, like any change to the kernel's API, should always
> +be cc'ed to linux-api@vger.kernel.org
> +
> +
> +Generic System Call Implementation
> +----------------------------------
> +
> +The main entry point for your new xyzzy(2) system call will be called
> +sys_xyzzy(), but you add this entry point with the appropriate
> +SYSCALL_DEFINEn() macro rather than explicitly. The 'n' indicates the number
> +of arguments to the system call, and the macro takes the system call name
> +followed by the (type, name) pairs for the parameters as arguments. Using
> +this macro allows metadata about the new system call to be made available for
> +other tools.
> +
> +The new entry point also needs a corresponding function prototype, in
> +include/linux/syscalls.h, marked as asmlinkage to match the way that system
> +calls are invoked:
> +
> + asmlinkage long sys_xyzzy(...);
> +
> +Some architectures (e.g. x86) have their own architecture-specific syscall
> +tables, but several other architectures share a generic syscall table. Add your
> +new system call to the generic list by adding an entry to the list in
> +include/uapi/asm-generic/unistd.h:
> +
> + #define __NR_xyzzy 292
> + __SYSCALL(__NR_xyzzy, sys_xyzzy)
> +
> +Also update the __NR_syscalls count to reflect the additional system call, and
> +note that if multiple new system calls are added in the same merge window,
> +your new syscall number may get adjusted to resolve conflicts.
> +
> +The file kernel/sys_ni.c provides a fallback stub implementation of each system
> +call, returning -ENOSYS. Add your new system call here too:
> +
> + cond_syscall(sys_xyzzy);
> +
> +Your new kernel functionality, and the system call that controls it, should
> +normally be optional, so add a CONFIG option (typically to init/Kconfig) for
> +it. As usual for new CONFIG options:
> +
> + - Include a description of the new functionality and system call controlled
> + by the option.
> + - Make the option depend on EXPERT if it should be hidden from normal users.
> + - Make any new source files implementing the function dependent on the CONFIG
> + option in the Makefile (e.g. "obj-$(CONFIG_XYZZY_SYSCALL) += xyzzy.c").
> + - Double check that the kernel still builds with the new CONFIG option turned
> + off.
> +
> +To summarize, you need a commit that includes:
> +
> + - CONFIG option for the new function, normally in init/Kconfig
> + - SYSCALL_DEFINEn(xyzzy, ...) for the entry point
> + - corresponding prototype in include/linux/syscalls.h
> + - generic table entry in include/uapi/asm-generic/unistd.h
> + - fallback stub in kernel/sys_ni.c
> +
> +
> +x86 System Call Implementation
> +------------------------------
> +
> +To wire up your new system call for x86 platforms, you need to update the
> +master syscall tables. Assuming your new system call isn't special in some
> +way (see below), this involves a "common" entry (for x86_64 and x32) in
> +arch/x86/entry/syscalls/syscall_64.tbl:
> +
> + 333 common xyzzy sys_xyzzy
> +
> +and an "i386" entry in arch/x86/entry/syscalls/syscall_32.tbl:
> +
> + 380 i386 xyzzy sys_xyzzy
> +
> +Again, these numbers are liable to be changed if there are conflicts in the
> +relevant merge window.
> +
> +
> +Compatibility System Calls (Generic)
> +------------------------------------
> +
> +For most system calls the same 64-bit implementation can be invoked even when
> +the userspace program is itself 32-bit; even if the system call's parameters
> +include an explicit pointer, this is handled transparently.
> +
> +However, there are a couple of situations where a compatibility layer is
> +needed to cope with size differences between 32-bit and 64-bit.
> +
> +The first is if the 64-bit kernel also supports 32-bit userspace programs, and
> +so needs to parse areas of (__user) memory that could hold either 32-bit or
> +64-bit values. In particular, this is needed whenever a system call argument
> +is:
> +
> + - a pointer to a pointer
> + - a pointer to a struct containing a pointer (e.g. struct iovec __user *)
> + - a pointer to a varying sized integral type (time_t, off_t, long, ...)
> + - a pointer to a struct containing a varying sized integral type.
> +
> +The second situation that requires a compatibility layer is if one of the
> +system call's arguments has a type that is explicitly 64-bit even on a 32-bit
> +architecture, for example loff_t or __u64. In this case, a value that arrives
> +at a 64-bit kernel from a 32-bit application will be split into two 32-bit
> +values, which then need to be re-assembled in the compatibility layer.
> +
> +(Note that a system call argument that's a pointer to an explicit 64-bit type
> +does *not* need a compatibility layer; for example, splice(2)'s arguments of
> +type loff_t __user * do not trigger the need for a compat_ system call.)
> +
> +The compatibility version of the system call is called compat_sys_xyzzy(), and
> +is added with the COMPAT_SYSCALL_DEFINEn() macro, analogously to
> +SYSCALL_DEFINEn. This version of the implementation runs as part of a 64-bit
> +kernel, but expects to receive 32-bit parameter values and does whatever is
> +needed to deal with them. (Typically, the compat_sys_ version converts the
> +values to 64-bit versions and either calls on to the sys_ version, or both of
> +them call a common inner implementation function.)
> +
> +The compat entry point also needs a corresponding function prototype, in
> +include/linux/compat.h, marked as asmlinkage to match the way that system
> +calls are invoked:
> +
> + asmlinkage long compat_sys_xyzzy(...);
> +
> +If the system call involves a structure that is laid out differently on 32-bit
> +and 64-bit systems, say struct xyzzy_args, then the include/linux/compat.h
> +header file should also include a compat version of the structure (struct
> +compat_xyzzy_args) where each variable-size field has the appropriate compat_
> +type that corresponds to the type in struct xyzzy_args. The
> +compat_sys_xyzzy() routine can then use this compat_ structure to parse the
> +arguments from a 32-bit invocation.
> +
> +For example, if there are fields:
> +
> + struct xyzzy_args {
> + const char __user *ptr;
> + __kernel_long_t varying_val;
> + u64 fixed_val;
> + /* ... */
> + };
> +
> +in struct xyzzy_args, then struct compat_xyzzy_args would have:
> +
> + struct compat_xyzzy_args {
> + compat_uptr_t ptr;
> + compat_long_t varying_val;
> + u64 fixed_val;
> + /* ... */
> + };
> +
> +The generic system call list also needs adjusting to allow for the compat
> +version; the entry in include/uapi/asm-generic/unistd.h should use
> +__SC_COMP rather than __SYSCALL:
> +
> + #define __NR_xyzzy 292
> + __SC_COMP(__NR_xyzzy, sys_xyzzy, compat_sys_xyzzy)
> +
> +To summarize, you need:
> +
> + - a COMPAT_SYSCALL_DEFINEn(xyzzy, ...) for the compat entry point
> + - corresponding prototype in include/linux/compat.h
> + - (if needed) 32-bit mapping struct in include/linux/compat.h
> + - instance of __SC_COMP not __SYSCALL in include/uapi/asm-generic/unistd.h
> +
> +
> +Compatibility System Calls (x86)
> +--------------------------------
> +
> +To wire up the x86 architecture of a system call with a compatibility version,
> +the entries in the syscall tables need to be adjusted.
> +
> +First, the entry in arch/x86/entry/syscalls/syscall_32.tbl gets an extra
> +column to indicate that a 32-bit userspace program running on a 64-bit kernel
> +should hit the compat entry point:
> +
> + 380 i386 xyzzy sys_xyzzy compat_sys_xyzzy
> +
> +Second, you need to figure out what should happen for the x32 ABI version of
> +the new system call. There's a choice here: the layout of the arguments
> +should either match the 64-bit version or the 32-bit version.
> +
> +If there's a pointer-to-a-pointer involved, the decision is easy: x32 is
> +ILP32, so the layout should match the 32-bit version, and the entry in
> +arch/x86/entry/syscalls/syscall_64.tbl is split so that x32 programs hit the
> +compatibility wrapper:
> +
> + 333 64 xyzzy sys_xyzzy
> + ...
> + 555 x32 xyzzy compat_sys_xyzzy
> +
> +If no pointers are involved, then it is preferable to re-use the 64-bit system
> +call for the x32 ABI (and consequently the entry in
> +arch/x86/entry/syscalls/syscall_64.tbl is unchanged).
> +
> +In either case, you should check that the types involved in your argument
> +layout do indeed map exactly from x32 (-mx32) to either the 32-bit (-m32) or
> +64-bit (-m64) equivalents.
> +
> +
> +System Calls Returning Elsewhere
> +--------------------------------
> +
> +For most system calls, once the system call is complete the user program
> +continues exactly where it left off -- at the next instruction, with the same
> +stack and registers as before the system call, and with the same virtual
> +memory space.
> +
> +However, a few system calls do things differently. They might return to a
> +different location (rt_sigreturn) or change the memory space (fork/vfork/clone)
> +or even architecture (execve/execveat) of the program.
> +
> +To allow for this, the kernel implementation of the system call may need to
> +save and restore additional registers to the kernel stack, allowing complete
> +control of where and how execution continues after the system call.
> +
> +This is arch-specific, but typically involves defining assembly entry points
> +that save/restore additional registers and invoke the real system call entry
> +point.
> +
> +For x86_64, this is implemented as a stub_xyzzy entry point in
> +arch/x86/entry/entry_64.S, and the entry in the syscall table
> +(arch/x86/entry/syscalls/syscall_64.tbl) is adjusted to match:
> +
> + 333 common xyzzy stub_xyzzy
> +
> +The equivalent for 32-bit programs running on a 64-bit kernel is normally
> +called stub32_xyzzy and implemented in arch/x86/entry/entry_64_compat.S,
> +with the corresponding syscall table adjustment in
> +arch/x86/entry/syscalls/syscall_32.tbl:
> +
> + 380 i386 xyzzy sys_xyzzy stub32_xyzzy
> +
> +If the system call needs a compatibility layer (as in the previous section)
> +then the stub32_ version needs to call on to the compat_sys_ version of the
> +system call rather than the native 64-bit version. Also, if the x32 ABI
> +implementation is not common with the x86_64 version, then its syscall
> +table will also need to invoke a stub that calls on to the compat_sys_
> +version.
> +
> +For completeness, it's also nice to set up a mapping so that user-mode Linux
> +still works -- its syscall table will reference stub_xyzzy, but the UML build
> +doesn't include arch/x86/entry/entry_64.S implementation (because UML
> +simulates registers etc). Fixing this is as simple as adding a #define to
> +arch/x86/um/sys_call_table_64.c:
> +
> + #define stub_xyzzy sys_xyzzy
> +
> +
> +Other Details
> +-------------
> +
> +Most of the kernel treats system calls in a generic way, but there is the
> +occasional exception that may need updating for your particular system call.
> +
> +The audit subsystem is one such special case; it includes (arch-specific)
> +functions that classify some special types of system call -- specifically
> +file open (open/openat), program execution (execve/exeveat) or socket
> +multiplexor (socketcall) operations. If your new system call is analogous to
> +one of these, then the audit system should be updated.
> +
> +More generally, if there is an existing system call that is analogous to your
> +new system call, it's worth doing a kernel-wide grep for the existing system
> +call to check there are no other special cases.
> +
> +
> +Testing
> +-------
> +
> +A new system call should obviously be tested; it is also useful to provide
> +reviewers with a demonstration of how user space programs will use the system
> +call. A good way to combine these aims is to include a simple self-test
> +program in a new directory under tools/testing/selftests/.
> +
> +For a new system call, there will obviously be no libc wrapper function and so
> +the test will need to invoke it using syscall(); also, if the system call
> +involves a new userspace-visible structure, the corresponding header will need
> +to be installed to compile the test.
> +
> +Make sure the selftest runs successfully on all supported architectures. For
> +example, check that it works when compiled as an x86_64 (-m64), x86_32 (-m32)
> +and x32 (-mx32) ABI program.
> +
> +For more extensive and thorough testing of new functionality, you should also
> +consider adding tests to the Linux Test Project, or to the xfstests project
> +for filesystem-related changes.
> + - https://linux-test-project.github.io/
> + - git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
> +
> +
> +Man Page
> +--------
> +
> +All new system calls should come with a complete man page, ideally using groff
> +markup, but plain text will do. If groff is used, it's helpful to include a
> +pre-rendered ASCII version of the man page in the cover email for the
> +patchset, for the convenience of reviewers.
> +
> +The man page should be cc'ed to linux-man@vger.kernel.org
> +For more details, see https://www.kernel.org/doc/man-pages/patches.html
> +
> +References and Sources
> +----------------------
> +
> + - LWN article from Michael Kerrisk on use of flags argument in system calls:
> + https://lwn.net/Articles/585415/
> + - LWN article from Michael Kerrisk on how to handle unknown flags in a system
> + call: https://lwn.net/Articles/588444/
> + - LWN article from Jake Edge describing constraints on 64-bit system call
> + arguments: https://lwn.net/Articles/311630/
> + - Pair of LWN articles from David Drysdale that describe the system call
> + implementation paths in detail for v3.14:
> + - https://lwn.net/Articles/604287/
> + - https://lwn.net/Articles/604515/
> + - Architecture-specific requirements for system calls are discussed in the
> + syscall(2) man-page:
> + http://man7.org/linux/man-pages/man2/syscall.2.html#NOTES
> + - Collated emails from Linus Torvalds discussing the problems with ioctl():
> + http://yarchive.net/comp/linux/ioctl.html
> + - "How to not invent kernel interfaces", Arnd Bergmann,
> + http://www.ukuug.org/events/linux2007/2007/papers/Bergmann.pdf
> + - LWN article from Michael Kerrisk on avoiding new uses of CAP_SYS_ADMIN:
> + https://lwn.net/Articles/486306/
> + - Recommendation from Andrew Morton that all related information for a new
> + system call should come in the same email thread:
> + https://lkml.org/lkml/2014/7/24/641
> + - Recommendation from Michael Kerrisk that a new system call should come with
> + a man page: https://lkml.org/lkml/2014/6/13/309
> + - Suggestion from Thomas Gleixner that x86 wire-up should be in a separate
> + commit: https://lkml.org/lkml/2014/11/19/254
> + - Suggestion from Greg Kroah-Hartman that it's good for new system calls to
> + come with a man-page & selftest: https://lkml.org/lkml/2014/3/19/710
> + - Discussion from Michael Kerrisk of new system call vs. prctl(2) extension:
> + https://lkml.org/lkml/2014/6/3/411
> + - Suggestion from Ingo Molnar that system calls that involve multiple arguments
> + should encapsulate those arguments in a struct, which includes a size field
> + for future extensibility: https://lkml.org/lkml/2015/7/30/117
> + - Numbering oddities arising from (re-)use of O_* numbering space flags:
> + - commit 75069f2b5bfb ("vfs: renumber FMODE_NONOTIFY and add to uniqueness
> + check")
> + - commit 12ed2e36c98a ("fanotify: FMODE_NONOTIFY and __O_SYNC in sparc
> + conflict")
> + - commit bb458c644a59 ("Safer ABI for O_TMPFILE")
> + - Discussion from Matthew Wilcox about restrictions on 64-bit arguments:
> + https://lkml.org/lkml/2008/12/12/187
> + - Recommendation from Greg Kroah-Hartman that unknown flags should be
> + policed: https://lkml.org/lkml/2014/7/17/577
> + - Recommendation from Linus Torvalds that x32 system calls should prefer
> + compatibility with 64-bit versions rather than 32-bit versions:
> + https://lkml.org/lkml/2011/8/31/244
> --
> 2.5.0.rc2.392.g76e840b
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: David Drysdale @ 2015-07-31 14:42 UTC (permalink / raw)
To: Josh Triplett
Cc: Michael Kerrisk, Linux API, Andrew Morton, Arnd Bergmann,
Shuah Khan, Jonathan Corbet, Eric B Munson, Randy Dunlap,
Andrea Arcangeli, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Oleg Nesterov, Linus Torvalds, Greg Kroah-Hartman,
Andy Lutomirski, Al Viro, Rusty Russell, Peter Zijlstra,
Vivek Goyal, Alexei Starovoitov, David Herrmann,
Theodore Ts'o, Kee
In-Reply-To: <20150731130620.GA16435@x>
On Fri, Jul 31, 2015 at 2:06 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Fri, Jul 31, 2015 at 10:48:32AM +0100, David Drysdale wrote:
>> On Thu, Jul 30, 2015 at 7:50 PM, Josh Triplett <josh@joshtriplett.org> wrote:
>> > On Thu, Jul 30, 2015 at 08:52:11AM +0100, David Drysdale wrote:
>> >> +needs to be governed by the appropriate Linux capability bit (checked with a
>> >> +call to capable()), as described in the capabilities(7) man page.
>> >> +
>> >> + - If there is an existing capability that governs related functionality, then
>> >> + use that. However, avoid combining lots of only vaguely related functions
>> >> + together under the same bit, as this goes against capabilities' purpose of
>> >> + splitting the power of root. In particular, avoid adding new uses of the
>> >> + already overly-general CAP_SYS_ADMIN capability.
>> >> + - If there is no related capability, then consider adding a new capability
>> >> + bit -- but bear in mind that the numbering space is limited, and each new
>> >> + bit needs to be understood and administered by sysadmins.
>> >
>> > Many previous discussions on this topic seem to have concluded that it's
>> > almost impossible to add a new capability without breaking existing
>> > programs. I would suggest not even mentioning this possibility.
>>
>> I'm not particularly knowledgable about capabilities (at least, not the
>> POSIX.1e/Linux kind), so I'll confess that I got this suggestion from
>> Michael Kerrisk.
>>
>> Michael, do you agree that we should just drop the possibility of adding
>> new capability bits?
>>
>> Also, Josh, do you have any references to the earlier discussions on the
>> topic so I can update the Sources section?
>
> No direct links handy at the moment without some searching, but one
> iteration of it came up when Matthew Garrett suggested adding
> CAP_COMPROMISE_KERNEL, and the ensuing discussion of capability
> semantics suggested that the way the capability space was defined and
> controlled by userspace meant that adding any new capability would
> effectively break userspace ABI. The userspace ABI for capabilities is
> not clear; some applications drop all possible capabilities and could
> get surprised by a new capability being dropped, while other
> applications drop only capabilities they know about and could get
> surprised by a new capability *not* being dropped.
BTW, I left this paragraph unchanged in the v3 version I just sent
out -- I'll update it for v4 when I get back from vacation, depending
on what discussion occurs in the meantime...
^ permalink raw reply
* [PATCHv3 1/1] Documentation: describe how to add a system call
From: David Drysdale @ 2015-07-31 14:35 UTC (permalink / raw)
To: linux-api, Michael Kerrisk, Andrew Morton, Arnd Bergmann,
Shuah Khan, Jonathan Corbet, Eric B Munson, Randy Dunlap,
Cyril Hrubis, Josh Triplett
Cc: Andrea Arcangeli, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Oleg Nesterov, Linus Torvalds, Greg Kroah-Hartman,
Andy Lutomirski, Al Viro, Rusty Russell, Peter Zijlstra,
Vivek Goyal, Alexei Starovoitov, David Herrmann,
Theodore Ts'o, Kees Cook, Milosz Tanski, Fam Zheng,
Mathieu Desnoyers, linux-doc, linux-kernel, David Drysdale
In-Reply-To: <1438353346-67177-1-git-send-email-drysdale@google.com>
Add a document describing the process of adding a new system call,
including the need for a flags argument for future compatibility, and
covering 32-bit/64-bit concerns (albeit in an x86-centric way).
Signed-off-by: David Drysdale <drysdale@google.com>
Reviewed-by: Michael Kerrisk <mtk.manpages@gmail.com>
Reviewed-by: Eric B Munson <emunson@akamai.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
---
Documentation/adding-syscalls.txt | 531 ++++++++++++++++++++++++++++++++++++++
1 file changed, 531 insertions(+)
create mode 100644 Documentation/adding-syscalls.txt
diff --git a/Documentation/adding-syscalls.txt b/Documentation/adding-syscalls.txt
new file mode 100644
index 000000000000..633691c5fb45
--- /dev/null
+++ b/Documentation/adding-syscalls.txt
@@ -0,0 +1,531 @@
+Adding a New System Call
+========================
+
+This document describes what's involved in adding a new system call to the
+Linux kernel, over and above the normal submission advice in
+Documentation/SubmittingPatches.
+
+
+System Call Alternatives
+------------------------
+
+The first thing to consider when adding a new system call is whether one of
+the alternatives might be suitable instead. Although system calls are the
+most traditional and most obvious interaction points between userspace and the
+kernel, there are other possibilities -- choose what fits best for your
+interface.
+
+ - If the operations involved can be made to look like a filesystem-like
+ object, it may make more sense to create a new filesystem or device. This
+ also makes it easier to encapsulate the new functionality in a kernel module
+ rather than requiring it to be built into the main kernel.
+ - If the new functionality involves operations where the kernel notifies
+ userspace that something has happened, then returning a new file
+ descriptor for the relevant object allows userspace to use
+ poll/select/epoll to receive that notification.
+ - However, operations that don't map to read(2)/write(2)-like operations
+ have to be implemented as ioctl(2) requests, which can lead to a
+ somewhat opaque API.
+ - If you're just exposing runtime system information, a new node in sysfs
+ (see Documentation/filesystems/sysfs.txt) or the /proc filesystem may be
+ more appropriate. However, access to these mechanisms requires that the
+ relevant filesystem is mounted, which might not always be the case (e.g.
+ in a namespaced/sandboxed/chrooted environment). Avoid adding any API to
+ debugfs, as this is not considered a 'production' interface to userspace.
+ - If the operation is specific to a particular file or file descriptor, then
+ an additional fcntl(2) command option may be more appropriate. However,
+ fcntl(2) is a multiplexing system call that hides a lot of complexity, so
+ this option is best for when the new function is closely analogous to
+ existing fcntl(2) functionality, or the new functionality is very simple
+ (for example, getting/setting a simple flag related to a file descriptor).
+ - If the operation is specific to a particular task or process, then an
+ additional prctl(2) command option may be more appropriate. As with
+ fcntl(2), this system call is a complicated multiplexor so is best reserved
+ for near-analogs of existing prctl() commands or getting/setting a simple
+ flag related to a process.
+
+
+Designing the API: Planning for Extension
+-----------------------------------------
+
+A new system call forms part of the API of the kernel, and has to be supported
+indefinitely. As such, it's a very good idea to explicitly discuss the
+interface on the kernel mailing list, and it's important to plan for future
+extensions of the interface.
+
+(The syscall table is littered with historical examples where this wasn't done,
+together with the corresponding follow-up system calls -- eventfd/eventfd2,
+dup2/dup3, inotify_init/inotify_init1, pipe/pipe2, renameat/renameat2 -- so
+learn from the history of the kernel and plan for extensions from the start.)
+
+For simpler system calls that only take a couple of arguments, the preferred way
+to allow for future extensibility is to include a flags argument to the system
+call. To make sure that userspace programs can safely use flags between kernel
+versions, check whether the flags value holds any unknown flags, and reject the
+sycall (with EINVAL) if it does:
+
+ if (flags & ~(THING_FLAG1 | THING_FLAG2 | THING_FLAG3))
+ return -EINVAL;
+
+(If no flags values are used yet, check that the flags argument is zero.)
+
+For more sophisticated system calls that involve a larger number of arguments,
+it's preferred to encapsulate the majority of the arguments into a structure
+that is passed in by pointer. Such a structure can cope with future extension
+by including a size argument in the structure:
+
+ struct xyzzy_params {
+ u32 size; /* userspace sets p->size = sizeof(struct xyzzy_params) */
+ u32 param_1;
+ u64 param_2;
+ u64 param_3;
+ };
+
+As long as any subsequently added field, say param_4, is designed so that a zero
+value gives the previous behaviour, then this allows both directions of version
+mismatch:
+
+ - To cope with a later userspace program calling an older kernel, the kernel
+ code should check that any memory beyond the size of the structure that it
+ expects is zero (effectively checking that param_4 == 0).
+ - To cope with an older userspace program calling a newer kernel, the kernel
+ code can zero-extend a smaller instance of the structure (effectively setting
+ param_4 = 0).
+
+See perf_event_open(2) and the perf_copy_attr() function (in
+kernel/events/core.c) for an example of this approach.
+
+
+Designing the API: Other Considerations
+---------------------------------------
+
+If your new system call allows userspace to refer to a kernel object, it
+should use a file descriptor as the handle for that object -- don't invent a
+new type of userspace object handle when the kernel already has mechanisms and
+well-defined semantics for using file descriptors.
+
+If your new xyzzy(2) system call does return a new file descriptor, then the
+flags argument should include a value that is equivalent to setting O_CLOEXEC
+on the new FD. This makes it possible for userspace to close the timing
+window between xyzzy() and calling fcntl(fd, F_SETFD, FD_CLOEXEC), where an
+unexpected fork() and execve() in another thread could leak a descriptor to
+the exec'ed program. (However, resist the temptation to re-use the actual value
+of the O_CLOEXEC constant, as it is architecture-specific and is part of a
+numbering space of O_* flags that is fairly full.)
+
+If your system call returns a new file descriptor, you should also consider
+what it means to use the poll(2) family of system calls on that file
+descriptor. Making a file descriptor ready for reading or writing is the
+normal way for the kernel to indicate to userspace that an event has
+occurred on the corresponding kernel object.
+
+If your new xyzzy(2) system call involves a filename argument:
+
+ int sys_xyzzy(const char __user *path, ..., unsigned int flags);
+
+you should also consider whether an xyzzyat(2) version is more appropriate:
+
+ int sys_xyzzyat(int dfd, const char __user *path, ..., unsigned int flags);
+
+This allows more flexibility for how userspace specifies the file in question;
+in particular it allows userspace to request the functionality for an
+already-opened file descriptor using the AT_EMPTY_PATH flag, effectively giving
+an fxyzzy(3) operation for free:
+
+ - xyzzyat(AT_FDCWD, path, ..., 0) is equivalent to xyzzy(path,...)
+ - xyzzyat(fd, "", ..., AT_EMPTY_PATH) is equivalent to fxyzzy(fd, ...)
+
+(For more details on the rationale of the *at() calls, see the openat(2) man
+page; for an example of AT_EMPTY_PATH, see the statat(2) man page.)
+
+If your new xyzzy(2) system call involves a parameter describing an offset
+within a file, make its type loff_t so that 64-bit offsets can be supported
+even on 32-bit architectures.
+
+If your new xyzzy(2) system call involves privileged functionality, it
+needs to be governed by the appropriate Linux capability bit (checked with a
+call to capable()), as described in the capabilities(7) man page.
+
+ - If there is an existing capability that governs related functionality, then
+ use that. However, avoid combining lots of only vaguely related functions
+ together under the same bit, as this goes against capabilities' purpose of
+ splitting the power of root. In particular, avoid adding new uses of the
+ already overly-general CAP_SYS_ADMIN capability.
+ - If there is no related capability, then consider adding a new capability
+ bit -- but bear in mind that the numbering space is limited, and each new
+ bit needs to be understood and administered by sysadmins.
+
+If your new xyzzy(2) system call manipulates a process other than the calling
+process, it should be restricted (using a call to ptrace_may_access()) so that
+only a calling process with the same permissions as the target process, or
+with the necessary capabilities, can manipulate the target process.
+
+Finally, be aware that some non-x86 architectures have an easier time if
+system call parameters that are explicitly 64-bit fall on odd-numbered
+arguments (i.e. parameter 1, 3, 5), to allow use of contiguous pairs of 32-bit
+registers. (This concern does not apply if the arguments are part of a
+structure that's passed in by pointer.)
+
+
+Proposing the API
+-----------------
+
+To make new system calls easy to review, it's best to divide up the patchset
+into separate chunks. These should include at least the following items as
+distinct commits (each of which is described further below):
+
+ - The core implementation of the system call, together with prototypes,
+ generic numbering, Kconfig changes and fallback stub implementation.
+ - Wiring up of the new system call for one particular architecture, usually
+ x86 (including all of x86_64, x86_32 and x32).
+ - A demonstration of the use of the new system call in userspace via a
+ selftest in tools/testing/selftests/.
+ - A draft man-page for the new system call, either as plain text in the
+ cover letter, or as a patch to the (separate) man-pages repository.
+
+New system call proposals, like any change to the kernel's API, should always
+be cc'ed to linux-api@vger.kernel.org
+
+
+Generic System Call Implementation
+----------------------------------
+
+The main entry point for your new xyzzy(2) system call will be called
+sys_xyzzy(), but you add this entry point with the appropriate
+SYSCALL_DEFINEn() macro rather than explicitly. The 'n' indicates the number
+of arguments to the system call, and the macro takes the system call name
+followed by the (type, name) pairs for the parameters as arguments. Using
+this macro allows metadata about the new system call to be made available for
+other tools.
+
+The new entry point also needs a corresponding function prototype, in
+include/linux/syscalls.h, marked as asmlinkage to match the way that system
+calls are invoked:
+
+ asmlinkage long sys_xyzzy(...);
+
+Some architectures (e.g. x86) have their own architecture-specific syscall
+tables, but several other architectures share a generic syscall table. Add your
+new system call to the generic list by adding an entry to the list in
+include/uapi/asm-generic/unistd.h:
+
+ #define __NR_xyzzy 292
+ __SYSCALL(__NR_xyzzy, sys_xyzzy)
+
+Also update the __NR_syscalls count to reflect the additional system call, and
+note that if multiple new system calls are added in the same merge window,
+your new syscall number may get adjusted to resolve conflicts.
+
+The file kernel/sys_ni.c provides a fallback stub implementation of each system
+call, returning -ENOSYS. Add your new system call here too:
+
+ cond_syscall(sys_xyzzy);
+
+Your new kernel functionality, and the system call that controls it, should
+normally be optional, so add a CONFIG option (typically to init/Kconfig) for
+it. As usual for new CONFIG options:
+
+ - Include a description of the new functionality and system call controlled
+ by the option.
+ - Make the option depend on EXPERT if it should be hidden from normal users.
+ - Make any new source files implementing the function dependent on the CONFIG
+ option in the Makefile (e.g. "obj-$(CONFIG_XYZZY_SYSCALL) += xyzzy.c").
+ - Double check that the kernel still builds with the new CONFIG option turned
+ off.
+
+To summarize, you need a commit that includes:
+
+ - CONFIG option for the new function, normally in init/Kconfig
+ - SYSCALL_DEFINEn(xyzzy, ...) for the entry point
+ - corresponding prototype in include/linux/syscalls.h
+ - generic table entry in include/uapi/asm-generic/unistd.h
+ - fallback stub in kernel/sys_ni.c
+
+
+x86 System Call Implementation
+------------------------------
+
+To wire up your new system call for x86 platforms, you need to update the
+master syscall tables. Assuming your new system call isn't special in some
+way (see below), this involves a "common" entry (for x86_64 and x32) in
+arch/x86/entry/syscalls/syscall_64.tbl:
+
+ 333 common xyzzy sys_xyzzy
+
+and an "i386" entry in arch/x86/entry/syscalls/syscall_32.tbl:
+
+ 380 i386 xyzzy sys_xyzzy
+
+Again, these numbers are liable to be changed if there are conflicts in the
+relevant merge window.
+
+
+Compatibility System Calls (Generic)
+------------------------------------
+
+For most system calls the same 64-bit implementation can be invoked even when
+the userspace program is itself 32-bit; even if the system call's parameters
+include an explicit pointer, this is handled transparently.
+
+However, there are a couple of situations where a compatibility layer is
+needed to cope with size differences between 32-bit and 64-bit.
+
+The first is if the 64-bit kernel also supports 32-bit userspace programs, and
+so needs to parse areas of (__user) memory that could hold either 32-bit or
+64-bit values. In particular, this is needed whenever a system call argument
+is:
+
+ - a pointer to a pointer
+ - a pointer to a struct containing a pointer (e.g. struct iovec __user *)
+ - a pointer to a varying sized integral type (time_t, off_t, long, ...)
+ - a pointer to a struct containing a varying sized integral type.
+
+The second situation that requires a compatibility layer is if one of the
+system call's arguments has a type that is explicitly 64-bit even on a 32-bit
+architecture, for example loff_t or __u64. In this case, a value that arrives
+at a 64-bit kernel from a 32-bit application will be split into two 32-bit
+values, which then need to be re-assembled in the compatibility layer.
+
+(Note that a system call argument that's a pointer to an explicit 64-bit type
+does *not* need a compatibility layer; for example, splice(2)'s arguments of
+type loff_t __user * do not trigger the need for a compat_ system call.)
+
+The compatibility version of the system call is called compat_sys_xyzzy(), and
+is added with the COMPAT_SYSCALL_DEFINEn() macro, analogously to
+SYSCALL_DEFINEn. This version of the implementation runs as part of a 64-bit
+kernel, but expects to receive 32-bit parameter values and does whatever is
+needed to deal with them. (Typically, the compat_sys_ version converts the
+values to 64-bit versions and either calls on to the sys_ version, or both of
+them call a common inner implementation function.)
+
+The compat entry point also needs a corresponding function prototype, in
+include/linux/compat.h, marked as asmlinkage to match the way that system
+calls are invoked:
+
+ asmlinkage long compat_sys_xyzzy(...);
+
+If the system call involves a structure that is laid out differently on 32-bit
+and 64-bit systems, say struct xyzzy_args, then the include/linux/compat.h
+header file should also include a compat version of the structure (struct
+compat_xyzzy_args) where each variable-size field has the appropriate compat_
+type that corresponds to the type in struct xyzzy_args. The
+compat_sys_xyzzy() routine can then use this compat_ structure to parse the
+arguments from a 32-bit invocation.
+
+For example, if there are fields:
+
+ struct xyzzy_args {
+ const char __user *ptr;
+ __kernel_long_t varying_val;
+ u64 fixed_val;
+ /* ... */
+ };
+
+in struct xyzzy_args, then struct compat_xyzzy_args would have:
+
+ struct compat_xyzzy_args {
+ compat_uptr_t ptr;
+ compat_long_t varying_val;
+ u64 fixed_val;
+ /* ... */
+ };
+
+The generic system call list also needs adjusting to allow for the compat
+version; the entry in include/uapi/asm-generic/unistd.h should use
+__SC_COMP rather than __SYSCALL:
+
+ #define __NR_xyzzy 292
+ __SC_COMP(__NR_xyzzy, sys_xyzzy, compat_sys_xyzzy)
+
+To summarize, you need:
+
+ - a COMPAT_SYSCALL_DEFINEn(xyzzy, ...) for the compat entry point
+ - corresponding prototype in include/linux/compat.h
+ - (if needed) 32-bit mapping struct in include/linux/compat.h
+ - instance of __SC_COMP not __SYSCALL in include/uapi/asm-generic/unistd.h
+
+
+Compatibility System Calls (x86)
+--------------------------------
+
+To wire up the x86 architecture of a system call with a compatibility version,
+the entries in the syscall tables need to be adjusted.
+
+First, the entry in arch/x86/entry/syscalls/syscall_32.tbl gets an extra
+column to indicate that a 32-bit userspace program running on a 64-bit kernel
+should hit the compat entry point:
+
+ 380 i386 xyzzy sys_xyzzy compat_sys_xyzzy
+
+Second, you need to figure out what should happen for the x32 ABI version of
+the new system call. There's a choice here: the layout of the arguments
+should either match the 64-bit version or the 32-bit version.
+
+If there's a pointer-to-a-pointer involved, the decision is easy: x32 is
+ILP32, so the layout should match the 32-bit version, and the entry in
+arch/x86/entry/syscalls/syscall_64.tbl is split so that x32 programs hit the
+compatibility wrapper:
+
+ 333 64 xyzzy sys_xyzzy
+ ...
+ 555 x32 xyzzy compat_sys_xyzzy
+
+If no pointers are involved, then it is preferable to re-use the 64-bit system
+call for the x32 ABI (and consequently the entry in
+arch/x86/entry/syscalls/syscall_64.tbl is unchanged).
+
+In either case, you should check that the types involved in your argument
+layout do indeed map exactly from x32 (-mx32) to either the 32-bit (-m32) or
+64-bit (-m64) equivalents.
+
+
+System Calls Returning Elsewhere
+--------------------------------
+
+For most system calls, once the system call is complete the user program
+continues exactly where it left off -- at the next instruction, with the same
+stack and registers as before the system call, and with the same virtual
+memory space.
+
+However, a few system calls do things differently. They might return to a
+different location (rt_sigreturn) or change the memory space (fork/vfork/clone)
+or even architecture (execve/execveat) of the program.
+
+To allow for this, the kernel implementation of the system call may need to
+save and restore additional registers to the kernel stack, allowing complete
+control of where and how execution continues after the system call.
+
+This is arch-specific, but typically involves defining assembly entry points
+that save/restore additional registers and invoke the real system call entry
+point.
+
+For x86_64, this is implemented as a stub_xyzzy entry point in
+arch/x86/entry/entry_64.S, and the entry in the syscall table
+(arch/x86/entry/syscalls/syscall_64.tbl) is adjusted to match:
+
+ 333 common xyzzy stub_xyzzy
+
+The equivalent for 32-bit programs running on a 64-bit kernel is normally
+called stub32_xyzzy and implemented in arch/x86/entry/entry_64_compat.S,
+with the corresponding syscall table adjustment in
+arch/x86/entry/syscalls/syscall_32.tbl:
+
+ 380 i386 xyzzy sys_xyzzy stub32_xyzzy
+
+If the system call needs a compatibility layer (as in the previous section)
+then the stub32_ version needs to call on to the compat_sys_ version of the
+system call rather than the native 64-bit version. Also, if the x32 ABI
+implementation is not common with the x86_64 version, then its syscall
+table will also need to invoke a stub that calls on to the compat_sys_
+version.
+
+For completeness, it's also nice to set up a mapping so that user-mode Linux
+still works -- its syscall table will reference stub_xyzzy, but the UML build
+doesn't include arch/x86/entry/entry_64.S implementation (because UML
+simulates registers etc). Fixing this is as simple as adding a #define to
+arch/x86/um/sys_call_table_64.c:
+
+ #define stub_xyzzy sys_xyzzy
+
+
+Other Details
+-------------
+
+Most of the kernel treats system calls in a generic way, but there is the
+occasional exception that may need updating for your particular system call.
+
+The audit subsystem is one such special case; it includes (arch-specific)
+functions that classify some special types of system call -- specifically
+file open (open/openat), program execution (execve/exeveat) or socket
+multiplexor (socketcall) operations. If your new system call is analogous to
+one of these, then the audit system should be updated.
+
+More generally, if there is an existing system call that is analogous to your
+new system call, it's worth doing a kernel-wide grep for the existing system
+call to check there are no other special cases.
+
+
+Testing
+-------
+
+A new system call should obviously be tested; it is also useful to provide
+reviewers with a demonstration of how user space programs will use the system
+call. A good way to combine these aims is to include a simple self-test
+program in a new directory under tools/testing/selftests/.
+
+For a new system call, there will obviously be no libc wrapper function and so
+the test will need to invoke it using syscall(); also, if the system call
+involves a new userspace-visible structure, the corresponding header will need
+to be installed to compile the test.
+
+Make sure the selftest runs successfully on all supported architectures. For
+example, check that it works when compiled as an x86_64 (-m64), x86_32 (-m32)
+and x32 (-mx32) ABI program.
+
+For more extensive and thorough testing of new functionality, you should also
+consider adding tests to the Linux Test Project, or to the xfstests project
+for filesystem-related changes.
+ - https://linux-test-project.github.io/
+ - git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
+
+
+Man Page
+--------
+
+All new system calls should come with a complete man page, ideally using groff
+markup, but plain text will do. If groff is used, it's helpful to include a
+pre-rendered ASCII version of the man page in the cover email for the
+patchset, for the convenience of reviewers.
+
+The man page should be cc'ed to linux-man@vger.kernel.org
+For more details, see https://www.kernel.org/doc/man-pages/patches.html
+
+References and Sources
+----------------------
+
+ - LWN article from Michael Kerrisk on use of flags argument in system calls:
+ https://lwn.net/Articles/585415/
+ - LWN article from Michael Kerrisk on how to handle unknown flags in a system
+ call: https://lwn.net/Articles/588444/
+ - LWN article from Jake Edge describing constraints on 64-bit system call
+ arguments: https://lwn.net/Articles/311630/
+ - Pair of LWN articles from David Drysdale that describe the system call
+ implementation paths in detail for v3.14:
+ - https://lwn.net/Articles/604287/
+ - https://lwn.net/Articles/604515/
+ - Architecture-specific requirements for system calls are discussed in the
+ syscall(2) man-page:
+ http://man7.org/linux/man-pages/man2/syscall.2.html#NOTES
+ - Collated emails from Linus Torvalds discussing the problems with ioctl():
+ http://yarchive.net/comp/linux/ioctl.html
+ - "How to not invent kernel interfaces", Arnd Bergmann,
+ http://www.ukuug.org/events/linux2007/2007/papers/Bergmann.pdf
+ - LWN article from Michael Kerrisk on avoiding new uses of CAP_SYS_ADMIN:
+ https://lwn.net/Articles/486306/
+ - Recommendation from Andrew Morton that all related information for a new
+ system call should come in the same email thread:
+ https://lkml.org/lkml/2014/7/24/641
+ - Recommendation from Michael Kerrisk that a new system call should come with
+ a man page: https://lkml.org/lkml/2014/6/13/309
+ - Suggestion from Thomas Gleixner that x86 wire-up should be in a separate
+ commit: https://lkml.org/lkml/2014/11/19/254
+ - Suggestion from Greg Kroah-Hartman that it's good for new system calls to
+ come with a man-page & selftest: https://lkml.org/lkml/2014/3/19/710
+ - Discussion from Michael Kerrisk of new system call vs. prctl(2) extension:
+ https://lkml.org/lkml/2014/6/3/411
+ - Suggestion from Ingo Molnar that system calls that involve multiple arguments
+ should encapsulate those arguments in a struct, which includes a size field
+ for future extensibility: https://lkml.org/lkml/2015/7/30/117
+ - Numbering oddities arising from (re-)use of O_* numbering space flags:
+ - commit 75069f2b5bfb ("vfs: renumber FMODE_NONOTIFY and add to uniqueness
+ check")
+ - commit 12ed2e36c98a ("fanotify: FMODE_NONOTIFY and __O_SYNC in sparc
+ conflict")
+ - commit bb458c644a59 ("Safer ABI for O_TMPFILE")
+ - Discussion from Matthew Wilcox about restrictions on 64-bit arguments:
+ https://lkml.org/lkml/2008/12/12/187
+ - Recommendation from Greg Kroah-Hartman that unknown flags should be
+ policed: https://lkml.org/lkml/2014/7/17/577
+ - Recommendation from Linus Torvalds that x32 system calls should prefer
+ compatibility with 64-bit versions rather than 32-bit versions:
+ https://lkml.org/lkml/2011/8/31/244
--
2.5.0.rc2.392.g76e840b
^ permalink raw reply related
* [PATCHv3 0/1] Document how to add a new syscall
From: David Drysdale @ 2015-07-31 14:35 UTC (permalink / raw)
To: linux-api, Michael Kerrisk, Andrew Morton, Arnd Bergmann,
Shuah Khan, Jonathan Corbet, Eric B Munson, Randy Dunlap,
Cyril Hrubis, Josh Triplett
Cc: Andrea Arcangeli, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Oleg Nesterov, Linus Torvalds, Greg Kroah-Hartman,
Andy Lutomirski, Al Viro, Rusty Russell, Peter Zijlstra,
Vivek Goyal, Alexei Starovoitov, David Herrmann,
Theodore Ts'o, Kees Cook, Milosz Tanski, Fam Zheng,
Mathieu Desnoyers, linux-doc, linux-kernel, David Drysdale
[Admin note: I'm on vacation next week, so I'll collate any additional
feedback when I get back.]
Given that I've gotten some of the details wrong in the past (and I've
seen others do likewise), I thought it might be helpful to collate the
best practices for adding a new system call to the kernel.
Apologies for the wide circulation -- I've tried to include folk who've
recently added or proposed a system call, as they're most likely to
have opinions on:
- whether this a useful addition to Documentation/
- whether the details of the advice are correct and complete.
Thanks,
David
(With thanks to Andrew Morton for looking over an initial draft, and to
Michael Kerrisk for suggesting several clarifications and additions.)
Changes since v2:
- add struct-with-size as option for extensibility [Ingo Molnar]
- mention LTP and xfstests [Cyril Hrubis]
- always use file descriptors as handles for kernel objects [Josh Triplett]
- consider poll(2) semantics for new file descriptors [Josh Triplett]
- use a CONFIG option to control new functionality [Josh Triplett]
- check ptrace_may_access() for process manipulation [Josh Triplett]
- various minor markups [Josh Triplett]
Changes since v1:
- added paragraph on build requirements to Testing section [Shuah Khan,
Peter Zijlstra]
- various text clarifications [Kees Cook]
- added Reviewed-by markers
David Drysdale (1):
Documentation: describe how to add a system call
Documentation/adding-syscalls.txt | 531 ++++++++++++++++++++++++++++++++++++++
1 file changed, 531 insertions(+)
create mode 100644 Documentation/adding-syscalls.txt
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply
* Re: [PATCH] kdbus/samples: skip on CROSS_COMPILE
From: Sudip Mukherjee @ 2015-07-31 14:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: David Herrmann, Shuah Khan, linux-api, linux-kernel,
Paul Gortmaker, Daniel Mack, Djalal Harouni
In-Reply-To: <20150730163307.GA20048@kroah.com>
On Thu, Jul 30, 2015 at 09:33:07AM -0700, Greg Kroah-Hartman wrote:
> On Thu, Jul 30, 2015 at 06:13:25PM +0200, David Herrmann wrote:
> > Apparently we cannot rely on up-to-date kernel headers to be available
> > when cross-compiling, not even for HOSTCC. That's sad, but it's how it
> > is. Skip samples on cross-compiles as suggested by Paul, so allmodconfig
> > runs smoothly again.
> >
> > Tested-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> > ---
> > samples/kdbus/Makefile | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/samples/kdbus/Makefile b/samples/kdbus/Makefile
> > index 137f842..dbd9de8 100644
> > --- a/samples/kdbus/Makefile
> > +++ b/samples/kdbus/Makefile
> > @@ -1,9 +1,13 @@
> > # kbuild trick to avoid linker error. Can be omitted if a module is built.
> > obj- := dummy.o
> >
> > +ifndef CROSS_COMPILE
>
> This really feels like the wrong solution.
I also faced the same problem while testing allmodconfig for mn10300.
regards
sudip
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Josh Triplett @ 2015-07-31 13:06 UTC (permalink / raw)
To: David Drysdale
Cc: Michael Kerrisk, Linux API, Andrew Morton, Arnd Bergmann,
Shuah Khan, Jonathan Corbet, Eric B Munson, Randy Dunlap,
Andrea Arcangeli, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Oleg Nesterov, Linus Torvalds, Greg Kroah-Hartman,
Andy Lutomirski, Al Viro, Rusty Russell, Peter Zijlstra,
Vivek Goyal, Alexei Starovoitov, David Herrmann,
Theodore Ts'o, Kees
In-Reply-To: <CAHse=S-jtynT-1Pk8+JMd98wViao-bJh3K8TtGKPk7RXWgy=rg@mail.gmail.com>
On Fri, Jul 31, 2015 at 10:48:32AM +0100, David Drysdale wrote:
> On Thu, Jul 30, 2015 at 7:50 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Thu, Jul 30, 2015 at 08:52:11AM +0100, David Drysdale wrote:
> >> Add a document describing the process of adding a new system call,
> >> including the need for a flags argument for future compatibility, and
> >> covering 32-bit/64-bit concerns (albeit in an x86-centric way).
> >>
> >> Signed-off-by: David Drysdale <drysdale@google.com>
> >> Reviewed-by: Michael Kerrisk <mtk.manpages@gmail.com>
> >> Reviewed-by: Eric B Munson <emunson@akamai.com>
> >> Reviewed-by: Kees Cook <keescook@chromium.org>
> >> Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
> >
> > A few comments below. I also agree with Ingo's comment about
> > documenting the param-structure approach in addition to flags.
>
> Many thanks for looking through the doc in detail!
>
> I'll send out an updated doc today; I'm then on vacation for the next
> week so I'll collate further feedback after I return.
Thanks for the quick response and for incorporating these changes.
> > Some things to add a this point, before talking about adding a new file
> > descriptor:
> >
> > If you want to provide userspace with a handle to a kernel object, do so
> > in the form of a file descriptor. Do not invent a new type of userspace
> > object handle with its own namespace. The kernel already has numerous
> > system calls and well-defined semantics for managing and passing around
> > file descriptors, as well as for cleaning up when the file descriptor is
> > no longer in use.
> >
> > If your new system call returns a file descriptor, think about what it
> > means to use the poll(2) family of syscalls on that file descriptor. If
> > you want to send an event to userspace associated with your kernel
> > object, you should do so by making your file descriptor ready for
> > reading or writing.
>
> Good points, I've added a couple of paragraphs along these lines:
>
> "
> If your new system call allows userspace to refer to a kernel object, it
> should use a file descriptor as the handle for that object -- don't invent a
> new type of userspace object handle when the kernel already has mechanisms and
> well-defined semantics for using file descriptors.
>
> [snip: existing text on CLOEXEC]
>
> If your system call returns a new file descriptor, you should also consider
> what it means to use the poll(2) family of system calls on that file
> descriptor. Making a file descriptor ready for reading or writing is the
> normal way for the kernel to indicate to userspace that an event has
> occurred on the corresponding kernel object.
> "
These look good to me.
> >> +needs to be governed by the appropriate Linux capability bit (checked with a
> >> +call to capable()), as described in the capabilities(7) man page.
> >> +
> >> + - If there is an existing capability that governs related functionality, then
> >> + use that. However, avoid combining lots of only vaguely related functions
> >> + together under the same bit, as this goes against capabilities' purpose of
> >> + splitting the power of root. In particular, avoid adding new uses of the
> >> + already overly-general CAP_SYS_ADMIN capability.
> >> + - If there is no related capability, then consider adding a new capability
> >> + bit -- but bear in mind that the numbering space is limited, and each new
> >> + bit needs to be understood and administered by sysadmins.
> >
> > Many previous discussions on this topic seem to have concluded that it's
> > almost impossible to add a new capability without breaking existing
> > programs. I would suggest not even mentioning this possibility.
>
> I'm not particularly knowledgable about capabilities (at least, not the
> POSIX.1e/Linux kind), so I'll confess that I got this suggestion from
> Michael Kerrisk.
>
> Michael, do you agree that we should just drop the possibility of adding
> new capability bits?
>
> Also, Josh, do you have any references to the earlier discussions on the
> topic so I can update the Sources section?
No direct links handy at the moment without some searching, but one
iteration of it came up when Matthew Garrett suggested adding
CAP_COMPROMISE_KERNEL, and the ensuing discussion of capability
semantics suggested that the way the capability space was defined and
controlled by userspace meant that adding any new capability would
effectively break userspace ABI. The userspace ABI for capabilities is
not clear; some applications drop all possible capabilities and could
get surprised by a new capability being dropped, while other
applications drop only capabilities they know about and could get
surprised by a new capability *not* being dropped.
- Josh Triplett
^ permalink raw reply
* Re: [PATCH 2/4] mm/compaction: enable mobile-page migration
From: Minchan Kim @ 2015-07-31 10:43 UTC (permalink / raw)
To: Gioh Kim
Cc: jlayton, bfields, vbabka, iamjoonsoo.kim, viro, mst, koct9i,
aquini, linux-fsdevel, virtualization, linux-kernel, linux-api,
linux-mm, dri-devel, akpm, Gioh Kim
In-Reply-To: <1436776519-17337-3-git-send-email-gioh.kim@lge.com>
Hi Gioh,
On Mon, Jul 13, 2015 at 05:35:17PM +0900, Gioh Kim wrote:
> From: Gioh Kim <gurugio@hanmail.net>
>
> Add framework to register callback functions and check page mobility.
> There are some modes for page isolation so that isolate interface
> has arguments of page address and isolation mode while putback
> interface has only page address as argument.
>
> Signed-off-by: Gioh Kim <gioh.kim@lge.com>
> Acked-by: Rafael Aquini <aquini@redhat.com>
> ---
> fs/proc/page.c | 3 ++
> include/linux/compaction.h | 80 ++++++++++++++++++++++++++++++++++
> include/linux/fs.h | 2 +
> include/linux/page-flags.h | 19 ++++++++
> include/uapi/linux/kernel-page-flags.h | 1 +
> 5 files changed, 105 insertions(+)
>
> diff --git a/fs/proc/page.c b/fs/proc/page.c
> index 7eee2d8..a4f5a00 100644
> --- a/fs/proc/page.c
> +++ b/fs/proc/page.c
> @@ -146,6 +146,9 @@ u64 stable_page_flags(struct page *page)
> if (PageBalloon(page))
> u |= 1 << KPF_BALLOON;
>
> + if (PageMobile(page))
> + u |= 1 << KPF_MOBILE;
> +
Need a new page flag for non-LRU page migration?
I am worry that we don't have empty slot for page flag on 32-bit.
Aha, see SetPageMobile below. You use _mapcount.
It seems to be work for non-LRU pages but unfortunately, zsmalloc
already have used that field as own purpose so there is no room
for it.
> u |= kpf_copy_bit(k, KPF_LOCKED, PG_locked);
>
> u |= kpf_copy_bit(k, KPF_SLAB, PG_slab);
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index aa8f61c..f693072 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -1,6 +1,9 @@
> #ifndef _LINUX_COMPACTION_H
> #define _LINUX_COMPACTION_H
>
> +#include <linux/page-flags.h>
> +#include <linux/pagemap.h>
> +
> /* Return values for compact_zone() and try_to_compact_pages() */
> /* compaction didn't start as it was deferred due to past failures */
> #define COMPACT_DEFERRED 0
> @@ -51,6 +54,70 @@ extern void compaction_defer_reset(struct zone *zone, int order,
> bool alloc_success);
> extern bool compaction_restarting(struct zone *zone, int order);
>
> +static inline bool mobile_page(struct page *page)
> +{
> + return page->mapping && (PageMobile(page) || PageBalloon(page));
What's the locking rule to test mobile_page?
Why we need such many checking?
Why we need PageBalloon check?
You are trying to make generic abstraction for non-LRU page to migrate
but PageBalloon check in here breaks your goal.
> +}
> +
> +static inline bool isolate_mobilepage(struct page *page, isolate_mode_t mode)
> +{
> + bool ret = false;
> +
> + /*
> + * Avoid burning cycles with pages that are yet under __free_pages(),
> + * or just got freed under us.
> + *
> + * In case we 'win' a race for a mobile page being freed under us and
> + * raise its refcount preventing __free_pages() from doing its job
> + * the put_page() at the end of this block will take care of
> + * release this page, thus avoiding a nasty leakage.
> + */
Good.
> + if (unlikely(!get_page_unless_zero(page)))
> + goto out;
> +
> + /*
> + * As mobile pages are not isolated from LRU lists, concurrent
> + * compaction threads can race against page migration functions
> + * as well as race against the releasing a page.
How can it race with releasing a page?
We already get refcount above.
Do you mean page->mapping tearing race?
If so, zsmalloc should be chaned to hold a lock.
> + *
> + * In order to avoid having an already isolated mobile page
> + * being (wrongly) re-isolated while it is under migration,
> + * or to avoid attempting to isolate pages being released,
> + * lets be sure we have the page lock
> + * before proceeding with the mobile page isolation steps.
> + */
> + if (unlikely(!trylock_page(page)))
> + goto out_putpage;
> +
> + if (!(mobile_page(page) && page->mapping->a_ops->isolatepage))
> + goto out_not_isolated;
I cannot know how isolate_mobilepage is called by upper layer
because this patch doesn't include it.
Anyway, why we need such many checks to prove it's mobile page?
mobile_page() {
page->mapping && (PageMobile(page) || PageBalloon(page));
}
Additionally, added page->mapping->a_ops->isolatepage check
Is it possible that a non-LRU page's address space provides
only page->maping->migratepage without isolate/putback?
Couldn't we make it simple like this?
page->mapping && page->mapping->migratepage
So, couldn't we make mobile_page like this
PageMobile(struct page *page)
{
VM_BUG_ON_PAGE(!PageLocked(page), page);
VM_BUG_ON_PAGE(PageLRU(page), page);
return page->mapping && page->mapping->migratepage;
}
It will save _mapcount of struct page.
> + ret = page->mapping->a_ops->isolatepage(page, mode);
> + if (!ret)
> + goto out_not_isolated;
> + unlock_page(page);
> + return ret;
> +
> +out_not_isolated:
> + unlock_page(page);
> +out_putpage:
> + put_page(page);
> +out:
> + return ret;
> +}
> +
> +static inline void putback_mobilepage(struct page *page)
> +{
> + /*
> + * 'lock_page()' stabilizes the page and prevents races against
What does it mean 'stabilize'?
Clear comment is always welcome rather than vague word.
> + * concurrent isolation threads attempting to re-isolate it.
> + */
> + lock_page(page);
> + if (page->mapping && page->mapping->a_ops->putbackpage)
> + page->mapping->a_ops->putbackpage(page);
> + unlock_page(page);
> + /* drop the extra ref count taken for mobile page isolation */
> + put_page(page);
> +}
> #else
> static inline unsigned long try_to_compact_pages(gfp_t gfp_mask,
> unsigned int order, int alloc_flags,
> @@ -83,6 +150,19 @@ static inline bool compaction_deferred(struct zone *zone, int order)
> return true;
> }
>
> +static inline bool mobile_page(struct page *page)
> +{
> + return false;
> +}
> +
> +static inline bool isolate_mobilepage(struct page *page, isolate_mode_t mode)
> +{
> + return false;
> +}
> +
> +static inline void putback_mobilepage(struct page *page)
> +{
> +}
> #endif /* CONFIG_COMPACTION */
>
> #if defined(CONFIG_COMPACTION) && defined(CONFIG_SYSFS) && defined(CONFIG_NUMA)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index a0653e5..2cc4b24 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -396,6 +396,8 @@ struct address_space_operations {
> */
> int (*migratepage) (struct address_space *,
> struct page *, struct page *, enum migrate_mode);
> + bool (*isolatepage) (struct page *, isolate_mode_t);
> + void (*putbackpage) (struct page *);
> int (*launder_page) (struct page *);
> int (*is_partially_uptodate) (struct page *, unsigned long,
> unsigned long);
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index f34e040..abef145 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -582,6 +582,25 @@ static inline void __ClearPageBalloon(struct page *page)
> atomic_set(&page->_mapcount, -1);
> }
>
> +#define PAGE_MOBILE_MAPCOUNT_VALUE (-255)
> +
> +static inline int PageMobile(struct page *page)
> +{
> + return atomic_read(&page->_mapcount) == PAGE_MOBILE_MAPCOUNT_VALUE;
> +}
> +
> +static inline void __SetPageMobile(struct page *page)
> +{
> + VM_BUG_ON_PAGE(atomic_read(&page->_mapcount) != -1, page);
> + atomic_set(&page->_mapcount, PAGE_MOBILE_MAPCOUNT_VALUE);
> +}
> +
> +static inline void __ClearPageMobile(struct page *page)
> +{
> + VM_BUG_ON_PAGE(!PageMobile(page), page);
> + atomic_set(&page->_mapcount, -1);
> +}
> +
> /*
> * If network-based swap is enabled, sl*b must keep track of whether pages
> * were allocated from pfmemalloc reserves.
> diff --git a/include/uapi/linux/kernel-page-flags.h b/include/uapi/linux/kernel-page-flags.h
> index a6c4962..d50d9e8 100644
> --- a/include/uapi/linux/kernel-page-flags.h
> +++ b/include/uapi/linux/kernel-page-flags.h
> @@ -33,6 +33,7 @@
> #define KPF_THP 22
> #define KPF_BALLOON 23
> #define KPF_ZERO_PAGE 24
> +#define KPF_MOBILE 25
>
>
> #endif /* _UAPILINUX_KERNEL_PAGE_FLAGS_H */
> --
> 2.1.4
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: David Drysdale @ 2015-07-31 9:48 UTC (permalink / raw)
To: Josh Triplett, Michael Kerrisk
Cc: Linux API, Andrew Morton, Arnd Bergmann, Shuah Khan,
Jonathan Corbet, Eric B Munson, Randy Dunlap, Andrea Arcangeli,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Andy Lutomirski, Al Viro,
Rusty Russell, Peter Zijlstra, Vivek Goyal, Alexei Starovoitov,
David Herrmann, Theodore Ts'o, Kees Cook, Miklos
In-Reply-To: <20150730185007.GC16452@x>
On Thu, Jul 30, 2015 at 7:50 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Thu, Jul 30, 2015 at 08:52:11AM +0100, David Drysdale wrote:
>> Add a document describing the process of adding a new system call,
>> including the need for a flags argument for future compatibility, and
>> covering 32-bit/64-bit concerns (albeit in an x86-centric way).
>>
>> Signed-off-by: David Drysdale <drysdale@google.com>
>> Reviewed-by: Michael Kerrisk <mtk.manpages@gmail.com>
>> Reviewed-by: Eric B Munson <emunson@akamai.com>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
>
> A few comments below. I also agree with Ingo's comment about
> documenting the param-structure approach in addition to flags.
Many thanks for looking through the doc in detail!
I'll send out an updated doc today; I'm then on vacation for the next
week so I'll collate further feedback after I return.
>> --- /dev/null
>> +++ b/Documentation/adding-syscalls.txt
>> @@ -0,0 +1,463 @@
>> +Adding a New System Call
>> +========================
>> +
>> +This document describes what's involved in adding a new system call to the
>> +Linux kernel, over and above the normal submission advice in
>> +Documentation/SubmittingPatches.
>> +
>> +
>> +System Call Alternatives
>> +------------------------
>> +
>> +The first thing to consider when adding a new system call is whether one of
>> +the alternatives might be suitable instead. Although system calls are the
>> +most traditional and most obvious interaction points between userspace and the
>> +kernel, there are other possibilities -- choose what fits best for your
>> +interface.
>> +
>> + - If the operations involved can be made to look like a filesystem-like
>> + object, it may make more sense to create a new filesystem or device. This
>> + also makes it easier to encapsulate the new functionality in a kernel module
>> + rather than requiring it to be built into the main kernel.
>> + - If the new functionality involves operations where the kernel notifies
>> + userspace that something has happened, then returning a new file
>> + descriptor for the relevant object allows userspace to use
>> + poll/select/epoll to receive that notification.
>> + - However, operations that don't map to read(2)/write(2)-like operations
>> + have to be implemented as ioctl(2) requests, which can lead to a
>> + somewhat opaque API.
>> + - If you're just exposing runtime system information, a new node in sysfs
>> + (see Documentation/filesystems/sysfs.txt) or the /proc filesystem may be
>> + more appropriate. However, access to these mechanisms requires that the
>> + relevant filesystem is mounted, which might not always be the case (e.g.
>> + in a namespaced/sandboxed/chrooted environment). Avoid adding anything to
>> + debugfs, as this is not considered a 'production' interface to userspace.
>
> s/anything/any "API"/
Done.
>> + - If the operation is specific to a particular file or file descriptor, then
>> + an additional fcntl(2) command option may be more appropriate. However,
>> + fcntl(2) is a multiplexing system call that hides a lot of complexity, so
>> + this option is best for when the new function is closely analogous to
>> + existing fcntl(2) functionality, or the new functionality is very simple
>> + (for example, getting/setting a simple flag related to a file descriptor).
>> + - If the operation is specific to a particular task or process, then an
>> + additional prctl(2) command option may be more appropriate. As with
>> + fcntl(2), this system call is a complicated multiplexor so is best reserved
>> + for near-analogs of existing prctl() commands or getting/setting a simple
>> + flag related to a process.
>> +
>> +
>> +Designing the API
>> +-----------------
>> +
>> +A new system call forms part of the API of the kernel, and has to be supported
>> +indefinitely. As such, it's a very good idea to explicitly discuss the
>> +interface on the kernel mailing list, and to plan for future extensions of the
>> +interface. In particular:
>> +
>> + **Include a flags argument for every new system call**
>> +
>> +The syscall table is littered with historical examples where this wasn't done,
>> +together with the corresponding follow-up system calls (eventfd/eventfd2,
>> +dup2/dup3, inotify_init/inotify_init1, pipe/pipe2, renameat/renameat2), so
>> +learn from the history of the kernel and include a flags argument from the
>> +start.
>> +
>> +Also, to make sure that userspace programs can safely use flags between kernel
>> +versions, check whether the flags value holds any unknown flags, and reject the
>> +sycall (with EINVAL) if it does:
>> +
>> + if (flags & ~(THING_FLAG1 | THING_FLAG2 | THING_FLAG3))
>> + return -EINVAL;
>> +
>> +(If no flags values are used yet, check that the flags argument is zero.)
>
> Some things to add a this point, before talking about adding a new file
> descriptor:
>
> If you want to provide userspace with a handle to a kernel object, do so
> in the form of a file descriptor. Do not invent a new type of userspace
> object handle with its own namespace. The kernel already has numerous
> system calls and well-defined semantics for managing and passing around
> file descriptors, as well as for cleaning up when the file descriptor is
> no longer in use.
>
> If your new system call returns a file descriptor, think about what it
> means to use the poll(2) family of syscalls on that file descriptor. If
> you want to send an event to userspace associated with your kernel
> object, you should do so by making your file descriptor ready for
> reading or writing.
Good points, I've added a couple of paragraphs along these lines:
"
If your new system call allows userspace to refer to a kernel object, it
should use a file descriptor as the handle for that object -- don't invent a
new type of userspace object handle when the kernel already has mechanisms and
well-defined semantics for using file descriptors.
[snip: existing text on CLOEXEC]
If your system call returns a new file descriptor, you should also consider
what it means to use the poll(2) family of system calls on that file
descriptor. Making a file descriptor ready for reading or writing is the
normal way for the kernel to indicate to userspace that an event has
occurred on the corresponding kernel object.
"
(Given that I'm working on Capsicum, which treats file descriptors as
object capabilities, the first of those points is particularly important!)
>> +If your new xyzzy(2) system call returns a new file descriptor, then the flags
>> +argument should include a value that is equivalent to setting O_CLOEXEC on the
>> +new FD. This makes it possible for userspace to close the timing window
>> +between xyzzy() and calling fcntl(fd, F_SETFD, FD_CLOEXEC), where an
>> +unexpected fork() and execve() in another thread could leak a descriptor to
>> +the exec'ed program. (However, resist the temptation to re-use the actual value
>> +of the O_CLOEXEC constant, as it is architecture-specific and is part of a
>> +numbering space of O_* flags that is fairly full.)
>> +
>> +If your new xyzzy(2) system call involves a filename argument:
>> +
>> + int sys_xyzzy(const char __user *path, ..., unsigned int flags);
>> +
>> +you should also consider whether an xyzzyat(2) version is more appropriate:
>> +
>> + int sys_xyzzyat(int dfd, const char __user *path, ..., unsigned int flags);
>> +
>> +This allows more flexibility for how userspace specifies the file in question;
>> +in particular it allows userspace to request the functionality for an
>> +already-opened file descriptor using the AT_EMPTY_PATH flag, effectively giving
>> +an fxyzzy(3) operation for free:
>> +
>> + - xyzzyat(AT_FDCWD, path, ..., 0) is equivalent to xyzzy(path,...)
>> + - xyzzyat(fd, "", ..., AT_EMPTY_PATH) is equivalent to fxyzzy(fd, ...)
>> +
>> +(For more details on the rationale of the *at() calls, see the openat(2) man
>> +page; for an example of AT_EMPTY_PATH, see the statat(2) man page.)
>> +
>> +If your new xyzzy(2) system call involves a parameter describing an offset
>> +within a file, make its type loff_t so that 64-bit offsets can be supported
>> +even on 32-bit architectures.
>> +
>> +If your new xyzzy(2) system call involves administrative functionality, it
>
> s/administrative/privileged/
Done.
>> +needs to be governed by the appropriate Linux capability bit (checked with a
>> +call to capable()), as described in the capabilities(7) man page.
>> +
>> + - If there is an existing capability that governs related functionality, then
>> + use that. However, avoid combining lots of only vaguely related functions
>> + together under the same bit, as this goes against capabilities' purpose of
>> + splitting the power of root. In particular, avoid adding new uses of the
>> + already overly-general CAP_SYS_ADMIN capability.
>> + - If there is no related capability, then consider adding a new capability
>> + bit -- but bear in mind that the numbering space is limited, and each new
>> + bit needs to be understood and administered by sysadmins.
>
> Many previous discussions on this topic seem to have concluded that it's
> almost impossible to add a new capability without breaking existing
> programs. I would suggest not even mentioning this possibility.
I'm not particularly knowledgable about capabilities (at least, not the
POSIX.1e/Linux kind), so I'll confess that I got this suggestion from
Michael Kerrisk.
Michael, do you agree that we should just drop the possibility of adding
new capability bits?
Also, Josh, do you have any references to the earlier discussions on the
topic so I can update the Sources section?
> There's another possibility here that should be discussed:
>
> "If your system call manipulates a process other than the calling
> process, the correct permission check is ptrace_may_access, which checks
> either that the calling process has all the same permissions as the
> target process or that the calling process has the necessary
> capabilities to manipulate any process."
I've added a paragraph along those lines:
"
If your new xyzzy(2) system call manipulates a process other than the calling
process, it should be restricted (using a call to ptrace_may_access()) so that
only a calling process with the same permissions as the target process, or
with the necessary capabilities, can manipulate the target process.
"
>> +Finally, be aware that some non-x86 architectures have an easier time if
>> +system call parameters that are explicitly 64-bit fall on odd-numbered
>> +arguments (i.e. parameter 1, 3, 5), to allow use of contiguous pairs of 32-bit
>> +registers.
>
> "Alternatively, note that this limitation does not apply to parameters
> passed in a data structure, as mentioned above."
>
> (in reference to the new additions Ingo proposed)
Done.
>> +Proposing the API
>> +-----------------
>> +
>> +To make new system calls easy to review, it's best to divide up the patchset
>> +into separate chunks. These should include at least the following items as
>> +distinct commits (each of which is described further below):
>> +
>> + - The core implementation of the system call together with prototypes, generic
>
> s/call together/call, together/
Done.
>> + numbering and fallback stub implementation.
>
> And any Kconfig bits.
Done.
>> + - Wiring up of the new system call for one particular architecture, usually
>> + x86 (including all of x86_64, x86_32 and x32).
>> + - A demonstration of the use of the new system call in userspace via a
>> + selftest in tools/testing/selftests/.
>> + - A draft man-page for the new system call.
>
> "either as a patch to the man-pages repository (not the kernel), or in
> plain-text format in the cover letter".
Done.
>> +New system call proposals, like any change to the kernel's API, should always
>> +be cc'ed to linux-api@vger.kernel.org
>> +
>> +
>> +Generic System Call Implementation
>> +----------------------------------
>> +
>> +The main entry point for your new xyzzy(2) system call will be called
>> +sys_xyzzy(), but you add this entry point with the appropriate
>> +SYSCALL_DEFINEn() macro rather than explicitly. The 'n' indicates the number
>> +of arguments to the system call, and the macro takes the system call name
>> +followed by the (type, name) pairs for the parameters as arguments. Using
>> +this macro allows metadata about the new system call to be made available for
>> +other tools.
>> +
>> +The new entry point also needs a corresponding function prototype, in
>> +include/linux/syscalls.h, marked as asmlinkage to match the way that system
>> +calls are invoked:
>> +
>> + asmlinkage long sys_xyzzy(...);
>> +
>> +Some architectures (e.g. x86) have their own architecture-specific syscall
>> +tables, but several other architectures share a generic syscall table. Add your
>> +new system call to the generic list by adding an entry to the list in
>> +include/uapi/asm-generic/unistd.h:
>> +
>> + #define __NR_xyzzy 292
>> + __SYSCALL(__NR_xyzzy, sys_xyzzy)
>> +
>> +Also update the __NR_syscalls count to reflect the additional system call, and
>> +note that if multiple new system calls are added in the same merge window,
>> +your new syscall number may get adjusted to resolve conflicts.
>> +
>> +The file kernel/sys_ni.c provides a fallback stub implementation of each system
>> +call, returning -ENOSYS. Add your new system call here too:
>> +
>> + cond_syscall(sys_xyzzy);
>
> Please add something like this as well, to go along with the
> cond_syscall bit:
>
> Your new system call should be optional. Add a new Kconfig option
> CONFIG_XYZZY_SYSCALL" (typically in init/Kconfig), possibly depending on
> EXPERT, with a description of the new system call. Make the source
> files implementing your system call depend on that new Kconfig option
> (obj-$(CONFIG_XYZZY_SYSCALL) += xyzzy.c).
Added a paragraph along those lines:
"
Your new kernel functionality, and the system call that controls it, should
normally be optional, so add a CONFIG option (typically to init/Kconfig) for
it. As usual for new CONFIG options:
- Include a description of the new functionality and system call controlled
by the option.
- Make the option depend on EXPERT if it should be hidden from normal users.
- Make any new source files implementing the function dependent on the CONFIG
option in the Makefile (e.g. "obj-$(CONFIG_XYZZY_SYSCALL) += xyzzy.c").
- Double check that the kernel still builds with the new CONFIG option turned
off.
"
>> +To summarize, you need a commit that includes:
>> +
>> + - SYSCALL_DEFINEn(xyzzy, ...) for the entry point
>> + - corresponding prototype in include/linux/syscalls.h
>> + - generic table entry in include/uapi/asm-generic/unistd.h
>> + - fallback stub in kernel/sys_ni.c
>
> - new Kconfig symbol, typically in init/Kconfig
Done.
>> +Testing
>> +-------
>> +
>> +A new system call should obviously be tested; it is also useful to provide
>> +reviewers with a demonstration of how user space programs will use the system
>> +call. A good way to combine these aims is to include a simple self-test
>> +program in a new directory under tools/testing/selftests/.
>> +
>> +For a new system call, there will obviously be no libc wrapper function and so
>> +the test will need to invoke it using syscall(); also, if the system call
>> +involves a new userspace-visible structure, the corresponding header will need
>> +to be installed to compile the test.
>> +
>> +Make sure the selftest runs successfully on all supported architectures. For
>> +example, check that it works when compiled as an x86_64 (-m64), x86_32 (-m32)
>> +and x32 (-mx32) ABI program.
>
> "Make sure the kernel compiles when you configure out your new system
> call."
Added, albeit higher up.
>> +Man Page
>> +--------
>> +
>> +All new system calls should come with a complete man page, ideally using groff
>> +markup, but plain text will do. If groff is used, it's helpful to include a
>> +pre-rendered ASCII version of the man page in the cover email for the
>> +patchset, for the convenience of reviewers.
>> +
>> +The man page should be cc'ed to linux-man@vger.kernel.org
>> +For more details, see https://www.kernel.org/doc/man-pages/patches.html
>> +
>> +References and Sources
>> +----------------------
>> +
>> + - LWN article from Michael Kerrisk on use of flags argument in system calls:
>> + https://lwn.net/Articles/585415/
>> + - LWN article from Michael Kerrisk on how to handle unknown flags in a system
>> + call: https://lwn.net/Articles/588444/
>> + - LWN article from Jake Edge describing constraints on 64-bit system call
>> + arguments: https://lwn.net/Articles/311630/
>> + - Pair of LWN articles from David Drysdale that describe the system call
>> + implementation paths in detail for v3.14:
>> + - https://lwn.net/Articles/604287/
>> + - https://lwn.net/Articles/604515/
>> + - Architecture-specific requirements for system calls are discussed in the
>> + syscall(2) man-page:
>> + http://man7.org/linux/man-pages/man2/syscall.2.html#NOTES
>> + - Collated emails from Linus Torvalds discussing the problems with ioctl():
>> + http://yarchive.net/comp/linux/ioctl.html
>> + - "How to not invent kernel interfaces", Arnd Bergmann,
>> + http://www.ukuug.org/events/linux2007/2007/papers/Bergmann.pdf
>> + - LWN article from Michael Kerrisk on avoiding new uses of CAP_SYS_ADMIN:
>> + https://lwn.net/Articles/486306/
>> +
>
> Nit: why is there a blank line here but not elsewhere in this list?
I was originally trying to separate references from sources, but as I
added more things the distinction got blurred -- removed.
>> + - Recommendation from Andrew Morton that all related information for a new
>> + system call should come in the same email thread:
>> + https://lkml.org/lkml/2014/7/24/641
>> + - Recommendation from Michael Kerrisk that a new system call should come with
>> + a man page: https://lkml.org/lkml/2014/6/13/309
>> + - Suggestion from Thomas Gleixner that x86 wire-up should be in a separate
>> + commit: https://lkml.org/lkml/2014/11/19/254
>> + - Suggestion from Greg Kroah-Hartman that it's good for new system calls to
>> + come with a man-page & selftest: https://lkml.org/lkml/2014/3/19/710
>> + - Discussion from Michael Kerrisk of new system call vs. prctl(2) extension:
>> + https://lkml.org/lkml/2014/6/3/411
>> + - Numbering oddities arising from (re-)use of O_* numbering space flags:
>> + - commit 75069f2b5bfb ("vfs: renumber FMODE_NONOTIFY and add to uniqueness
>> + check")
>> + - commit 12ed2e36c98a ("fanotify: FMODE_NONOTIFY and __O_SYNC in sparc
>> + conflict")
>> + - commit bb458c644a59 ("Safer ABI for O_TMPFILE")
>> + - Discussion from Matthew Wilcox about restrictions on 64-bit arguments:
>> + https://lkml.org/lkml/2008/12/12/187
>> + - Recommendation from Greg Kroah-Hartman that unknown flags should be
>> + policed: https://lkml.org/lkml/2014/7/17/577
>> + - Recommendation from Linus Torvalds that x32 system calls should prefer
>> + compatibility with 64-bit versions rather than 32-bit versions:
>> + https://lkml.org/lkml/2011/8/31/244
>> --
>> 2.5.0.rc2.392.g76e840b
^ permalink raw reply
* Re: [PATCH -mm v9 0/8] idle memory tracking
From: Vladimir Davydov @ 2015-07-31 9:34 UTC (permalink / raw)
To: Andrew Morton
Cc: Michal Hocko, Andres Lagar-Cavilla, Minchan Kim, Raghavendra K T,
Johannes Weiner, Greg Thelen, Michel Lespinasse, David Rientjes,
Pavel Emelyanov, Cyrill Gorcunov, Jonathan Corbet, linux-api,
linux-doc, linux-mm, cgroups, linux-kernel
In-Reply-To: <20150730130122.GC8100@esperanza>
On Thu, Jul 30, 2015 at 04:01:22PM +0300, Vladimir Davydov wrote:
> On Thu, Jul 30, 2015 at 12:12:12PM +0300, Vladimir Davydov wrote:
> > On Wed, Jul 29, 2015 at 02:30:15PM -0700, Andrew Morton wrote:
> > > On Wed, 29 Jul 2015 19:29:08 +0300 Vladimir Davydov <vdavydov@parallels.com> wrote:
> > >
> > > > /proc/kpageidle should probably live somewhere in /sys/kernel/mm, but I
> > > > added it where similar files are located (kpagecount, kpageflags) to
> > > > keep things consistent.
> > >
> > > I think these files should be moved elsewhere. Consistency is good,
> > > but not when we're being consistent with a bad thing.
> > >
> > > So let's place these in /sys/kernel/mm and then start being consistent
> > > with that?
> >
> > I really don't think we should separate kpagecgroup from kpagecount and
> > kpageflags, because they look very similar (each of them is read-only,
> > contains an array of u64 values referenced by PFN). Scattering these
> > files between different filesystems would look ugly IMO.
> >
> > However, kpageidle is somewhat different (it's read-write, contains a
> > bitmap) so I think it's worth moving it to /sys/kernel/mm. We have to
> > move the code from fs/proc to mm/something then to remove dependency
> > from PROC_FS, which would be unnecessary. Let me give it a try.
>
> Here it goes:
>
> From: Vladimir Davydov <vdavydov@parallels.com>
> Subject: [PATCH] Move /proc/kpageidle to /sys/kernel/mm/page_idle/bitmap
Since it is rather difficult to merge it into proc-add-kpageidle, should
I resend the whole series with all fixes included, provided you find
this patch OK of course?
Thanks,
Vladimir
^ permalink raw reply
* Re: [PATCH] userfaultfd: register syscall numbers for x86 32bit and x86-64 64bit
From: Dr. David Alan Gilbert @ 2015-07-31 9:13 UTC (permalink / raw)
To: Ingo Molnar
Cc: Andrea Arcangeli, Thomas Gleixner, Stephen Rothwell,
Andrew Morton, Ingo Molnar, \"H. Peter Anvin\",
Peter Zijlstra, linux-next, linux-api, linux-kernel,
Andy Lutomirski, Eric B Munson, Pavel Emelyanov, Linus Torvalds,
Peter Zijlstra
In-Reply-To: <20150731083130.GA24624@gmail.com>
* Ingo Molnar (mingo@kernel.org) wrote:
>
> * Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> > This registers the official numbers of the userfaultfd syscall for x86
> > 32bit and x86-64 64bit. This registration allows to ship kernels in
> > production using these two syscall numbers for userfaultfd.
> >
> > Acked-by: Pavel Emelyanov <xemul@parallels.com>
> > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> > ---
> > arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> > arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> > include/linux/syscalls.h | 1 +
> > kernel/sys_ni.c | 1 +
> > 4 files changed, 4 insertions(+)
>
> Hm, is there consensus on the upstream desirability of this sytem call?
I hope so:
a) It's generic enough that there's interest in using it in
a range of projects - there's nothing QEMU specific in it.
b) It's worked very well on the QEMU code that I wrote that uses it
for postcopy, and has been solid doing that for many months now.
c) I'm not aware of any suggestions for a different, generic solution
to solve the same set of problems.
d) It's got a good self test, that's survived heavy testing.
e) I think Andrea has addressed all previous comments on it.
Dave
>
> Thanks,
>
> Ingo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply
* Re: [PATCH] userfaultfd: register syscall numbers for x86 32bit and x86-64 64bit
From: Ingo Molnar @ 2015-07-31 8:31 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Thomas Gleixner, Stephen Rothwell, Andrew Morton, Ingo Molnar,
\"H. Peter Anvin\", Peter Zijlstra,
linux-next-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andy Lutomirski,
Eric B Munson, Pavel Emelyanov,
\"Dr. David Alan Gilbert\", Linus Torvalds,
Peter Zijlstra
In-Reply-To: <1438270467-13324-1-git-send-email-aarcange-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
* Andrea Arcangeli <aarcange-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> This registers the official numbers of the userfaultfd syscall for x86
> 32bit and x86-64 64bit. This registration allows to ship kernels in
> production using these two syscall numbers for userfaultfd.
>
> Acked-by: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Signed-off-by: Andrea Arcangeli <aarcange-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> include/linux/syscalls.h | 1 +
> kernel/sys_ni.c | 1 +
> 4 files changed, 4 insertions(+)
Hm, is there consensus on the upstream desirability of this sytem call?
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Josh Triplett @ 2015-07-31 1:03 UTC (permalink / raw)
To: Kees Cook
Cc: David Drysdale, Ingo Molnar, Linux API, Michael Kerrisk,
Andrew Morton, Arnd Bergmann, Shuah Khan, Jonathan Corbet,
Eric B Munson, Randy Dunlap, Andrea Arcangeli, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, Oleg Nesterov, Linus Torvalds,
Greg Kroah-Hartman, Andy Lutomirski, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <20150731010234.GA7265@x>
On Thu, Jul 30, 2015 at 06:02:34PM -0700, Josh Triplett wrote:
> On Thu, Jul 30, 2015 at 01:03:43PM -0700, Kees Cook wrote:
> > On Thu, Jul 30, 2015 at 12:04 PM, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote:
> > > On Thu, Jul 30, 2015 at 11:21:54AM -0700, Kees Cook wrote:
> > >> I like this, it's a good description of both options. I'm still biased
> > >> about the approach: I prefer flags, since pointers to user structures
> > >> complicate syscall filtering. ;)
> > >
> > > Seems like we should do two things to make that easier:
> > >
> > > 1) Create a standardized kernel mechanism for parameter-struct handling,
> > > implementing the recommendations mentioned here.
> >
> > It's been suggested in the past that nlmsg is appropriate for such a
> > thing, but I remain suspicious. :)
>
> Likewise. :)
>
> > > 2) Integrate into that mechanism a way to filter the resulting parameter
> > > struct with BPF *after* it has been copied to kernel space (and thus
> > > can no longer be tampered with).
> >
> > Yeah, this is a irritating part: the structures operated on are copied
> > from userspace adhoc in each syscall. Doing argument checking would
> > mean double copies initially, and perhaps teaching syscalls about
> > optional "already copied" arguments or something as an optimization.
>
> No, double copies can't work for security reasons. Because otherwise
> you could race the kernel from another thread, substituting different
> values after the check and before the use.
>
> I think the right API looks *roughly* like this:
>
> int _copy_param_struct(size_t kernel_len, void *kernel_struct, size_t user_len, void __user *user_struct)
> {
> if (user_len > kernel_len)
> return -EINVAL;
> if (user_len && copy_from_user(kernel_struct, user_struct, user_len))
> return -EFAULT;
> if (user_len < kernel_len)
> memset(kernel_struct + user_len, 0, kernel_len - user_len);
> return 0;
> }
>
> #define copy_param_struct(kernel_struct, user_len, user_struct) _copy_param_struct( \
> sizeof(*kernel_struct) + BUILD_BUG_ON_ZERO(!__same_type(*kernel_struct, *user_struct)), \
> kernel_struct, user_len, user_struct)
>
>
> Then the syscall looks like this:
>
> SYSCALL_DEFINEn(xyzzy, ..., ..., size_t user_params_len, struct xyzzy_params __user *user_params)
Missed a couple of commas here (after the types and before the names).
> {
> int ret;
> struct xyzzy_params params;
>
> ret = copy_param_struct(¶ms, user_params_len, user_params);
> if (ret)
> return ret;
> ...
>
>
> And you could then hook copy_params_struct to add arbitrary additional
> syscall parameter validation. Bonus if there's some way to make the
> copy and validation occur before the syscall is ever invoked, rather
> than inside the syscall, but that would require adding fancier syscall
> definition mechanisms that autogenerate such code.
>
> - Josh Triplett
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Josh Triplett @ 2015-07-31 1:02 UTC (permalink / raw)
To: Kees Cook
Cc: David Drysdale, Ingo Molnar, Linux API, Michael Kerrisk,
Andrew Morton, Arnd Bergmann, Shuah Khan, Jonathan Corbet,
Eric B Munson, Randy Dunlap, Andrea Arcangeli, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, Oleg Nesterov, Linus Torvalds,
Greg Kroah-Hartman, Andy Lutomirski, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <CAGXu5jJwtVgPmmMHiR0M27he3ma7exfSH8OBNdLMFQcbbtzUfg@mail.gmail.com>
On Thu, Jul 30, 2015 at 01:03:43PM -0700, Kees Cook wrote:
> On Thu, Jul 30, 2015 at 12:04 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Thu, Jul 30, 2015 at 11:21:54AM -0700, Kees Cook wrote:
> >> I like this, it's a good description of both options. I'm still biased
> >> about the approach: I prefer flags, since pointers to user structures
> >> complicate syscall filtering. ;)
> >
> > Seems like we should do two things to make that easier:
> >
> > 1) Create a standardized kernel mechanism for parameter-struct handling,
> > implementing the recommendations mentioned here.
>
> It's been suggested in the past that nlmsg is appropriate for such a
> thing, but I remain suspicious. :)
Likewise. :)
> > 2) Integrate into that mechanism a way to filter the resulting parameter
> > struct with BPF *after* it has been copied to kernel space (and thus
> > can no longer be tampered with).
>
> Yeah, this is a irritating part: the structures operated on are copied
> from userspace adhoc in each syscall. Doing argument checking would
> mean double copies initially, and perhaps teaching syscalls about
> optional "already copied" arguments or something as an optimization.
No, double copies can't work for security reasons. Because otherwise
you could race the kernel from another thread, substituting different
values after the check and before the use.
I think the right API looks *roughly* like this:
int _copy_param_struct(size_t kernel_len, void *kernel_struct, size_t user_len, void __user *user_struct)
{
if (user_len > kernel_len)
return -EINVAL;
if (user_len && copy_from_user(kernel_struct, user_struct, user_len))
return -EFAULT;
if (user_len < kernel_len)
memset(kernel_struct + user_len, 0, kernel_len - user_len);
return 0;
}
#define copy_param_struct(kernel_struct, user_len, user_struct) _copy_param_struct( \
sizeof(*kernel_struct) + BUILD_BUG_ON_ZERO(!__same_type(*kernel_struct, *user_struct)), \
kernel_struct, user_len, user_struct)
Then the syscall looks like this:
SYSCALL_DEFINEn(xyzzy, ..., ..., size_t user_params_len, struct xyzzy_params __user *user_params)
{
int ret;
struct xyzzy_params params;
ret = copy_param_struct(¶ms, user_params_len, user_params);
if (ret)
return ret;
...
And you could then hook copy_params_struct to add arbitrary additional
syscall parameter validation. Bonus if there's some way to make the
copy and validation occur before the syscall is ever invoked, rather
than inside the syscall, but that would require adding fancier syscall
definition mechanisms that autogenerate such code.
- Josh Triplett
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Kees Cook @ 2015-07-30 20:03 UTC (permalink / raw)
To: Josh Triplett
Cc: David Drysdale, Ingo Molnar, Linux API, Michael Kerrisk,
Andrew Morton, Arnd Bergmann, Shuah Khan, Jonathan Corbet,
Eric B Munson, Randy Dunlap, Andrea Arcangeli, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, Oleg Nesterov, Linus Torvalds,
Greg Kroah-Hartman, Andy Lutomirski, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <20150730190434.GD16452@x>
On Thu, Jul 30, 2015 at 12:04 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Thu, Jul 30, 2015 at 11:21:54AM -0700, Kees Cook wrote:
>> I like this, it's a good description of both options. I'm still biased
>> about the approach: I prefer flags, since pointers to user structures
>> complicate syscall filtering. ;)
>
> Seems like we should do two things to make that easier:
>
> 1) Create a standardized kernel mechanism for parameter-struct handling,
> implementing the recommendations mentioned here.
It's been suggested in the past that nlmsg is appropriate for such a
thing, but I remain suspicious. :)
> 2) Integrate into that mechanism a way to filter the resulting parameter
> struct with BPF *after* it has been copied to kernel space (and thus
> can no longer be tampered with).
Yeah, this is a irritating part: the structures operated on are copied
from userspace adhoc in each syscall. Doing argument checking would
mean double copies initially, and perhaps teaching syscalls about
optional "already copied" arguments or something as an optimization.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Josh Triplett @ 2015-07-30 19:04 UTC (permalink / raw)
To: Kees Cook
Cc: David Drysdale, Ingo Molnar, Linux API, Michael Kerrisk,
Andrew Morton, Arnd Bergmann, Shuah Khan, Jonathan Corbet,
Eric B Munson, Randy Dunlap, Andrea Arcangeli, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, Oleg Nesterov, Linus Torvalds,
Greg Kroah-Hartman, Andy Lutomirski, Al Viro, Rusty Russell,
Peter Zijlstra, Vivek Goyal, Alexei Starovoitov
In-Reply-To: <CAGXu5j+5KHy68ELU6PmNWaj7mQBXTbRQGXqJFwsXHt9n0LPw8Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Jul 30, 2015 at 11:21:54AM -0700, Kees Cook wrote:
> I like this, it's a good description of both options. I'm still biased
> about the approach: I prefer flags, since pointers to user structures
> complicate syscall filtering. ;)
Seems like we should do two things to make that easier:
1) Create a standardized kernel mechanism for parameter-struct handling,
implementing the recommendations mentioned here.
2) Integrate into that mechanism a way to filter the resulting parameter
struct with BPF *after* it has been copied to kernel space (and thus
can no longer be tampered with).
- Josh Triplett
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Josh Triplett @ 2015-07-30 18:50 UTC (permalink / raw)
To: David Drysdale
Cc: linux-api, Michael Kerrisk, Andrew Morton, Arnd Bergmann,
Shuah Khan, Jonathan Corbet, Eric B Munson, Randy Dunlap,
Andrea Arcangeli, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Oleg Nesterov, Linus Torvalds, Greg Kroah-Hartman,
Andy Lutomirski, Al Viro, Rusty Russell, Peter Zijlstra,
Vivek Goyal, Alexei Starovoitov, David Herrmann,
Theodore Ts'o, Kees Cook <keesc>
In-Reply-To: <1438242731-27756-2-git-send-email-drysdale@google.com>
On Thu, Jul 30, 2015 at 08:52:11AM +0100, David Drysdale wrote:
> Add a document describing the process of adding a new system call,
> including the need for a flags argument for future compatibility, and
> covering 32-bit/64-bit concerns (albeit in an x86-centric way).
>
> Signed-off-by: David Drysdale <drysdale@google.com>
> Reviewed-by: Michael Kerrisk <mtk.manpages@gmail.com>
> Reviewed-by: Eric B Munson <emunson@akamai.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
A few comments below. I also agree with Ingo's comment about
documenting the param-structure approach in addition to flags.
> --- /dev/null
> +++ b/Documentation/adding-syscalls.txt
> @@ -0,0 +1,463 @@
> +Adding a New System Call
> +========================
> +
> +This document describes what's involved in adding a new system call to the
> +Linux kernel, over and above the normal submission advice in
> +Documentation/SubmittingPatches.
> +
> +
> +System Call Alternatives
> +------------------------
> +
> +The first thing to consider when adding a new system call is whether one of
> +the alternatives might be suitable instead. Although system calls are the
> +most traditional and most obvious interaction points between userspace and the
> +kernel, there are other possibilities -- choose what fits best for your
> +interface.
> +
> + - If the operations involved can be made to look like a filesystem-like
> + object, it may make more sense to create a new filesystem or device. This
> + also makes it easier to encapsulate the new functionality in a kernel module
> + rather than requiring it to be built into the main kernel.
> + - If the new functionality involves operations where the kernel notifies
> + userspace that something has happened, then returning a new file
> + descriptor for the relevant object allows userspace to use
> + poll/select/epoll to receive that notification.
> + - However, operations that don't map to read(2)/write(2)-like operations
> + have to be implemented as ioctl(2) requests, which can lead to a
> + somewhat opaque API.
> + - If you're just exposing runtime system information, a new node in sysfs
> + (see Documentation/filesystems/sysfs.txt) or the /proc filesystem may be
> + more appropriate. However, access to these mechanisms requires that the
> + relevant filesystem is mounted, which might not always be the case (e.g.
> + in a namespaced/sandboxed/chrooted environment). Avoid adding anything to
> + debugfs, as this is not considered a 'production' interface to userspace.
s/anything/any "API"/
> + - If the operation is specific to a particular file or file descriptor, then
> + an additional fcntl(2) command option may be more appropriate. However,
> + fcntl(2) is a multiplexing system call that hides a lot of complexity, so
> + this option is best for when the new function is closely analogous to
> + existing fcntl(2) functionality, or the new functionality is very simple
> + (for example, getting/setting a simple flag related to a file descriptor).
> + - If the operation is specific to a particular task or process, then an
> + additional prctl(2) command option may be more appropriate. As with
> + fcntl(2), this system call is a complicated multiplexor so is best reserved
> + for near-analogs of existing prctl() commands or getting/setting a simple
> + flag related to a process.
> +
> +
> +Designing the API
> +-----------------
> +
> +A new system call forms part of the API of the kernel, and has to be supported
> +indefinitely. As such, it's a very good idea to explicitly discuss the
> +interface on the kernel mailing list, and to plan for future extensions of the
> +interface. In particular:
> +
> + **Include a flags argument for every new system call**
> +
> +The syscall table is littered with historical examples where this wasn't done,
> +together with the corresponding follow-up system calls (eventfd/eventfd2,
> +dup2/dup3, inotify_init/inotify_init1, pipe/pipe2, renameat/renameat2), so
> +learn from the history of the kernel and include a flags argument from the
> +start.
> +
> +Also, to make sure that userspace programs can safely use flags between kernel
> +versions, check whether the flags value holds any unknown flags, and reject the
> +sycall (with EINVAL) if it does:
> +
> + if (flags & ~(THING_FLAG1 | THING_FLAG2 | THING_FLAG3))
> + return -EINVAL;
> +
> +(If no flags values are used yet, check that the flags argument is zero.)
Some things to add a this point, before talking about adding a new file
descriptor:
If you want to provide userspace with a handle to a kernel object, do so
in the form of a file descriptor. Do not invent a new type of userspace
object handle with its own namespace. The kernel already has numerous
system calls and well-defined semantics for managing and passing around
file descriptors, as well as for cleaning up when the file descriptor is
no longer in use.
If your new system call returns a file descriptor, think about what it
means to use the poll(2) family of syscalls on that file descriptor. If
you want to send an event to userspace associated with your kernel
object, you should do so by making your file descriptor ready for
reading or writing.
> +If your new xyzzy(2) system call returns a new file descriptor, then the flags
> +argument should include a value that is equivalent to setting O_CLOEXEC on the
> +new FD. This makes it possible for userspace to close the timing window
> +between xyzzy() and calling fcntl(fd, F_SETFD, FD_CLOEXEC), where an
> +unexpected fork() and execve() in another thread could leak a descriptor to
> +the exec'ed program. (However, resist the temptation to re-use the actual value
> +of the O_CLOEXEC constant, as it is architecture-specific and is part of a
> +numbering space of O_* flags that is fairly full.)
> +
> +If your new xyzzy(2) system call involves a filename argument:
> +
> + int sys_xyzzy(const char __user *path, ..., unsigned int flags);
> +
> +you should also consider whether an xyzzyat(2) version is more appropriate:
> +
> + int sys_xyzzyat(int dfd, const char __user *path, ..., unsigned int flags);
> +
> +This allows more flexibility for how userspace specifies the file in question;
> +in particular it allows userspace to request the functionality for an
> +already-opened file descriptor using the AT_EMPTY_PATH flag, effectively giving
> +an fxyzzy(3) operation for free:
> +
> + - xyzzyat(AT_FDCWD, path, ..., 0) is equivalent to xyzzy(path,...)
> + - xyzzyat(fd, "", ..., AT_EMPTY_PATH) is equivalent to fxyzzy(fd, ...)
> +
> +(For more details on the rationale of the *at() calls, see the openat(2) man
> +page; for an example of AT_EMPTY_PATH, see the statat(2) man page.)
> +
> +If your new xyzzy(2) system call involves a parameter describing an offset
> +within a file, make its type loff_t so that 64-bit offsets can be supported
> +even on 32-bit architectures.
> +
> +If your new xyzzy(2) system call involves administrative functionality, it
s/administrative/privileged/
> +needs to be governed by the appropriate Linux capability bit (checked with a
> +call to capable()), as described in the capabilities(7) man page.
> +
> + - If there is an existing capability that governs related functionality, then
> + use that. However, avoid combining lots of only vaguely related functions
> + together under the same bit, as this goes against capabilities' purpose of
> + splitting the power of root. In particular, avoid adding new uses of the
> + already overly-general CAP_SYS_ADMIN capability.
> + - If there is no related capability, then consider adding a new capability
> + bit -- but bear in mind that the numbering space is limited, and each new
> + bit needs to be understood and administered by sysadmins.
Many previous discussions on this topic seem to have concluded that it's
almost impossible to add a new capability without breaking existing
programs. I would suggest not even mentioning this possibility.
There's another possibility here that should be discussed:
"If your system call manipulates a process other than the calling
process, the correct permission check is ptrace_may_access, which checks
either that the calling process has all the same permissions as the
target process or that the calling process has the necessary
capabilities to manipulate any process."
> +Finally, be aware that some non-x86 architectures have an easier time if
> +system call parameters that are explicitly 64-bit fall on odd-numbered
> +arguments (i.e. parameter 1, 3, 5), to allow use of contiguous pairs of 32-bit
> +registers.
"Alternatively, note that this limitation does not apply to parameters
passed in a data structure, as mentioned above."
(in reference to the new additions Ingo proposed)
> +Proposing the API
> +-----------------
> +
> +To make new system calls easy to review, it's best to divide up the patchset
> +into separate chunks. These should include at least the following items as
> +distinct commits (each of which is described further below):
> +
> + - The core implementation of the system call together with prototypes, generic
s/call together/call, together/
> + numbering and fallback stub implementation.
And any Kconfig bits.
> + - Wiring up of the new system call for one particular architecture, usually
> + x86 (including all of x86_64, x86_32 and x32).
> + - A demonstration of the use of the new system call in userspace via a
> + selftest in tools/testing/selftests/.
> + - A draft man-page for the new system call.
"either as a patch to the man-pages repository (not the kernel), or in
plain-text format in the cover letter".
> +New system call proposals, like any change to the kernel's API, should always
> +be cc'ed to linux-api@vger.kernel.org
> +
> +
> +Generic System Call Implementation
> +----------------------------------
> +
> +The main entry point for your new xyzzy(2) system call will be called
> +sys_xyzzy(), but you add this entry point with the appropriate
> +SYSCALL_DEFINEn() macro rather than explicitly. The 'n' indicates the number
> +of arguments to the system call, and the macro takes the system call name
> +followed by the (type, name) pairs for the parameters as arguments. Using
> +this macro allows metadata about the new system call to be made available for
> +other tools.
> +
> +The new entry point also needs a corresponding function prototype, in
> +include/linux/syscalls.h, marked as asmlinkage to match the way that system
> +calls are invoked:
> +
> + asmlinkage long sys_xyzzy(...);
> +
> +Some architectures (e.g. x86) have their own architecture-specific syscall
> +tables, but several other architectures share a generic syscall table. Add your
> +new system call to the generic list by adding an entry to the list in
> +include/uapi/asm-generic/unistd.h:
> +
> + #define __NR_xyzzy 292
> + __SYSCALL(__NR_xyzzy, sys_xyzzy)
> +
> +Also update the __NR_syscalls count to reflect the additional system call, and
> +note that if multiple new system calls are added in the same merge window,
> +your new syscall number may get adjusted to resolve conflicts.
> +
> +The file kernel/sys_ni.c provides a fallback stub implementation of each system
> +call, returning -ENOSYS. Add your new system call here too:
> +
> + cond_syscall(sys_xyzzy);
Please add something like this as well, to go along with the
cond_syscall bit:
Your new system call should be optional. Add a new Kconfig option
CONFIG_XYZZY_SYSCALL" (typically in init/Kconfig), possibly depending on
EXPERT, with a description of the new system call. Make the source
files implementing your system call depend on that new Kconfig option
(obj-$(CONFIG_XYZZY_SYSCALL) += xyzzy.c).
> +To summarize, you need a commit that includes:
> +
> + - SYSCALL_DEFINEn(xyzzy, ...) for the entry point
> + - corresponding prototype in include/linux/syscalls.h
> + - generic table entry in include/uapi/asm-generic/unistd.h
> + - fallback stub in kernel/sys_ni.c
- new Kconfig symbol, typically in init/Kconfig
> +Testing
> +-------
> +
> +A new system call should obviously be tested; it is also useful to provide
> +reviewers with a demonstration of how user space programs will use the system
> +call. A good way to combine these aims is to include a simple self-test
> +program in a new directory under tools/testing/selftests/.
> +
> +For a new system call, there will obviously be no libc wrapper function and so
> +the test will need to invoke it using syscall(); also, if the system call
> +involves a new userspace-visible structure, the corresponding header will need
> +to be installed to compile the test.
> +
> +Make sure the selftest runs successfully on all supported architectures. For
> +example, check that it works when compiled as an x86_64 (-m64), x86_32 (-m32)
> +and x32 (-mx32) ABI program.
"Make sure the kernel compiles when you configure out your new system
call."
> +Man Page
> +--------
> +
> +All new system calls should come with a complete man page, ideally using groff
> +markup, but plain text will do. If groff is used, it's helpful to include a
> +pre-rendered ASCII version of the man page in the cover email for the
> +patchset, for the convenience of reviewers.
> +
> +The man page should be cc'ed to linux-man@vger.kernel.org
> +For more details, see https://www.kernel.org/doc/man-pages/patches.html
> +
> +References and Sources
> +----------------------
> +
> + - LWN article from Michael Kerrisk on use of flags argument in system calls:
> + https://lwn.net/Articles/585415/
> + - LWN article from Michael Kerrisk on how to handle unknown flags in a system
> + call: https://lwn.net/Articles/588444/
> + - LWN article from Jake Edge describing constraints on 64-bit system call
> + arguments: https://lwn.net/Articles/311630/
> + - Pair of LWN articles from David Drysdale that describe the system call
> + implementation paths in detail for v3.14:
> + - https://lwn.net/Articles/604287/
> + - https://lwn.net/Articles/604515/
> + - Architecture-specific requirements for system calls are discussed in the
> + syscall(2) man-page:
> + http://man7.org/linux/man-pages/man2/syscall.2.html#NOTES
> + - Collated emails from Linus Torvalds discussing the problems with ioctl():
> + http://yarchive.net/comp/linux/ioctl.html
> + - "How to not invent kernel interfaces", Arnd Bergmann,
> + http://www.ukuug.org/events/linux2007/2007/papers/Bergmann.pdf
> + - LWN article from Michael Kerrisk on avoiding new uses of CAP_SYS_ADMIN:
> + https://lwn.net/Articles/486306/
> +
Nit: why is there a blank line here but not elsewhere in this list?
> + - Recommendation from Andrew Morton that all related information for a new
> + system call should come in the same email thread:
> + https://lkml.org/lkml/2014/7/24/641
> + - Recommendation from Michael Kerrisk that a new system call should come with
> + a man page: https://lkml.org/lkml/2014/6/13/309
> + - Suggestion from Thomas Gleixner that x86 wire-up should be in a separate
> + commit: https://lkml.org/lkml/2014/11/19/254
> + - Suggestion from Greg Kroah-Hartman that it's good for new system calls to
> + come with a man-page & selftest: https://lkml.org/lkml/2014/3/19/710
> + - Discussion from Michael Kerrisk of new system call vs. prctl(2) extension:
> + https://lkml.org/lkml/2014/6/3/411
> + - Numbering oddities arising from (re-)use of O_* numbering space flags:
> + - commit 75069f2b5bfb ("vfs: renumber FMODE_NONOTIFY and add to uniqueness
> + check")
> + - commit 12ed2e36c98a ("fanotify: FMODE_NONOTIFY and __O_SYNC in sparc
> + conflict")
> + - commit bb458c644a59 ("Safer ABI for O_TMPFILE")
> + - Discussion from Matthew Wilcox about restrictions on 64-bit arguments:
> + https://lkml.org/lkml/2008/12/12/187
> + - Recommendation from Greg Kroah-Hartman that unknown flags should be
> + policed: https://lkml.org/lkml/2014/7/17/577
> + - Recommendation from Linus Torvalds that x32 system calls should prefer
> + compatibility with 64-bit versions rather than 32-bit versions:
> + https://lkml.org/lkml/2011/8/31/244
> --
> 2.5.0.rc2.392.g76e840b
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Josh Triplett @ 2015-07-30 18:22 UTC (permalink / raw)
To: Ingo Molnar
Cc: David Drysdale, linux-api, Michael Kerrisk, Andrew Morton,
Arnd Bergmann, Shuah Khan, Jonathan Corbet, Eric B Munson,
Randy Dunlap, Andrea Arcangeli, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Oleg Nesterov, Linus Torvalds, Greg Kroah-Hartman,
Andy Lutomirski, Al Viro, Rusty Russell, Peter Zijlstra,
Vivek Goyal, Alexei Starovoitov, David Herrmann, Theodore
In-Reply-To: <20150730083831.GA22182@gmail.com>
On Thu, Jul 30, 2015 at 10:38:31AM +0200, Ingo Molnar wrote:
> When the system call is extended in the future on the kernel side, with 'u64
> param_4', then the structure expands from an old size of 24 to a new size of 32
> bytes. The following scenarios might occur:
>
> - the common case: new user-space calls the new kernel code, ->size is 32 on both
> sides.
>
> - old binaries might call the kernel with params->size == 24, in which case the
> kernel sets the new fields to 0. The new feature should be written
> accordingly, so that a value of 0 means the old behavior.
>
> - new binaries might run on old kernels, with params->size == 32. In this case
> the old kernel will check that all the new fields it does not know about are
> set to 0 - if they are nonzero (if the new feature is used) it returns with
> -ENOSYS or -EINVAL.
Nit: it seems easier, rather than having the kernel check this, to have
userspace only use the minimum version of the structure that contains
the features they use, and then have older kernels reject sizes bigger
than they understand. If you don't need param_4, don't use the version
of the structure that has param_4. That also means the kernel doesn't
need to copy and read those values. Either approach works, though.
- Josh Triplett
^ permalink raw reply
* Re: [PATCHv2 1/1] Documentation: describe how to add a system call
From: Kees Cook @ 2015-07-30 18:21 UTC (permalink / raw)
To: David Drysdale
Cc: Ingo Molnar, Linux API, Michael Kerrisk, Andrew Morton,
Arnd Bergmann, Shuah Khan, Jonathan Corbet, Eric B Munson,
Randy Dunlap, Andrea Arcangeli, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, Oleg Nesterov, Linus Torvalds, Greg Kroah-Hartman,
Andy Lutomirski, Al Viro, Rusty Russell, Peter Zijlstra,
Vivek Goyal, Alexei Starovoitov, David Herrmann
In-Reply-To: <CAHse=S-Wgv8JKu72FQaivou1PAwyexqjn_sjtMu3n=ezX0x-vg@mail.gmail.com>
On Thu, Jul 30, 2015 at 4:10 AM, David Drysdale <drysdale@google.com> wrote:
> On Thu, Jul 30, 2015 at 9:38 AM, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * David Drysdale <drysdale@google.com> wrote:
>>
>>> +Designing the API
>>> +-----------------
>>> +
>>> +A new system call forms part of the API of the kernel, and has to be supported
>>> +indefinitely. As such, it's a very good idea to explicitly discuss the
>>> +interface on the kernel mailing list, and to plan for future extensions of the
>>> +interface. In particular:
>>> +
>>> + **Include a flags argument for every new system call**
>>
>> Sorry, but I think that's bad avice, because even a 'flags' field is inflexible
>> and stupid in many cases - it fosters an 'ioctl' kind of design.
>>
>>> +The syscall table is littered with historical examples where this wasn't done,
>>> +together with the corresponding follow-up system calls (eventfd/eventfd2,
>>> +dup2/dup3, inotify_init/inotify_init1, pipe/pipe2, renameat/renameat2), so
>>> +learn from the history of the kernel and include a flags argument from the
>>> +start.
>>
>> The syscall table is also littered with system calls that have an argument space
>> considerably larger than what 6 parameters can express, where various 'flags' are
>> used to bring in different parts of new APIs, in a rather messy way.
>>
>> The right approach IMHO is to think about how extensible a system call is expected
>> to be, and to plan accordingly.
>>
>> If you are anywhere close to 6 parameters, you should not introduce 'flags' but
>> you should _reduce_ the number of parameters to a clean essential of 2 or 3
>> parameters and should shuffle parameters out to a separate 'parameters/attributes'
>> structure that is passed in by pointer:
>>
>> SYSCALL_DEFINE2(syscall, int, fd, struct params __user *, params);
>>
>> And it's the design of 'struct params' that determines future flexibility of the
>> interface. A very flexible approach is to not use flags but a 'size' argument:
>>
>> struct params {
>> u32 size;
>> u32 param_1;
>> u64 param_2;
>> u64 param_3;
>> };
>>
>> Where 'size' is set by user-space to the size of 'struct params' known to it at
>> build time:
>>
>> params->size = sizeof(*params);
>>
>> In the normal case the kernel will get param->size == sizeof(*params) as known to
>> the kernel.
>>
>> When the system call is extended in the future on the kernel side, with 'u64
>> param_4', then the structure expands from an old size of 24 to a new size of 32
>> bytes. The following scenarios might occur:
>>
>> - the common case: new user-space calls the new kernel code, ->size is 32 on both
>> sides.
>>
>> - old binaries might call the kernel with params->size == 24, in which case the
>> kernel sets the new fields to 0. The new feature should be written
>> accordingly, so that a value of 0 means the old behavior.
>>
>> - new binaries might run on old kernels, with params->size == 32. In this case
>> the old kernel will check that all the new fields it does not know about are
>> set to 0 - if they are nonzero (if the new feature is used) it returns with
>> -ENOSYS or -EINVAL.
>>
>> With this approach we have both backwards and forwards binary compatibility: new
>> binaries will run on old kernels just fine, even if they have ->size set to 32, as
>> long as they make use of the features.
>>
>> This design simplifies application design considerably: as new code can mostly
>> forget about old ABIs, there's no multiple versions to be taken care of, there's
>> just a single 'struct param' known to both sides, and there's no version skew.
>>
>> We are using such a design in perf_event_open(), see perf_copy_attr() in
>> kernel/events/core.c. And yes, ironically that system call still has a historic
>> 'flags' argument, but it's not used anymore for extension: we've made over 30
>> extensions to the ABI in the last 3 years, which would have been impossible with a
>> 'flags' approach.
>>
>> Thanks,
>>
>> Ingo
>
> Fair point, there are other ways to ensure extendability that just the
> simple flags
> approach -- and as you say, for more sophisticated interfaces flags might hinder
> more than they help.
>
> How about the attached text as an attempt to cover both bases?
>
> Thanks,
> David
>
>
> ------------------
>
> Designing the API: Planning for Extension
> -----------------------------------------
>
> A new system call forms part of the API of the kernel, and has to be supported
> indefinitely. As such, it's a very good idea to explicitly discuss the
> interface on the kernel mailing list, and it's important to plan for future
> extensions of the interface.
>
> (The syscall table is littered with historical examples where this wasn't done,
> together with the corresponding follow-up system calls -- eventfd/eventfd2,
> dup2/dup3, inotify_init/inotify_init1, pipe/pipe2, renameat/renameat2 -- so
> learn from the history of the kernel and plan for extensions from the start.)
>
> For simpler system calls that only take a couple of arguments, the preferred way
> to allow for future extensibility is to include a flags argument to the system
> call. To make sure that userspace programs can safely use flags between kernel
> versions, check whether the flags value holds any unknown flags, and reject the
> sycall (with EINVAL) if it does:
>
> if (flags & ~(THING_FLAG1 | THING_FLAG2 | THING_FLAG3))
> return -EINVAL;
>
> (If no flags values are used yet, check that the flags argument is zero.)
>
> For more sophisticated system calls that involve a larger number of arguments,
> it's preferred to encapsulate the majority of the arguments into a structure
> that is passed in by pointer. Such a structure can cope with future extension
> by including a size argument in the structure:
>
> struct xyzzy_params {
> u32 size; /* userspace sets p->size = sizeof(struct xyzzy_params) */
> u32 param_1;
> u64 param_2;
> u64 param_3;
> };
>
> As long as any subsequently added field, say param_4, is designed so that a zero
> value gives the previous behaviour, then this allows both directions of version
> mismatch:
>
> - To cope with a later userspace program calling an older kernel, the kernel
> code should check that any memory beyond the size of the structure that it
> expects is zero (effectively checking that param_4 == 0).
> - To cope with an older userspace program calling a newer kernel, the kernel
> code can zero-extend a smaller instance of the structure (effectively setting
> param_4 = 0).
>
> See perf_event_open(2) and the perf_copy_attr() function (in
> kernel/events/core.c) for an example of this approach.
I like this, it's a good description of both options. I'm still biased
about the approach: I prefer flags, since pointers to user structures
complicate syscall filtering. ;)
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH] kdbus/samples: skip on CROSS_COMPILE
From: Paul Gortmaker @ 2015-07-30 17:00 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: David Herrmann, Shuah Khan, linux-api, linux-kernel, Daniel Mack,
Djalal Harouni
In-Reply-To: <20150730163307.GA20048@kroah.com>
[Re: [PATCH] kdbus/samples: skip on CROSS_COMPILE] On 30/07/2015 (Thu 09:33) Greg Kroah-Hartman wrote:
> On Thu, Jul 30, 2015 at 06:13:25PM +0200, David Herrmann wrote:
> > Apparently we cannot rely on up-to-date kernel headers to be available
> > when cross-compiling, not even for HOSTCC. That's sad, but it's how it
> > is. Skip samples on cross-compiles as suggested by Paul, so allmodconfig
> > runs smoothly again.
> >
> > Tested-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> > ---
> > samples/kdbus/Makefile | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/samples/kdbus/Makefile b/samples/kdbus/Makefile
> > index 137f842..dbd9de8 100644
> > --- a/samples/kdbus/Makefile
> > +++ b/samples/kdbus/Makefile
> > @@ -1,9 +1,13 @@
> > # kbuild trick to avoid linker error. Can be omitted if a module is built.
> > obj- := dummy.o
> >
> > +ifndef CROSS_COMPILE
>
> This really feels like the wrong solution.
>
> > +
> > hostprogs-$(CONFIG_SAMPLE_KDBUS) += kdbus-workers
> >
> > always := $(hostprogs-y)
> >
> > HOSTCFLAGS_kdbus-workers.o += -I$(objtree)/usr/include
> > HOSTLOADLIBES_kdbus-workers := -lrt
> > +
> > +endif
>
> Shuah, how should we fix this "properly"? How is this resolved for the
> kernel test scripts, it should have the same issue that the samples do.
If you guys want to reproduce it, in order to figure out what mips does
with its headers that no other arch does, it should be as simple as:
wget https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/4.6.3/x86_64-gcc-4.6.3-nolibc_mips-linux.tar.xz
<untar somewhere>
<add resulting bin dir to your $PATH>
export CROSS_COMPILE=mips-linux-
export ARCH=mips
cd kernel-source
make allmodconfig
make -j16 samples/
Paul.
--
>
> thanks,
>
> greg k-h
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox