[Mesa-dev] [PATCH] nouveau: Use dup fd as key in drm-winsys hash table to fix ZaphodHeads.
imirkin at alum.mit.edu
Sat Jun 27 18:48:44 PDT 2015
On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner
<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.
See below, but it shouldn't matter which fd gets used.
> 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.
I need to think about this some more, but... this shouldn't happen :)
In fact, the whole dupfd thing was added there for ZaphodHeads screens
in the first place. See
https://bugs.freedesktop.org/show_bug.cgi?id=79823 and commit
a59f2bb17bcc which fixed it.
Note that the hash has the following hash/eq functions:
static unsigned hash_fd(void *key)
int fd = pointer_to_intptr(key);
struct stat stat;
return stat.st_dev ^ stat.st_ino ^ stat.st_rdev;
static int compare_fd(void *key1, void *key2)
int fd1 = pointer_to_intptr(key1);
int fd2 = pointer_to_intptr(key2);
struct stat stat1, stat2;
return stat1.st_dev != stat2.st_dev ||
stat1.st_ino != stat2.st_ino ||
stat1.st_rdev != stat2.st_rdev;
so fd and dupfd should get hashed to the same thing. I suspect there's
something else going on in your application...
> Cc: "10.3 10.4 10.5 10.6" <mesa-stable at lists.freedesktop.org>
> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> Cc: Ilia Mirkin <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;
> return &screen->base;
More information about the mesa-dev