Bug 108762

Summary:

Screen-specific privates can not be used in CloseScreen

Product:

xorg

Reporter:

Michal Srb <michalsrb>

Component:

Server/General

Assignee:

Xorg Project Team <xorg-team>

Status:

RESOLVED MOVED

QA Contact:

Xorg Project Team <xorg-team>

Severity:

normal

Priority:

medium

CC:

anjala.thulasiram

Version:

git

Hardware:

Other

OS:

All

Whiteboard:

i915 platform:

i915 features:

Attachments:

Description	Flags
dix: Track DevPrivateKey initialization by generation	none
dix: Ignore initialized flag for screen-specific privates	none

Description Michal Srb 2018-11-15 15:49:36 UTC

I have a setup with nouveau driver, two monitors - one normal, one rotated. When the X server does the internal restart after last client disconnected, it aborts with assertion failure:

  dixGetPrivateAddr: Assertion `key->initialized' failed.

Backtrace:
  #0  0x00007ffff57fb24b in raise () from /lib64/libc.so.6
  #1  0x00007ffff57e44f1 in abort () from /lib64/libc.so.6
  #2  0x00007ffff57e43c1 in __assert_fail_base.cold.0 () from /lib64/libc.so.6
  #3  0x00007ffff57f3832 in __assert_fail () from /lib64/libc.so.6
  #4  0x00007ffff2070c89 in dixGetPrivateAddr (privates=0x5555566665e0, key=0x555556292c10) at ../include/privates.h:121
  #5  0x00007ffff2071647 in exaDestroyPixmap_mixed (pPixmap=0x5555566665c0) at exa_mixed.c:253
  #6  0x00005555556f5a6e in damageDestroyPixmap (pPixmap=0x5555566665c0) at damage.c:1504
  #7  0x000055555568e93b in XvDestroyPixmap (pPix=0x5555566665c0) at xvmain.c:369
  #8  0x00005555555d6ab7 in FreeScratchPixmapHeader (pPixmap=0x5555566665c0) at pixmap.c:82
  #9  0x00007ffff24c6f1f in drmmode_crtc_shadow_destroy (crtc=0x555555ab7e30, rotate_pixmap=0x5555566665c0, data=0x7fffe71ef000) at drmmode_display.c:659
  #10 0x0000555555662e01 in xf86RotateDestroy (crtc=0x555555ab7e30) at xf86Rotate.c:252
  #11 0x0000555555662ff5 in xf86RotateCloseScreen (screen=0x555555b09840) at xf86Rotate.c:304
  #12 0x000055555564eef4 in xf86CrtcCloseScreen (screen=0x555555b09840) at xf86Crtc.c:781
  #13 0x0000555555604f0f in DGACloseScreen (pScreen=0x555555b09840) at xf86DGA.c:288
  #14 0x0000555555614695 in CMapCloseScreen (pScreen=0x555555b09840) at xf86cmap.c:250
  #15 0x000055555567888c in CursorCloseScreen (pScreen=0x555555b09840) at cursor.c:205
  #16 0x00005555556e5c88 in AnimCurCloseScreen (pScreen=0x555555b09840) at animcur.c:100
  #17 0x000055555567344a in compCloseScreen (pScreen=0x555555b09840) at compinit.c:86
  #18 0x00007ffff33f998f in glxCloseScreen (pScreen=0x555555b09840) at glxscreens.c:171
  #19 0x00005555555b3764 in dix_main (argc=2, argv=0x7fffffffe058, envp=0x7fffffffe070) at main.c:329
  #20 0x0000555555593ff2 in main (argc=2, argv=0x7fffffffe058, envp=0x7fffffffe070) at stubmain.c:34

Nouveau is destroying its rotate_pixmap during the CloseScreen call, which leads to EXA trying to retrieve its screen-specific private data from the pixmap, but that triggers the assert because the "initialized" flag on the ExaScreenPrivRec::pixmapPrivateKeyRec key is FALSE.

The key was properly initialized, but the flag was then set back to FALSE by dixFreeScreenSpecificPrivates function just *before* the CloseScreen call.

This behavior was introduced by commit 82eb490b "privates: Clear screen-specific keys during CloseScreen". The reasons why it should be called before CloseScreen is described in here:
https://patchwork.freedesktop.org/patch/59716/ and
https://patchwork.freedesktop.org/patch/59807/

In short: The initialized flag must be set to FALSE so that the key is registered again after the restart. It can not be done after CloseScreen, because some of the registered keys may be deallocated in CloseScreen. So it is done before, which however means that no screen-specific privates can be accessed in CloseScreen. The above example with nouveau and its rotate_pixmap shows that sometimes it is needed. I expect that there are more situations when it is needed - there are few older bugs that mention the same assert.

I think proper fix is to track the initialized state bit differently. I'll attach two possible solutions.

Comment 1 Michal Srb 2018-11-15 16:07:59 UTC

Created attachment 142480 [details] [review]
dix: Track DevPrivateKey initialization by generation

Here is first possible solution. It changes the "initialized" Bool into "initializedInGeneration" counter. It is initialized to zero and set to the current serverGeneration when the key is registered. A key is considered initialized if its initializedInGeneration counter equals the current serverGeneration. That way no explicit resetting is needed and all keys can be used until the very end. Those that are still alive at the restart become automatically uninitialized.

One issue is that it changes API because the structure is exposed in the privates.h file and also ABI because it is accessed by some of the inline functions in that file.

The size of the struct _DevPrivateKeyRec remains the same because the counter is unsigned int and the flag used to be Bool=int.

Comment 2 Michal Srb 2018-11-15 16:13:22 UTC

Created attachment 142481 [details] [review]
dix: Ignore initialized flag for screen-specific privates

Here is second possible solution. In this case the "initialized" flag is never set to FALSE for the screen-specific keys, so they can be used even inside CloseScreen function. The dixRegisterScreenSpecificPrivateKey does not use it to determine if a key was already registered, instead it searches the list of already registered keys.

I think this does not break any API or ABI.

Comment 3 GitLab Migration User 2018-12-17 17:34:29 UTC

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/xserver/issues/615.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.