On Fri, Jun 5, 2015 at 9:36 AM, Mario Kleiner <mario.kleiner...@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-sta...@lists.freedesktop.org> > > Signed-off-by: Mario Kleiner <mario.kleiner...@gmail.com> > Cc: Ilia Mirkin <imir...@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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev