* [PATCH] drm/mst: Fix MST sideband up-reply failure handling
@ 2019-05-23 21:24 Imre Deak
2019-05-23 22:09 ` Lyude Paul
0 siblings, 1 reply; 5+ messages in thread
From: Imre Deak @ 2019-05-23 21:24 UTC (permalink / raw)
To: dri-devel; +Cc: Dave Airlie
Fix the breakage resulting in the stacktrace below, due to tx queue
being full when trying to send an up-reply. txmsg->seqno is -1 in this
case leading to a corruption of the mstb object by
txmsg->dst->tx_slots[txmsg->seqno] = NULL;
in process_single_up_tx_qlock().
[ +0,005162] [drm:process_single_tx_qlock [drm_kms_helper]] set_hdr_from_dst_qlock: failed to find slot
[ +0,000015] [drm:drm_dp_send_up_ack_reply.constprop.19 [drm_kms_helper]] failed to send msg in q -11
[ +0,000939] BUG: kernel NULL pointer dereference, address: 00000000000005a0
[ +0,006982] #PF: supervisor write access in kernel mode
[ +0,005223] #PF: error_code(0x0002) - not-present page
[ +0,005135] PGD 0 P4D 0
[ +0,002581] Oops: 0002 [#1] PREEMPT SMP NOPTI
[ +0,004359] CPU: 1 PID: 1200 Comm: kworker/u16:3 Tainted: G U 5.2.0-rc1+ #410
[ +0,008433] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP, BIOS ICLSFWR1.R00.3175.A00.1904261428 04/26/2019
[ +0,013323] Workqueue: i915-dp i915_digport_work_func [i915]
[ +0,005676] RIP: 0010:queue_work_on+0x19/0x70
[ +0,004372] Code: ff ff ff 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 41 56 49 89 f6 41 55 41 89 fd 41 54 55 53 48 89 d3 9c 5d fa e8 e7 81 0c 00 <f0> 48 0f ba 2b 00 73 31 45 31 e4 f7 c5 00 02 00 00 74 13 e8 cf 7f
[ +0,018750] RSP: 0018:ffffc900007dfc50 EFLAGS: 00010006
[ +0,005222] RAX: 0000000000000046 RBX: 00000000000005a0 RCX: 0000000000000001
[ +0,007133] RDX: 000000000001b608 RSI: 0000000000000000 RDI: ffffffff82121972
[ +0,007129] RBP: 0000000000000202 R08: 0000000000000000 R09: 0000000000000001
[ +0,007129] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88847bfa5096
[ +0,007131] R13: 0000000000000010 R14: ffff88849c08f3f8 R15: 0000000000000000
[ +0,007128] FS: 0000000000000000(0000) GS:ffff88849dc80000(0000) knlGS:0000000000000000
[ +0,008083] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ +0,005749] CR2: 00000000000005a0 CR3: 0000000005210006 CR4: 0000000000760ee0
[ +0,007128] PKRU: 55555554
[ +0,002722] Call Trace:
[ +0,002458] drm_dp_mst_handle_up_req+0x517/0x540 [drm_kms_helper]
[ +0,006197] ? drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
[ +0,005764] drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
[ +0,005623] ? intel_dp_hpd_pulse+0x205/0x370 [i915]
[ +0,005018] intel_dp_hpd_pulse+0x205/0x370 [i915]
[ +0,004836] i915_digport_work_func+0xbb/0x140 [i915]
[ +0,005108] process_one_work+0x245/0x610
[ +0,004027] worker_thread+0x37/0x380
[ +0,003684] ? process_one_work+0x610/0x610
[ +0,004184] kthread+0x119/0x130
[ +0,003240] ? kthread_park+0x80/0x80
[ +0,003668] ret_from_fork+0x24/0x50
Cc: Lyude Paul <lyude@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index da1abca1b9e9..24c325f4a616 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1996,7 +1996,11 @@ static void process_single_up_tx_qlock(struct drm_dp_mst_topology_mgr *mgr,
if (ret != 1)
DRM_DEBUG_KMS("failed to send msg in q %d\n", ret);
- txmsg->dst->tx_slots[txmsg->seqno] = NULL;
+ if (txmsg->seqno != -1) {
+ WARN_ON((unsigned)txmsg->seqno >
+ ARRAY_SIZE(txmsg->dst->tx_slots));
+ txmsg->dst->tx_slots[txmsg->seqno] = NULL;
+ }
}
static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr,
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/mst: Fix MST sideband up-reply failure handling
2019-05-23 21:24 [PATCH] drm/mst: Fix MST sideband up-reply failure handling Imre Deak
@ 2019-05-23 22:09 ` Lyude Paul
2019-05-23 22:28 ` Imre Deak
0 siblings, 1 reply; 5+ messages in thread
From: Lyude Paul @ 2019-05-23 22:09 UTC (permalink / raw)
To: Imre Deak, dri-devel; +Cc: Dave Airlie
Patch mostly looks good to me, one comment below
On Fri, 2019-05-24 at 00:24 +0300, Imre Deak wrote:
> Fix the breakage resulting in the stacktrace below, due to tx queue
> being full when trying to send an up-reply. txmsg->seqno is -1 in this
> case leading to a corruption of the mstb object by
>
> txmsg->dst->tx_slots[txmsg->seqno] = NULL;
>
> in process_single_up_tx_qlock().
>
> [ +0,005162] [drm:process_single_tx_qlock [drm_kms_helper]]
> set_hdr_from_dst_qlock: failed to find slot
> [ +0,000015] [drm:drm_dp_send_up_ack_reply.constprop.19 [drm_kms_helper]]
> failed to send msg in q -11
> [ +0,000939] BUG: kernel NULL pointer dereference, address:
> 00000000000005a0
> [ +0,006982] #PF: supervisor write access in kernel mode
> [ +0,005223] #PF: error_code(0x0002) - not-present page
> [ +0,005135] PGD 0 P4D 0
> [ +0,002581] Oops: 0002 [#1] PREEMPT SMP NOPTI
> [ +0,004359] CPU: 1 PID: 1200 Comm: kworker/u16:3 Tainted:
> G U 5.2.0-rc1+ #410
> [ +0,008433] Hardware name: Intel Corporation Ice Lake Client
> Platform/IceLake U DDR4 SODIMM PD RVP, BIOS ICLSFWR1.R00.3175.A00.1904261428
> 04/26/2019
> [ +0,013323] Workqueue: i915-dp i915_digport_work_func [i915]
> [ +0,005676] RIP: 0010:queue_work_on+0x19/0x70
> [ +0,004372] Code: ff ff ff 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 41 56
> 49 89 f6 41 55 41 89 fd 41 54 55 53 48 89 d3 9c 5d fa e8 e7 81 0c 00 <f0> 48
> 0f ba 2b 00 73 31 45 31 e4 f7 c5 00 02 00 00 74 13 e8 cf 7f
> [ +0,018750] RSP: 0018:ffffc900007dfc50 EFLAGS: 00010006
> [ +0,005222] RAX: 0000000000000046 RBX: 00000000000005a0 RCX:
> 0000000000000001
> [ +0,007133] RDX: 000000000001b608 RSI: 0000000000000000 RDI:
> ffffffff82121972
> [ +0,007129] RBP: 0000000000000202 R08: 0000000000000000 R09:
> 0000000000000001
> [ +0,007129] R10: 0000000000000000 R11: 0000000000000000 R12:
> ffff88847bfa5096
> [ +0,007131] R13: 0000000000000010 R14: ffff88849c08f3f8 R15:
> 0000000000000000
> [ +0,007128] FS: 0000000000000000(0000) GS:ffff88849dc80000(0000)
> knlGS:0000000000000000
> [ +0,008083] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ +0,005749] CR2: 00000000000005a0 CR3: 0000000005210006 CR4:
> 0000000000760ee0
> [ +0,007128] PKRU: 55555554
> [ +0,002722] Call Trace:
> [ +0,002458] drm_dp_mst_handle_up_req+0x517/0x540 [drm_kms_helper]
> [ +0,006197] ? drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
> [ +0,005764] drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
> [ +0,005623] ? intel_dp_hpd_pulse+0x205/0x370 [i915]
> [ +0,005018] intel_dp_hpd_pulse+0x205/0x370 [i915]
> [ +0,004836] i915_digport_work_func+0xbb/0x140 [i915]
> [ +0,005108] process_one_work+0x245/0x610
> [ +0,004027] worker_thread+0x37/0x380
> [ +0,003684] ? process_one_work+0x610/0x610
> [ +0,004184] kthread+0x119/0x130
> [ +0,003240] ? kthread_park+0x80/0x80
> [ +0,003668] ret_from_fork+0x24/0x50
>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index da1abca1b9e9..24c325f4a616 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1996,7 +1996,11 @@ static void process_single_up_tx_qlock(struct
> drm_dp_mst_topology_mgr *mgr,
> if (ret != 1)
> DRM_DEBUG_KMS("failed to send msg in q %d\n", ret);
>
> - txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> + if (txmsg->seqno != -1) {
> + WARN_ON((unsigned)txmsg->seqno >
> + ARRAY_SIZE(txmsg->dst->tx_slots));
Not 100% sure on this nitpick myself but, if we know that txmsg->seqno is
about to go out of bounds shouldn't we also try to take action to stop it?
like
if (!WARN_ON((unsigned)txmsg->seqno > ARRAY_SIZE(txmsg->dst->tx_slots)))
txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> + txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> + }
> }
>
> static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr,
--
Cheers,
Lyude Paul
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/mst: Fix MST sideband up-reply failure handling
2019-05-23 22:09 ` Lyude Paul
@ 2019-05-23 22:28 ` Imre Deak
2019-05-23 22:31 ` Lyude Paul
0 siblings, 1 reply; 5+ messages in thread
From: Imre Deak @ 2019-05-23 22:28 UTC (permalink / raw)
To: Lyude Paul; +Cc: Dave Airlie, dri-devel
On Thu, May 23, 2019 at 06:09:56PM -0400, Lyude Paul wrote:
> Patch mostly looks good to me, one comment below
>
> On Fri, 2019-05-24 at 00:24 +0300, Imre Deak wrote:
> > Fix the breakage resulting in the stacktrace below, due to tx queue
> > being full when trying to send an up-reply. txmsg->seqno is -1 in this
> > case leading to a corruption of the mstb object by
> >
> > txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> >
> > in process_single_up_tx_qlock().
> >
> > [ +0,005162] [drm:process_single_tx_qlock [drm_kms_helper]]
> > set_hdr_from_dst_qlock: failed to find slot
> > [ +0,000015] [drm:drm_dp_send_up_ack_reply.constprop.19 [drm_kms_helper]]
> > failed to send msg in q -11
> > [ +0,000939] BUG: kernel NULL pointer dereference, address:
> > 00000000000005a0
> > [ +0,006982] #PF: supervisor write access in kernel mode
> > [ +0,005223] #PF: error_code(0x0002) - not-present page
> > [ +0,005135] PGD 0 P4D 0
> > [ +0,002581] Oops: 0002 [#1] PREEMPT SMP NOPTI
> > [ +0,004359] CPU: 1 PID: 1200 Comm: kworker/u16:3 Tainted:
> > G U 5.2.0-rc1+ #410
> > [ +0,008433] Hardware name: Intel Corporation Ice Lake Client
> > Platform/IceLake U DDR4 SODIMM PD RVP, BIOS ICLSFWR1.R00.3175.A00.1904261428
> > 04/26/2019
> > [ +0,013323] Workqueue: i915-dp i915_digport_work_func [i915]
> > [ +0,005676] RIP: 0010:queue_work_on+0x19/0x70
> > [ +0,004372] Code: ff ff ff 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 41 56
> > 49 89 f6 41 55 41 89 fd 41 54 55 53 48 89 d3 9c 5d fa e8 e7 81 0c 00 <f0> 48
> > 0f ba 2b 00 73 31 45 31 e4 f7 c5 00 02 00 00 74 13 e8 cf 7f
> > [ +0,018750] RSP: 0018:ffffc900007dfc50 EFLAGS: 00010006
> > [ +0,005222] RAX: 0000000000000046 RBX: 00000000000005a0 RCX:
> > 0000000000000001
> > [ +0,007133] RDX: 000000000001b608 RSI: 0000000000000000 RDI:
> > ffffffff82121972
> > [ +0,007129] RBP: 0000000000000202 R08: 0000000000000000 R09:
> > 0000000000000001
> > [ +0,007129] R10: 0000000000000000 R11: 0000000000000000 R12:
> > ffff88847bfa5096
> > [ +0,007131] R13: 0000000000000010 R14: ffff88849c08f3f8 R15:
> > 0000000000000000
> > [ +0,007128] FS: 0000000000000000(0000) GS:ffff88849dc80000(0000)
> > knlGS:0000000000000000
> > [ +0,008083] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ +0,005749] CR2: 00000000000005a0 CR3: 0000000005210006 CR4:
> > 0000000000760ee0
> > [ +0,007128] PKRU: 55555554
> > [ +0,002722] Call Trace:
> > [ +0,002458] drm_dp_mst_handle_up_req+0x517/0x540 [drm_kms_helper]
> > [ +0,006197] ? drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
> > [ +0,005764] drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
> > [ +0,005623] ? intel_dp_hpd_pulse+0x205/0x370 [i915]
> > [ +0,005018] intel_dp_hpd_pulse+0x205/0x370 [i915]
> > [ +0,004836] i915_digport_work_func+0xbb/0x140 [i915]
> > [ +0,005108] process_one_work+0x245/0x610
> > [ +0,004027] worker_thread+0x37/0x380
> > [ +0,003684] ? process_one_work+0x610/0x610
> > [ +0,004184] kthread+0x119/0x130
> > [ +0,003240] ? kthread_park+0x80/0x80
> > [ +0,003668] ret_from_fork+0x24/0x50
> >
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index da1abca1b9e9..24c325f4a616 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1996,7 +1996,11 @@ static void process_single_up_tx_qlock(struct
> > drm_dp_mst_topology_mgr *mgr,
> > if (ret != 1)
> > DRM_DEBUG_KMS("failed to send msg in q %d\n", ret);
> >
> > - txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> > + if (txmsg->seqno != -1) {
> > + WARN_ON((unsigned)txmsg->seqno >
> > + ARRAY_SIZE(txmsg->dst->tx_slots));
>
> Not 100% sure on this nitpick myself but, if we know that txmsg->seqno is
> about to go out of bounds shouldn't we also try to take action to stop it?
> like
Imo, it's worth keeping thins simple. If the WARN triggers we need to
fix the original issue in any case (txmsg->seqno never should be set to
anything else than -1 or a valid idx) so making the assignment here
conditional wouldn't have a real purpose.
>
> if (!WARN_ON((unsigned)txmsg->seqno > ARRAY_SIZE(txmsg->dst->tx_slots)))
> txmsg->dst->tx_slots[txmsg->seqno] = NULL;
>
>
>
> > + txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> > + }
> > }
> >
> > static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr,
> --
> Cheers,
> Lyude Paul
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/mst: Fix MST sideband up-reply failure handling
2019-05-23 22:28 ` Imre Deak
@ 2019-05-23 22:31 ` Lyude Paul
2019-05-30 9:43 ` Imre Deak
0 siblings, 1 reply; 5+ messages in thread
From: Lyude Paul @ 2019-05-23 22:31 UTC (permalink / raw)
To: imre.deak; +Cc: Dave Airlie, dri-devel
On Fri, 2019-05-24 at 01:28 +0300, Imre Deak wrote:
> On Thu, May 23, 2019 at 06:09:56PM -0400, Lyude Paul wrote:
> > Patch mostly looks good to me, one comment below
> >
> > On Fri, 2019-05-24 at 00:24 +0300, Imre Deak wrote:
> > > Fix the breakage resulting in the stacktrace below, due to tx queue
> > > being full when trying to send an up-reply. txmsg->seqno is -1 in this
> > > case leading to a corruption of the mstb object by
> > >
> > > txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> > >
> > > in process_single_up_tx_qlock().
> > >
> > > [ +0,005162] [drm:process_single_tx_qlock [drm_kms_helper]]
> > > set_hdr_from_dst_qlock: failed to find slot
> > > [ +0,000015] [drm:drm_dp_send_up_ack_reply.constprop.19
> > > [drm_kms_helper]]
> > > failed to send msg in q -11
> > > [ +0,000939] BUG: kernel NULL pointer dereference, address:
> > > 00000000000005a0
> > > [ +0,006982] #PF: supervisor write access in kernel mode
> > > [ +0,005223] #PF: error_code(0x0002) - not-present page
> > > [ +0,005135] PGD 0 P4D 0
> > > [ +0,002581] Oops: 0002 [#1] PREEMPT SMP NOPTI
> > > [ +0,004359] CPU: 1 PID: 1200 Comm: kworker/u16:3 Tainted:
> > > G U 5.2.0-rc1+ #410
> > > [ +0,008433] Hardware name: Intel Corporation Ice Lake Client
> > > Platform/IceLake U DDR4 SODIMM PD RVP, BIOS
> > > ICLSFWR1.R00.3175.A00.1904261428
> > > 04/26/2019
> > > [ +0,013323] Workqueue: i915-dp i915_digport_work_func [i915]
> > > [ +0,005676] RIP: 0010:queue_work_on+0x19/0x70
> > > [ +0,004372] Code: ff ff ff 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00
> > > 41 56
> > > 49 89 f6 41 55 41 89 fd 41 54 55 53 48 89 d3 9c 5d fa e8 e7 81 0c 00
> > > <f0> 48
> > > 0f ba 2b 00 73 31 45 31 e4 f7 c5 00 02 00 00 74 13 e8 cf 7f
> > > [ +0,018750] RSP: 0018:ffffc900007dfc50 EFLAGS: 00010006
> > > [ +0,005222] RAX: 0000000000000046 RBX: 00000000000005a0 RCX:
> > > 0000000000000001
> > > [ +0,007133] RDX: 000000000001b608 RSI: 0000000000000000 RDI:
> > > ffffffff82121972
> > > [ +0,007129] RBP: 0000000000000202 R08: 0000000000000000 R09:
> > > 0000000000000001
> > > [ +0,007129] R10: 0000000000000000 R11: 0000000000000000 R12:
> > > ffff88847bfa5096
> > > [ +0,007131] R13: 0000000000000010 R14: ffff88849c08f3f8 R15:
> > > 0000000000000000
> > > [ +0,007128] FS: 0000000000000000(0000) GS:ffff88849dc80000(0000)
> > > knlGS:0000000000000000
> > > [ +0,008083] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [ +0,005749] CR2: 00000000000005a0 CR3: 0000000005210006 CR4:
> > > 0000000000760ee0
> > > [ +0,007128] PKRU: 55555554
> > > [ +0,002722] Call Trace:
> > > [ +0,002458] drm_dp_mst_handle_up_req+0x517/0x540 [drm_kms_helper]
> > > [ +0,006197] ? drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
> > > [ +0,005764] drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
> > > [ +0,005623] ? intel_dp_hpd_pulse+0x205/0x370 [i915]
> > > [ +0,005018] intel_dp_hpd_pulse+0x205/0x370 [i915]
> > > [ +0,004836] i915_digport_work_func+0xbb/0x140 [i915]
> > > [ +0,005108] process_one_work+0x245/0x610
> > > [ +0,004027] worker_thread+0x37/0x380
> > > [ +0,003684] ? process_one_work+0x610/0x610
> > > [ +0,004184] kthread+0x119/0x130
> > > [ +0,003240] ? kthread_park+0x80/0x80
> > > [ +0,003668] ret_from_fork+0x24/0x50
> > >
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Dave Airlie <airlied@redhat.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > > drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index da1abca1b9e9..24c325f4a616 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -1996,7 +1996,11 @@ static void process_single_up_tx_qlock(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > > if (ret != 1)
> > > DRM_DEBUG_KMS("failed to send msg in q %d\n", ret);
> > >
> > > - txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> > > + if (txmsg->seqno != -1) {
> > > + WARN_ON((unsigned)txmsg->seqno >
> > > + ARRAY_SIZE(txmsg->dst->tx_slots));
> >
> > Not 100% sure on this nitpick myself but, if we know that txmsg->seqno is
> > about to go out of bounds shouldn't we also try to take action to stop it?
> > like
>
> Imo, it's worth keeping thins simple. If the WARN triggers we need to
> fix the original issue in any case (txmsg->seqno never should be set to
> anything else than -1 or a valid idx) so making the assignment here
> conditional wouldn't have a real purpose.
mm, fair enough. Then:
Reviewed-by: Lyude Paul <lyude@redhat.com>
Thanks for the fix!
>
> > if (!WARN_ON((unsigned)txmsg->seqno > ARRAY_SIZE(txmsg->dst->tx_slots)))
> > txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> >
> >
> >
> > > + txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> > > + }
> > > }
> > >
> > > static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr,
> > --
> > Cheers,
> > Lyude Paul
> >
--
Cheers,
Lyude Paul
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/mst: Fix MST sideband up-reply failure handling
2019-05-23 22:31 ` Lyude Paul
@ 2019-05-30 9:43 ` Imre Deak
0 siblings, 0 replies; 5+ messages in thread
From: Imre Deak @ 2019-05-30 9:43 UTC (permalink / raw)
To: Lyude Paul; +Cc: Dave Airlie, dri-devel
On Thu, May 23, 2019 at 06:31:15PM -0400, Lyude Paul wrote:
> On Fri, 2019-05-24 at 01:28 +0300, Imre Deak wrote:
> > On Thu, May 23, 2019 at 06:09:56PM -0400, Lyude Paul wrote:
> > > Patch mostly looks good to me, one comment below
> > >
> > > On Fri, 2019-05-24 at 00:24 +0300, Imre Deak wrote:
> > > > Fix the breakage resulting in the stacktrace below, due to tx queue
> > > > being full when trying to send an up-reply. txmsg->seqno is -1 in this
> > > > case leading to a corruption of the mstb object by
> > > >
> > > > txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> > > >
> > > > in process_single_up_tx_qlock().
> > > >
> > > > [ +0,005162] [drm:process_single_tx_qlock [drm_kms_helper]]
> > > > set_hdr_from_dst_qlock: failed to find slot
> > > > [ +0,000015] [drm:drm_dp_send_up_ack_reply.constprop.19
> > > > [drm_kms_helper]]
> > > > failed to send msg in q -11
> > > > [ +0,000939] BUG: kernel NULL pointer dereference, address:
> > > > 00000000000005a0
> > > > [ +0,006982] #PF: supervisor write access in kernel mode
> > > > [ +0,005223] #PF: error_code(0x0002) - not-present page
> > > > [ +0,005135] PGD 0 P4D 0
> > > > [ +0,002581] Oops: 0002 [#1] PREEMPT SMP NOPTI
> > > > [ +0,004359] CPU: 1 PID: 1200 Comm: kworker/u16:3 Tainted:
> > > > G U 5.2.0-rc1+ #410
> > > > [ +0,008433] Hardware name: Intel Corporation Ice Lake Client
> > > > Platform/IceLake U DDR4 SODIMM PD RVP, BIOS
> > > > ICLSFWR1.R00.3175.A00.1904261428
> > > > 04/26/2019
> > > > [ +0,013323] Workqueue: i915-dp i915_digport_work_func [i915]
> > > > [ +0,005676] RIP: 0010:queue_work_on+0x19/0x70
> > > > [ +0,004372] Code: ff ff ff 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00
> > > > 41 56
> > > > 49 89 f6 41 55 41 89 fd 41 54 55 53 48 89 d3 9c 5d fa e8 e7 81 0c 00
> > > > <f0> 48
> > > > 0f ba 2b 00 73 31 45 31 e4 f7 c5 00 02 00 00 74 13 e8 cf 7f
> > > > [ +0,018750] RSP: 0018:ffffc900007dfc50 EFLAGS: 00010006
> > > > [ +0,005222] RAX: 0000000000000046 RBX: 00000000000005a0 RCX:
> > > > 0000000000000001
> > > > [ +0,007133] RDX: 000000000001b608 RSI: 0000000000000000 RDI:
> > > > ffffffff82121972
> > > > [ +0,007129] RBP: 0000000000000202 R08: 0000000000000000 R09:
> > > > 0000000000000001
> > > > [ +0,007129] R10: 0000000000000000 R11: 0000000000000000 R12:
> > > > ffff88847bfa5096
> > > > [ +0,007131] R13: 0000000000000010 R14: ffff88849c08f3f8 R15:
> > > > 0000000000000000
> > > > [ +0,007128] FS: 0000000000000000(0000) GS:ffff88849dc80000(0000)
> > > > knlGS:0000000000000000
> > > > [ +0,008083] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [ +0,005749] CR2: 00000000000005a0 CR3: 0000000005210006 CR4:
> > > > 0000000000760ee0
> > > > [ +0,007128] PKRU: 55555554
> > > > [ +0,002722] Call Trace:
> > > > [ +0,002458] drm_dp_mst_handle_up_req+0x517/0x540 [drm_kms_helper]
> > > > [ +0,006197] ? drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
> > > > [ +0,005764] drm_dp_mst_hpd_irq+0x5b/0x9c0 [drm_kms_helper]
> > > > [ +0,005623] ? intel_dp_hpd_pulse+0x205/0x370 [i915]
> > > > [ +0,005018] intel_dp_hpd_pulse+0x205/0x370 [i915]
> > > > [ +0,004836] i915_digport_work_func+0xbb/0x140 [i915]
> > > > [ +0,005108] process_one_work+0x245/0x610
> > > > [ +0,004027] worker_thread+0x37/0x380
> > > > [ +0,003684] ? process_one_work+0x610/0x610
> > > > [ +0,004184] kthread+0x119/0x130
> > > > [ +0,003240] ? kthread_park+0x80/0x80
> > > > [ +0,003668] ret_from_fork+0x24/0x50
> > > >
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Cc: Dave Airlie <airlied@redhat.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > > drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++++-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > index da1abca1b9e9..24c325f4a616 100644
> > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > @@ -1996,7 +1996,11 @@ static void process_single_up_tx_qlock(struct
> > > > drm_dp_mst_topology_mgr *mgr,
> > > > if (ret != 1)
> > > > DRM_DEBUG_KMS("failed to send msg in q %d\n", ret);
> > > >
> > > > - txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> > > > + if (txmsg->seqno != -1) {
> > > > + WARN_ON((unsigned)txmsg->seqno >
> > > > + ARRAY_SIZE(txmsg->dst->tx_slots));
> > >
> > > Not 100% sure on this nitpick myself but, if we know that txmsg->seqno is
> > > about to go out of bounds shouldn't we also try to take action to stop it?
> > > like
> >
> > Imo, it's worth keeping thins simple. If the WARN triggers we need to
> > fix the original issue in any case (txmsg->seqno never should be set to
> > anything else than -1 or a valid idx) so making the assignment here
> > conditional wouldn't have a real purpose.
>
> mm, fair enough. Then:
>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
>
> Thanks for the fix!
Thanks for the review. Got now the permission and so pushed the change
to drm-misc-next. I wonder if there's a CI for this branch similar we
have in place for the intel-gfx branches. Maybe I should've CC'd the
patch to the intel-gfx ML for a pre-merge CI report?
>
> >
> > > if (!WARN_ON((unsigned)txmsg->seqno > ARRAY_SIZE(txmsg->dst->tx_slots)))
> > > txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> > >
> > >
> > >
> > > > + txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> > > > + }
> > > > }
> > > >
> > > > static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr,
> > > --
> > > Cheers,
> > > Lyude Paul
> > >
> --
> Cheers,
> Lyude Paul
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-05-30 9:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-23 21:24 [PATCH] drm/mst: Fix MST sideband up-reply failure handling Imre Deak
2019-05-23 22:09 ` Lyude Paul
2019-05-23 22:28 ` Imre Deak
2019-05-23 22:31 ` Lyude Paul
2019-05-30 9:43 ` Imre Deak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.