[Nouveau] [PATCH 9/9] dri2: Fix corner case crash for swaplimit > 1

[Anssi Hannula]

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