[Nouveau] [PATCH 9/9] dri2: Fix corner case crash for swaplimit > 1
Anssi Hannula
anssi.hannula at iki.fi
Thu Jul 12 12:39:16 PDT 2012
Hi!
On 16.02.2012 02:45, Mario Kleiner wrote:
> If a swaplimit > 1 is set on a server which
> supports the swaplimit api (XOrg 1.12.0+),
> the following can happen:
>
> 1. Client calls glXSwapBuffersMscOML() with a
> swap target > 1 vblank in the future, or a
> client calls glXSwapbuffers() while the swap
> interval is set to > 1 (unusual but possible).
>
> 2. nouveau_dri2_finish_swap() is therefore called
> only at the target vblank, instead of immediately.
>
> 3. Because of the deferred execution of
> nouveu_dri2_finish_swap(), the OpenGL client
> can call x-servers DRI2GetBuffersWithFormat()
> before nouveau_dri2_finish_swap() executes and
> it deletes pixmaps that would be needed by
> nouveau_dri2_finish_swap() --> Segfault --> Crash.
>
> Prevent this: When a swap is scheduled into the
> future, we temporarily reduce the swaplimit to 1
> until nouveau_dri2_finish_swap() is done, then
> restore it to its original value. This throttles
> the client inside the server in DRI2ThrottleClient()
> before it can call the evil DRI2GetbuffersWithFormat().
>
> The client will still be released one video refresh
> interval before swap completion, so there is still
> some potential win.
>
> This doesn't affect the common case of swapping at
> the next vblank, where this throttling is not needed
> or done.
After upgrading my system to X server 1.12.3, I've started to
experience some crashes/freezes related to apparent memory
corruption.
Debugging with valgrind and gdb, it seems to me like the "evil"
DRI2GetbuffersWithFormat() can be called before swap even when
target_msc == current_msc + 1. This results in writes+reads to
freed memory when the vblank event is finally being handled.
I'm not an Xorg/drm expert, but with a glance at the code I
didn't see anything that would/should prevent
DRI2GetbuffersWithFormat() from being called before the vblank
event is handled. Even if the event is immediately sent by the
kernel, isn't it possible that the X server services some other
requests before the event is dispatched?
Am I missing something, or is that a bug?
In any case, I'm running ddx 1.0.1, xserver 1.12.3, libdrm 2.4.37,
kernel 3.4.4, and I have Option "GLXVBlank" "true".
Here's one of the invalid write errors:
==24537== Invalid write of size 4
==24537== at 0x8D45029: nouveau_bo_name_get (nouveau.c:426)
==24537== by 0x8B1EFFD: nouveau_dri2_finish_swap
(nouveau_dri2.c:173)
==24537== by 0x8B1F65F: nouveau_dri2_vblank_handler
(nouveau_dri2.c:598)
==24537== by 0x8707BA0: drmHandleEvent (xf86drmMode.c:808)
==24537== by 0x8B37BC5: drmmode_wakeup_handler
(drmmode_display.c:1447)
==24537== by 0x43EA15: WakeupHandler (dixutils.c:421)
==24537== by 0x5D7429: WaitForSomething (WaitFor.c:224)
==24537== by 0x430B8B: Dispatch (dispatch.c:357)
==24537== by 0x421D6C: main (main.c:288)
==24537== Address 0xdd7c6d4 is 4 bytes inside a block of size 40
free'd
==24537== at 0x4C25A9E: free (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==24537== by 0x890E5A4: update_dri2_drawable_buffers (dri2.c:415)
==24537== by 0x890E97D: do_get_buffers (dri2.c:525)
==24537== by 0x890EB60: DRI2GetBuffersWithFormat (dri2.c:582)
==24537== by 0x8910A7E: ProcDRI2GetBuffersWithFormat (dri2ext.c:299)
==24537== by 0x8911256: ProcDRI2Dispatch (dri2ext.c:564)
==24537== by 0x430DDF: Dispatch (dispatch.c:428)
==24537== by 0x421D6C: main (main.c:288)
I stored some additional data regarding the vblank event request in
nouveau_dri2_vblank_state, so that I could retrieve the information
at invalid write time:
(gdb) print *(struct nouveau_dri2_vblank_state*) event_data
$1 = {action = SWAP, client = 0x7740b20, draw = 37748775, dst =
0xdd7c6d0, src = 0xbd1f5a0, func = 0x8910bf2 <DRI2SwapEvent>, data =
0x99247b0, frame = 302185, current_msc = 302184, target_msc = 302185,
remainder = 0, divisor = 0, hit = 0}
As you can see, in this event target_msc was current_msc + 1, but
still DRI2GetBuffersWithFormat() apparently managed to get called
first (and since swap limit was not altered, the call was not
throttled).
> Signed-off-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
> ---
> src/nouveau_dri2.c | 26 ++++++++++++++++++++++++++
> 1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
> index f0c7fec..7878a5a 100644
> --- a/src/nouveau_dri2.c
> +++ b/src/nouveau_dri2.c
> @@ -445,6 +445,26 @@ nouveau_dri2_schedule_swap(ClientPtr client,
> DrawablePtr draw,
> if (*target_msc == 0)
> *target_msc = 1;
>
> +#if DRI2INFOREC_VERSION >= 6
> + /* Is this a swap in the future, ie. the vblank event will
> + * not be immediately dispatched, but only at a future vblank?
> + * If so, we need to temporarily lower the swaplimit to 1, so
> + * that DRI2GetBuffersWithFormat() requests from the client get
> + * deferred in the x-server until the vblank event has been
> + * dispatched to us and nouveau_dri2_finish_swap() is done. If
> + * we wouldn't do this, DRI2GetBuffersWithFormat() would operate
> + * on wrong (pre-swap) buffers, and cause a segfault later on in
> + * nouveau_dri2_finish_swap(). Our vblank event handler restores
> + * the old swaplimit immediately after nouveau_dri2_finish_swap()
> + * is done, so we still get 1 video refresh cycle worth of
> + * triple-buffering. For a swap at next vblank, dispatch of the
> + * vblank event happens immediately, so there isn't any need
> + * for this lowered swaplimit.
> + */
> + if (current_msc < *target_msc - 1)
> + DRI2SwapLimit(draw, 1);
> +#endif
> +
> /* Request a vblank event one frame before the target */
> ret = nouveau_wait_vblank(draw, DRM_VBLANK_ABSOLUTE |
> DRM_VBLANK_EVENT,
> @@ -557,6 +577,12 @@ nouveau_dri2_vblank_handler(int fd, unsigned int
> frame,
> switch (s->action) {
> case SWAP:
> nouveau_dri2_finish_swap(draw, frame, tv_sec, tv_usec, s);
> +#if DRI2INFOREC_VERSION >= 6
> + /* Restore real swap limit on drawable, now that it is safe. */
> + ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
> + DRI2SwapLimit(draw, NVPTR(scrn)->swap_limit);
> +#endif
> +
> break;
>
> case WAIT:
--
Anssi Hannula
More information about the xorg-devel
mailing list