Re: [PATCH] events: add a flag to perf_event_open() to set O_CLOEXEC

[Yann Droneaud <ydroneaud@opteya.com>]

Hi,

Le jeudi 31 octobre 2013 à 19:12 +0100, Peter Zijlstra a écrit :
> On Wed, Oct 30, 2013 at 10:35:50PM +0100, Yann Droneaud wrote:
> > This patch adds PERF_FLAG_FD_CLOEXEC flag for
> > perf_event_open() syscall.
> > 
> > perf_event_open() creates a new file descriptor,
> > but unlike open() syscall, it lack a flag to atomicaly
> > set close-on-exec (O_CLOEXEC).
> > 
> > Not using O_CLOEXEC by default and not letting userspace
> > provide the "open" flags should be avoided: in most case
> > O_CLOEXEC must be used to not leak file descriptor across
> > exec().
> > 
> > Using O_CLOEXEC when creating a file descriptor allows
> > userspace to set latter, using fcntl(), without any risk
> > of race, if the file descriptor is going to be inherited
> > or not across exec().
> > 
> > Link: http://lkml.kernel.org/r/94b641a81a06ba4943cf77e80bc271c8@meuh.org
> > Link: http://lkml.kernel.org/r/cover.1383121137.git.ydroneaud@opteya.com
> > Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
> > ---
> > 
> > Hi Peter,
> > 
> > This patch should replaces
> > "[PATCH v4 6/7] events: use get_unused_fd_flags(0) instead of get_unused_fd()"
> > 
> > Please have a look.
> 
> I'm still terminally confused as to all of this... Why does it matter
> what the default is if you can change it with fcntl() ?

There's a possible race between open() and fcntl()in multi-threaded
program, that's why O_CLOEXEC was added to open(), and then added to
many other system calls.

>  Also, how can you tell nobody relies on the current behaviour and its therefore safe
> to change?
> 

If *you* cannot tell, then no one could tell, thus the default *must*
not be changed because no one could ever be sure that no application
rely on the current behavior.

But it's rather pointless to change the default, since the caller of
get_unused_fd(), eg. syscall perf_event_open(), has a flags argument
that can be used to ask for O_CLOEXEC. Just like other syscall extended
to support O_CLOEXEC.

You might want to read "Secure File Descriptor Handling" by 
Ulrich Drepper [1] who is responsible to adding O_CLOEXEC on open,
and probably other syscalls

[1] http://udrepper.livejournal.com/20407.html

Regards.

-- 
Yann Droneaud
OPTEYA