Re: [PATCH v3] getrandom system call wrapper [BZ #17252]

[Carlos O'Donell <carlos at redhat dot com>]

On 09/09/2016 10:21 AM, Torvald Riegel wrote:
> On Thu, 2016-09-08 at 13:44 +0200, Florian Weimer wrote:
>> I have made the system call wrapper a cancellation point.  (If we 
>> implement the simpler getentropy interface, it would not be a 
>> cancellation point.)
> 
> Why did you do that?  Even though the system call is new, and thus can't
> have been used in existing code directly, making it a cancellation point
> will make all callers cancellation points too.  Therefore, for example,
> we couldn't use it in the implementation of any POSIX functions (that
> are not cancellation points) in glibc without having to disable and
> restore the cancellation state around it every time.
> It might be even more convenient to have one wrapper that is a
> cancellation point and one that is not.
> 
> Can't we just let cancellation rot in its corner?
 
I will second what Florian has suggested, I also think getrandom() should
be a cancellation point.

* I see getrandom() as more like a read() and read() is cancellation point.
  The argument that the functions can block indefinitely makes them prime
  candidates for cancellation. The fact that rwlock-related, or in fact all
  pthread-related functions are not cancellation points is not a great
  comparison because threads are sufficiently complex that their cancellation
  is generally a combination of explicit cancellation checks and complex cancel
  handlers rather than pthread routines being cancellation points (too simple a model).

* Application authors that do not want to think about cancellation should disable it
  e.g. pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL), and libraries that
  enable it should only cancel _their own_ threads which are cancellation aware.
  This speaks broadly to Torvald's concerns about user code being canceled by
  accident. I would much rather see glibc set cancellation to disabled by default
  instead of the POSIX mandated 'enabled and deferred' by default, and document
  the difference from POSIX (the gun should be unloaded and in a locked box).

* Internal use of a non-cancellating version is done easily by calling
  __getrandom_nocancel() (As noted by Florian).

* It is true that making it a cancellation point will make all callers
  a cancellation point, but that's already true for much more common
  cancellation points like read() and write() and printf().

* Making it a cancellation point that is dependent on input flags e.g.
  non-blocking, will complicate the simple wrapper and make it more
  difficult to explain to users since it can no longer go on a simple
  list of cancellation points.

* Sun OS 5.11 (Open Indiana) passes the glibc tst-getrandom regression test.
  The getrandom() call in OI is _not_ a cancellation point, and verified with
  a test case. It appears to be a very small wrapper around a kernel getrandom
  syscall (disassembly of libc.so.1). Even if I think this is wrong, it means
  we might see problems porting from Solaris to Linux, but only in the intersection
  of applications that use cancellation and getrandom. I also note that getentropy
  seems built upon getrandom (disassembly shows that).

-- 
Cheers,
Carlos.