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

[Ilia Mirkin]

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;
    fstat(fd, &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;
    fstat(fd1, &stat1);
    fstat(fd2, &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;
>         pipe_mutex_unlock(nouveau_screen_mutex);
>         return &screen->base;
> --
> 2.1.4
>