On 06/28/2015 03:48 AM, Ilia Mirkin wrote:
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...
My application is a set of dynamically loaded plugins running inside another
host app (Psychtoolbox-3 inside GNU/Octave), so what happens often is that
the OpenGL using plugin gets unloaded and reloaded at runtime, so a after a
OpenGL session has ended with a XCloseDisplay() tearing the winsys down, you
can easily have it restart at plugin reload with another XOpenDisplay ->
glXQueryVersion -> .... sequence, where the new call to glXQueryVersion will
trigger a recreation of the winsys, which will find the stale entry in the
hash table pointing to nowhere instead of the previously released
nouveau_screen -> use-after-free -> boom!
The reason this fails is because during destruction of the 2nd, 3rd etc.
x-screen, the already closed fd associated with the 1st x-screen is fed into
the compare_fd function, so fstat() errors out on the invalid fd. I added
printf's etc. on both nouveau and now radeon to verify the fstat gives me
EBADF errors. So the hash calculation goes wrong when trying to find the
matching element in the hash table with a fd that has a matching hash -> The
element which should be removed is ignored/not removed because it contains
an already closed fd for which no proper hash can be calculated anymore ->
hash comparison during search goes wrong.
This is because multiple x-screens, e.g., 2 x-screens are destroyed in order
0, 1 by FreeScreenConfigs() as part of XCloseDisplay(). FreeScreenConfigs()
calls dri2DestroyScreen() in dri2.c or dri3_destroy_screen in dri3.c, which
are essentially identical, e.g.,:
static void
dri2DestroyScreen(struct glx_screen *base)
{
struct dri2_screen *psc = (struct dri2_screen *) base;
/* Free the direct rendering per screen data */
(*psc->core->destroyScreen) (psc->driScreen);
--> This core->destroyScreen calls eventually into the winsys, which then
drops the refcount of the nouveau_screen or radeon winsys structure by one,
and tries to release the struct and remove the hash table entry once the
refcount drops to zero.
driDestroyConfigs(psc->driver_configs);
close(psc->fd);
-> This close() closes the fd of a screen immediately after closing down
that screen. At the time screen 1 is closed down, the fd associated with
screen 0 is already closed.
free(psc);
}
So you have this sequence during creation:
glXQueryVersion() -> AllocAndFetchScreenConfigs():
create screen 0 and its fd, e.g., fd==5, dup(fd), e.g., fd==6 for the
nouveau_screen, but store x-screen 0's fd==5 *itself* in the hash table as
key.
create screen 1. This now creates an fd 7
=> refcount is now 2.
And this sequence during destruction at XCloseDisplay();
FreeScreenConfigs():
free screen 0: core->destroyScreen() -> ... -> nouveau_drm_screen_unref() =>
refcount-- is now 1
close(fd==5) of screen 0 and thereby the fd==5 stored inside the hash table
as key. From now on using fcntl(fd==5) for hash calculation on that element
will malfunction.
free screen 1: -> nouveau_drm_screen_unref -> refcount-- is now 0. -> Try to
remove hash table entry => fails because fcntl(fd==5) on the already closed
fd==5 of screen 0 fails with EBADF. => Hash table keeps its stale element
referencing the struct nouveau_screen.
Free nouveau_screen struct (close dup()ed fd==6 for nouveau_screen, close
down stuff, free the struct).
-> close(fd==7) associated with screen 1.
Now all fd's (5,6,7) are closed, the struct is gone, the hash table contains
a stale element with a no longer existent fd==5 and a pointer to an already
free()d struct nouveau_screen.
Now we reload the plugin and do glXQueryVersion() again, so
AllocAndFetchScreenConfigs() is executed again to create the winsys etc. for
screens 0 and 1.
create screen 0 and its fd: Here the first unused fd in the filedescriptor
table is recycled, which happens to be fd==5, just as in the first session
with the plugin! Now suddenly fd=5 is again the valid fd for x-screen 0, and
it points to the same /dev/drm/card0 device file as before. That means when
nouveau_drm_screen_create(fd==5) is called, it finds the stale element in
the hash table from the previous session, because the previously closed fd 5
in that element is now valid and open again and points to the same device
file as in the previous session, ergo has the same hash etc.
nouveau_drm_screen_create() concludes that a fully initialized
nouveau_screen already exists for this session, except it doesn't exist -
the pointer in the hash table points to invalid previously freed memory.
When it tries to access that memory we have a use-after-free and the
application segfaults.
So it depends a bit on what happens in the applications memory between the
two consecutive OpenGL sessions etc. how long it takes for an invalid memory
access to happen, but eventually it hits invalid data. Maybe occassionally
it gets lucky and the freed memory is still accessible and the struct
intact, so stuff works by chance.
If the application happened to open other files inbetween the 1st and 2nd
session, then the relevant fd in the 2nd session may not be exactly the same
as in the first session, because recycling that fd didn't work, so we are
only left with a dead but harmless entry in the hash table instead of a
crasher. I assume something like that prevented my crashes on radeon in the
past, because when i wasn't expecting/looking for trouble i didn't use a
test sequence carefully made to trigger the bug for certain.
Similar things happen on radeon for the same reason, ergo the 2nd patch with
a similar fix.
I don't have the gdb backtrace for nouveau anymore, but the traces for
radeon are attached to this mail to show you the flow of execution, similar
to nouveau.
-mario