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

[Mario Kleiner]

On 06/05/2015 03:57 PM, Mario Kleiner wrote:
> 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
>

And to contradict myself, yes, it needs a similar fix for radeon, as 
both code inspection and testing showed. Not sure why this (seemed?) to 
work without problems for me for a long time, but at least on Mesa 10.5+ 
it crashes nicely in the expected way.

I'll send out a patch in the next minute which fixes the problem, as 
tested on a triple-display Radeon HD-5770 setup with different single 
x-screen and multi-x-screen ZaphodHeads configs.

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
>>