Re: [PATCHv2 1/1] Documentation: describe how to add a system call

[Josh Triplett <josh@joshtriplett.org>]

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