Bug 106994 - [PATCH] Fix null pointer dereference in drmmode_output_dpms
Summary: [PATCH] Fix null pointer dereference in drmmode_output_dpms
Status: NEW
Alias: None
Product: xorg
Classification: Unclassified
Component: Driver/nouveau (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium major
Assignee: Nouveau Project
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-21 14:54 UTC by John Lindgren
Modified: 2018-10-09 04:50 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Fix null pointer dereference in drmmode_output_dpms (906 bytes, patch)
2018-06-21 14:54 UTC, John Lindgren
no flags Details | Splinter Review
X.org log (62.75 KB, text/plain)
2018-06-21 16:35 UTC, John Lindgren
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Lindgren 2018-06-21 14:54:40 UTC
Created attachment 140263 [details] [review]
Fix null pointer dereference in drmmode_output_dpms

This fixes an Xorg crash when a Lenovo P50 laptop is undocked, disconnecting it from two external DisplayPort monitors.

Checking for koutput == NULL has precedent in drmmode_output_get_modes.
Comment 1 John Lindgren 2018-06-21 14:55:59 UTC
Full backtrace:

Thread 1 "Xorg" received signal SIGSEGV, Segmentation fault.
drmmode_output_dpms (output=0x558b2dab8ee0, mode=3) at ../../src/drmmode_display.c:921
921	../../src/drmmode_display.c: No such file or directory.
#0  0x00007f386322a759 in drmmode_output_dpms (output=0x558b2dab8ee0, mode=3) at ../../src/drmmode_display.c:921
        drmmode_output = 0x558b2dab8cc0
        koutput = 0x0
        props = <optimized out>
        drmmode = 0x558b2dab5200
        mode_id = -1
        i = 0
#1  0x0000558b2ba39223 in xf86DisableUnusedFunctions (pScrn=0x558b2dab4930) at ../../../../../../hw/xfree86/modes/xf86Crtc.c:3000
        output = <optimized out>
        xf86_config = 0x558b2dab56e0
        o = 6
        c = <optimized out>
#2  0x0000558b2ba40ef0 in xf86RandR12CrtcSet (pScreen=<optimized out>, randr_crtc=0x558b2daf51f0, randr_mode=0x0, x=0, y=0, rotation=<optimized out>, num_randr_outputs=0, randr_outputs=0x0)
    at ../../../../../../hw/xfree86/modes/xf86RandR12.c:1245
        pScrn = 0x558b2dab4930
        config = <optimized out>
        crtc = 0x558b2dab9ec0
        transform = 0x0
        changed = <optimized out>
        o = <optimized out>
        ro = <optimized out>
        save_crtcs = 0x558b2debcc50
        save_enabled = <optimized out>
#3  0x0000558b2ba84a32 in RRCrtcSet (crtc=<optimized out>, mode=0x0, x=0, y=0, rotation=rotation@entry=1, numOutputs=numOutputs@entry=0, outputs=0x0) at ../../../../randr/rrcrtc.c:768
        pScreen = 0x558b2daacb80
        ret = <optimized out>
        crtcChanged = <optimized out>
        o = <optimized out>
#4  0x0000558b2ba862b3 in ProcRRSetCrtcConfig (client=0x558b2de43170) at ../../../../randr/rrcrtc.c:1366
        stuff = <optimized out>
        rep = {type = 0 '\000', status = 0 '\000', sequenceNumber = 0, length = 0, newTimestamp = 766260624, pad1 = 21899, pad2 = 735925696, pad3 = 21899, pad4 = 1660946009, pad5 = 32568}
        pScreen = <optimized out>
        crtc = 0x558b2daf51f0
        mode = 0x0
        numOutputs = 0
        outputs = <optimized out>
        time = <optimized out>
        rotation = 1
        ret = <optimized out>
        i = <optimized out>
        j = <optimized out>
        status = <optimized out>
#5  0x0000558b2b9c0e98 in Dispatch () at ../../../../dix/dispatch.c:479
        result = <optimized out>
        start_tick = 930
#6  0x0000558b2b9c4ee0 in dix_main (argc=3, argv=0x7ffdc0d05ac8, envp=<optimized out>) at ../../../../dix/main.c:287
        i = <optimized out>
        alwaysCheckForInput = {0, 1}
#7  0x00007f3865d7db97 in __libc_start_main (main=0x558b2b9aeb50 <main>, argc=3, argv=0x7ffdc0d05ac8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffdc0d05ab8) at ../csu/libc-start.c:310
        result = <optimized out>
        unwind_buf = 
              {cancel_jmp_buf = {{jmp_buf = {0, 837132253956422676, 94056220388192, 140727838333632, 0, 0, 6878053941391146004, 6843373089882258452}, mask_was_saved = 0}}, priv = {pad = {0x0, 0x0, 0x7f3868887733 <_dl_init+259>, 0x7f3868875438}, data = {prev = 0x0, cleanup = 0x0, canceltype = 1753773875}}}
        not_first_call = <optimized out>
#8  0x0000558b2b9aeb8a in _start ()
Comment 2 Ilia Mirkin 2018-06-21 15:14:25 UTC
Unfortunately DP connectors appearing and disappearing (due to MST) is a larger project. I've been trying to get people to test my patches, but thus far without someone consistently being able to iterate.

The tree is here:

https://github.com/imirkin/xf86-video-nouveau

At this point, I know it doesn't work, but I don't know why. (Needless to say, I don't have such a setup myself.) I believe it handles the disconnect fine, but doesn't find the new connectors on reconnect.

If you could run that, and provide information (or even better, figure out why it doesn't work, since you seem like you have such capability), I'd really appreciate it.
Comment 3 John Lindgren 2018-06-21 15:42:22 UTC
Ilia, I tested your tree and it behaves as you expected -- it fixes the crash on undocking (I see that it adds the same koutput == NULL check) but doesn't find the external displays on docking again.  I'm not sure what further information you want me to provide.

I understand that you would like to finish the larger project and get external DP displays working 100%, but is there any reason why we couldn't start by committing the koutput == NULL check to fix the crash?
Comment 4 Ilia Mirkin 2018-06-21 15:45:45 UTC
(In reply to John Lindgren from comment #3)
> Ilia, I tested your tree and it behaves as you expected -- it fixes the
> crash on undocking (I see that it adds the same koutput == NULL check) but
> doesn't find the external displays on docking again.  I'm not sure what
> further information you want me to provide.

The xorg log with my various additional prints + the output of "xrandr" after you reconnect the external displays would be a good start.

> 
> I understand that you would like to finish the larger project and get
> external DP displays working 100%, but is there any reason why we couldn't
> start by committing the koutput == NULL check to fix the crash?

Absolutely, I think that's a good idea. [I can't do it this second, but will try to get to it soon.]
Comment 5 John Lindgren 2018-06-21 16:35:55 UTC
Created attachment 140268 [details]
X.org log
Comment 6 John Lindgren 2018-06-21 16:36:48 UTC
xrandr before undocking:

Screen 0: minimum 320 x 200, current 5760 x 1200, maximum 16384 x 16384
eDP-1 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 344mm x 194mm
   1920x1080     60.02*+
   1680x1050     60.00  
   1400x1050     60.00  
   1280x1024     59.95  
   1280x960      59.99  
   1152x864      59.97  
   1024x768      59.95  
   800x600       59.96  
   640x480       59.94  
   720x400       59.97  
   640x400       59.96  
   640x350       59.84  
DP-1 disconnected (normal left inverted right x axis y axis)
DP-2 disconnected (normal left inverted right x axis y axis)
DP-3 disconnected (normal left inverted right x axis y axis)
DP-1-1 connected 1920x1200+1920+0 (normal left inverted right x axis y axis) 518mm x 324mm
   1920x1200     59.95*+
   1920x1080     60.00  
   1600x1200     60.00  
   1680x1050     59.95  
   1280x1024     60.02  
   1280x960      60.00  
   1024x768      60.00  
   800x600       60.32  
   640x480       59.94  
   720x400       70.08  
DP-1-2 connected 1920x1200+3840+0 (normal left inverted right x axis y axis) 518mm x 324mm
   1920x1200     59.95*+
   1920x1080     60.00  
   1600x1200     60.00  
   1680x1050     59.95  
   1280x1024     60.02  
   1280x960      60.00  
   1024x768      60.00  
   800x600       60.32  
   640x480       59.94  
   720x400       70.08  
DP-1-3 disconnected (normal left inverted right x axis y axis)

xrandr after undocking:

Screen 0: minimum 320 x 200, current 1920 x 1080, maximum 16384 x 16384
eDP-1 connected 1920x1080+0+0 (normal left inverted right x axis y axis) 344mm x 194mm
   1920x1080     60.02*+
   1680x1050     60.00  
   1400x1050     60.00  
   1280x1024     59.95  
   1280x960      59.99  
   1152x864      59.97  
   1024x768      59.95  
   800x600       59.96  
   640x480       59.94  
   720x400       59.97  
   640x400       59.96  
   640x350       59.84  
DP-1 disconnected (normal left inverted right x axis y axis)
DP-2 disconnected (normal left inverted right x axis y axis)
DP-3 disconnected (normal left inverted right x axis y axis)
DP-1-1 disconnected (normal left inverted right x axis y axis)
DP-1-2 disconnected (normal left inverted right x axis y axis)
DP-1-3 disconnected (normal left inverted right x axis y axis)

No difference after docking again.
Comment 7 Ilia Mirkin 2018-06-21 16:43:23 UTC
Right. The MST outputs (DP-1-1, etc) never get removed, and so then they never get reinitialized. Will try to confirm with people in the know as to what the expected situation is. I probably missed something in my copying of the logic added to the modesetting driver.

Thanks for that!
Comment 8 John Lindgren 2018-09-07 14:40:30 UTC
Ping.  Can someone review this patch and if it looks good, apply it please?
Comment 9 Ilia Mirkin 2018-10-09 04:50:53 UTC
I tested something very close to the patches I had, and it seemed to work fine. The monitors came back OK. Perhaps there was a kernel issue here. Not sure. I've updated my github branch, and sent an updated patch to the ML. I expect to make a release with it in the near future.


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.