| Message ID | 20181108082057.29001-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm: Check if primary mst is null | expand |
On Thu, 2018-11-08 at 10:20 +0200, Stanislav Lisovskiy wrote: > Unfortunately drm_dp_get_mst_branch_device which is called from both > drm_dp_mst_handle_down_rep and drm_dp_mst_handle_up_rep seem to rely > on that mgr->mst_primary is not NULL, which seem to be wrong as it > can be > cleared with simultaneous mode set, if probing fails or in other > case. > mgr->lock mutex doesn't protect against that as it might just get > assigned to NULL right before, not simultaneously. > > There are currently bugs 107738, 108616 bugs which crash in > drm_dp_get_mst_branch_device, caused by this issue. > > v2: Refactored the code, as it was nicely noticed. > Fixed Bugzilla bug numbers(second was 108616, but not 108816) > and added links. > Hi Lyude Paul, Thank you for quick review, just poking you here as requested :) > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108616 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107738 > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 5ff1d79b86c4..0e0df398222d 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1275,6 +1275,9 @@ static struct drm_dp_mst_branch > *drm_dp_get_mst_branch_device(struct drm_dp_mst_ > mutex_lock(&mgr->lock); > mstb = mgr->mst_primary; > > + if (!mstb) > + goto out; > + > for (i = 0; i < lct - 1; i++) { > int shift = (i % 2) ? 0 : 4; > int port_num = (rad[i / 2] >> shift) & 0xf;
On Thu, 2018-11-08 at 09:21 +0000, Patchwork wrote: > == Series Details == > > Series: drm: Check if primary mst is null (rev2) > URL : https://patchwork.freedesktop.org/series/52174/ > State : failure > > == Summary == > > = CI Bug Log - changes from CI_DRM_5103 -> Patchwork_10770 = > > == Summary - FAILURE == > > Serious unknown changes coming with Patchwork_10770 absolutely need > to be > verified manually. The patch itself is a two line fix against NULL pointer dereference like if (!mgr->mst_primary) goto exit. So most likely this is noise. If this check made it go different code path, this means it would crash anyway. Otherwise codepath would be exactly same as before. Also previously v1 version passed, even though new v2 changes are cosmetic. (just following instructions to notify you, instead of blindly tapping "test latest revision" button :D) > > If you think the reported changes have nothing to do with the > changes > introduced in Patchwork_10770, please notify your bug team to allow > them > to document this new failure mode, which will reduce false > positives in CI. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/5217 > 4/revisions/2/mbox/ > > == Possible new issues == > > Here are the unknown changes that may have been introduced in > Patchwork_10770: > > === IGT changes === > > ==== Possible regressions ==== > > igt@drv_selftest@live_hangcheck: > fi-cfl-8109u: PASS -> DMESG-FAIL +1 > > > == Known issues == > > Here are the changes found in Patchwork_10770 that come from known > issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@kms_chamelium@common-hpd-after-suspend: > fi-skl-6700k2: PASS -> INCOMPLETE (fdo#104108, > k.org#199541, fdo#107773, fdo#105524) > > igt@kms_frontbuffer_tracking@basic: > fi-byt-clapper: PASS -> FAIL (fdo#103167) > > igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b: > fi-byt-clapper: PASS -> FAIL (fdo#107362) > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c: > fi-icl-u: PASS -> INCOMPLETE (fdo#107713) > > > ==== Possible fixes ==== > > igt@gem_ctx_create@basic-files: > fi-icl-u2: DMESG-WARN (fdo#107724) -> PASS > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b: > fi-byt-clapper: FAIL (fdo#107362, fdo#103191) -> PASS > > igt@prime_vgem@basic-fence-flip: > fi-ilk-650: FAIL (fdo#104008) -> PASS > > > ==== Warnings ==== > > igt@drv_selftest@live_contexts: > fi-icl-u2: DMESG-FAIL (fdo#108569) -> INCOMPLETE > (fdo#108315) > > > fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167 > fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191 > fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008 > fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108 > fdo#105524 https://bugs.freedesktop.org/show_bug.cgi?id=105524 > fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362 > fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713 > fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724 > fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773 > fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315 > fdo#108569 https://bugs.freedesktop.org/show_bug.cgi?id=108569 > k.org#199541 https://bugzilla.kernel.org/show_bug.cgi?id=199541 > > > == Participating hosts (54 -> 47) == > > Missing (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt- > squawks fi-bsw-cyan fi-snb-2520m fi-ctg-p8600 > > > == Build changes == > > * Linux: CI_DRM_5103 -> Patchwork_10770 > > CI_DRM_5103: 23c1138030ad65402f698ab0b356e2f55722bc77 @ > git://anongit.freedesktop.org/gfx-ci/linux > IGT_4712: a3ede1b535ac8137f6949c468edd7054453d5dae @ > git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_10770: 13e7715c72d20e925ff22507365c9273c54ed4c6 @ > git://anongit.freedesktop.org/gfx-ci/linux > > > == Linux commits == > > 13e7715c72d2 drm: Check if primary mst is null > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchw > ork_10770/issues.html
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 5ff1d79b86c4..0e0df398222d 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1275,6 +1275,9 @@ static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_ mutex_lock(&mgr->lock); mstb = mgr->mst_primary; + if (!mstb) + goto out; + for (i = 0; i < lct - 1; i++) { int shift = (i % 2) ? 0 : 4; int port_num = (rad[i / 2] >> shift) & 0xf;
Unfortunately drm_dp_get_mst_branch_device which is called from both drm_dp_mst_handle_down_rep and drm_dp_mst_handle_up_rep seem to rely on that mgr->mst_primary is not NULL, which seem to be wrong as it can be cleared with simultaneous mode set, if probing fails or in other case. mgr->lock mutex doesn't protect against that as it might just get assigned to NULL right before, not simultaneously. There are currently bugs 107738, 108616 bugs which crash in drm_dp_get_mst_branch_device, caused by this issue. v2: Refactored the code, as it was nicely noticed. Fixed Bugzilla bug numbers(second was 108616, but not 108816) and added links. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108616 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107738 Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++ 1 file changed, 3 insertions(+)