[Mesa-dev] [PATCH] egl/drm: Try to use CLOEXEC for drm fds

[Emil Velikov]

On 12/06/15 19:05, Derek Foreman wrote:
> On 12/06/15 11:29 AM, Emil Velikov wrote:
>> Hi Derek,
> 
> Hi, thanks for looking at this. :)
> 
Props goes to Matt, for the reminder. It kind of fell of my radar.

>> On 1 May 2015 at 18:34, Derek Foreman <derekf at osg.samsung.com> wrote:
>>> These fds can propagate to child processes if we don't set CLOEXEC,
>>> so make a best effort to do that.
>>>
>>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>>> ---
>>> Noticed this when fixing up similar problems in weston - weston's
>>> drm fd gets dup()ed here and loses CLOEXEC and ends up in every child
>>> process the shell launches.
>>>
>>>  src/egl/drivers/dri2/platform_drm.c | 14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
>>> index 486b003..c326d6c 100644
>>> --- a/src/egl/drivers/dri2/platform_drm.c
>>> +++ b/src/egl/drivers/dri2/platform_drm.c
>>> @@ -596,7 +596,11 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>>>     struct dri2_egl_display *dri2_dpy;
>>>     struct gbm_device *gbm;
>>>     int fd = -1;
>>> -   int i;
>>> +   int i, flags = O_RDWR;
>>> +
>>> +#ifdef O_CLOEXEC
>>> +   flags |= O_CLOEXEC;
>>> +#endif
>>>
>>>     loader_set_logger(_eglLog);
>>>
>>> @@ -611,9 +615,9 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>>>        char buf[64];
>>>        int n = snprintf(buf, sizeof(buf), DRM_DEV_NAME, DRM_DIR_NAME, 0);
>>>        if (n != -1 && n < sizeof(buf))
>>> -         fd = open(buf, O_RDWR);
>>> +         fd = open(buf, flags);
>>>        if (fd < 0)
>>> -         fd = open("/dev/dri/card0", O_RDWR);
>>> +         fd = open("/dev/dri/card0", flags);
>>>        dri2_dpy->own_device = 1;
>>>        gbm = gbm_create_device(fd);
>>>        if (gbm == NULL)
>>> @@ -639,6 +643,10 @@ dri2_initialize_drm(_EGLDriver *drv, _EGLDisplay *disp)
>>>        }
>>>     }
>>>
>>> +   flags = fcntl(dri2_dpy->fd, F_GETFD);
>>> +   if (flags >= 0 && !(flags & FD_CLOEXEC))
>>> +      fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>>> +
>> In other places in mesa we explicitly check for EINVAL if open()
>> fails, and use that as indication that O_CLOEXEC is not supported
>> (note that we only use the O_RDWR and O_CLOEXEC flags). Curious which
>> would be the better approach to use through mesa ?
> 
> Oops - I think that way is better.
> 
> There's a simple function in loader.c called drm_open_device() that does
> the open that way, could we just rename that to loader_open_device() and
> make it non-static and call it from dri2_initialize_drm()?
> 
That's exactly what I was thinking actually :-)

> That said...
> 
> Unfortunately, it appears my previous testing was insufficient and
> there's still a leaked fd - dup() creates fds without CLOEXEC set.
> 
Seems that we ignore that in a few places through mesa - how about we
replace dup() + fcntl(F_SETFD, fcntl(F_GETFD)|FD_CLOEXEC) with
fcntl(F_DUPFD_CLOEXEC) ?

Its requirements are almost the same as O_CLOEXEC (kernel 2.6.{23,24}
and glibc 2.7) as opposed to dup2/dup3 which requires glibc 2.9 (not
sure about the kernel).

Cheers,
Emil