[Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.

[Mario Kleiner]

On 06/05/2015 03:50 PM, Ilia Mirkin wrote:
> This scheme is copied from radeon, does it need a similar fix? I'm away
> from computers for another week or so, will be able to look then.
>

For some reason, no. Testing on Radeon multi-x-screen ZaphodHeads never 
showed such problems over multiple years and many mesa versions. 
Apparently it does something slightly different, although i saw the 
hashing logic with fstat() is the same. Maybe correct by slightly 
different design, or maybe we just get lucky because the same fd's 
aren't recycled on successive glXQueryVersion() calls?

thanks,
-mario


> On Jun 5, 2015 4:37 PM, "Mario Kleiner" <mario.kleiner.de at gmail.com
> <mailto:mario.kleiner.de at gmail.com>> wrote:
>
>     The dup'ed fd owned by the nouveau_screen for a device node
>     must also be used as key for the winsys hash table, instead
>     of using the original fd passed in for a screen, to make
>     multi-x-screen ZaphodHeads configurations work on nouveau.
>
>     This prevents the following crash scenario that was observed
>     when a dynamically loaded rendering plugin used OpenGL on a
>     ZaphodHeads setup, e.g., on a dual x-screen setup. At first
>     load the plugin worked, but after unloading and reloading it,
>     the next rendering operation crashed:
>
>     1. Client, e.g., a plugin, calls glXQueryVersion.
>
>     2. DRI screens for the x-screens 0 and 1 are created, one shared
>         nouveau_screen is created for the shared device node of both
>         screens, but the original fd of x-screen 0 is used as identifying
>         key in the hash table, instead of the dup()ed fd of x-screen 0
>         which is owned by the nouveau_screen. nouveau_screen's refcount
>         is now 2.
>
>     3. Regular rendering happens by the client plugin, then the plugin
>         gets unloaded.
>
>     4. XCloseDisplay(). x-screen 0 gets its DRI screen destroyed,
>         nouveau_drm_screen_unref() drops the refcount to 1, calling mesa
>         code then closes the fd of x-screen 0, so now the fd which is
>         used as key in the hash table is invalid. x-screen 1 gets
>         destroyed, nouveau_drm_screen_unref() drops the refcount to 0,
>         the nouveau_screen gets destroyed, but removal of its entry
>         in the hash table fails, because the invalid fd in the hash
>         table no longer matches anything (fstat() on the fd is used
>         for hashing and key comparison, but fstat() on an already closed
>         fd fails and returns bogus results). x-screen 1 closes its fd.
>
>         Now all fd's are closed, the nouveau_screen destroyed, but
>         there is a dangling reference to the nouveau_screen in the
>         hash table.
>
>     5. Some OpenGL client plugin gets loaded again and calls
>         glXQueryVersion. Step 2 above repeats, but because a
>         dangling reference with a matching fd is found in the winsys
>         hash table, no new nouveau_screen is created this time. Instead
>         the invalid pointer to the old nouveau_screen is recycled,
>         which points to nirvana -> Crash.
>
>     This problem is avoided by use of the dup()ed fd which is
>     owned by the nouveau_screen and has the same lifetime as
>     the nouveau_screen itself.
>
>     Cc: "10.3 10.4 10.5 10.6" <mesa-stable at lists.freedesktop.org
>     <mailto:mesa-stable at lists.freedesktop.org>>
>
>     Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com
>     <mailto:mario.kleiner.de at gmail.com>>
>     Cc: Ilia Mirkin <imirkin at alum.mit.edu <mailto:imirkin at alum.mit.edu>>
>     ---
>       src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c | 3 ++-
>       1 file changed, 2 insertions(+), 1 deletion(-)
>
>     diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>     b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>     index 0635246..dbc3cae 100644
>     --- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>     +++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>     @@ -120,7 +120,8 @@ nouveau_drm_screen_create(int fd)
>              if (!screen)
>                      goto err;
>
>     -       util_hash_table_set(fd_tab, intptr_to_pointer(fd), screen);
>     +       /* Use dupfd in hash table, to avoid crashes in ZaphodHeads
>     configs */
>     +       util_hash_table_set(fd_tab, intptr_to_pointer(dupfd), screen);
>              screen->refcount = 1;
>              pipe_mutex_unlock(nouveau_screen_mutex);
>              return &screen->base;
>     --
>     2.1.4
>