From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anssi Hannula Subject: Re: [PATCH 9/9] dri2: Fix corner case crash for swaplimit > 1 Date: Mon, 23 Jul 2012 20:19:31 +0300 Message-ID: <500D87A3.1090308@iki.fi> References: <1329349524-11650-1-git-send-email-mario.kleiner@tuebingen.mpg.de> <1329349524-11650-10-git-send-email-mario.kleiner@tuebingen.mpg.de> <0d082daa8340ab694169e75ff2c3044c@mail.onse.fi> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <0d082daa8340ab694169e75ff2c3044c-NuFIJhXzKCMOpIWgD9kOMw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: nouveau-bounces+gcfxn-nouveau=m.gmane.org-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Errors-To: nouveau-bounces+gcfxn-nouveau=m.gmane.org-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org To: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Cc: xorg-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, Mario Kleiner List-Id: nouveau.vger.kernel.org 12.07.2012 22:39, Anssi Hannula kirjoitti: > 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 , 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). I've workarounded this locally for now with: Option "SwapLimit" "1" But I guess something should still be done about this memory corruption issue... > >> Signed-off-by: Mario Kleiner >> --- >> 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