| Message ID | 1417802083-4695-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
This is already part of the seqno/request patch series and has been right from the start. See email 'drm/i915: Zero fill the request structure'. On 05/12/2014 17:54, Mika Kuoppala wrote: > Otherwise we might end up referencing uninitialized fields. > This is apparent when we try to cleanup the preallocated request > on ring reset, before any request has been submitted to the ring. > The request->ctx is foobar and we end up freeing the foobarness. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=86959 > References: https://bugs.freedesktop.org/show_bug.cgi?id=86962 > References: https://bugs.freedesktop.org/show_bug.cgi?id=86992 > Cc: John Harrison <John.C.Harrison@Intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 79b4ca5..2c6c6f8 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring) > if (ring->outstanding_lazy_request) > return 0; > > - request = kmalloc(sizeof(*request), GFP_KERNEL); > + request = kzalloc(sizeof(*request), GFP_KERNEL); > if (request == NULL) > return -ENOMEM; >
But yes, the reasoning why it crashes without the zero fill is correct. Dodgy context pointers that used to be ignored now get processed. Doing the zero fill keeps it all sane. On 05/12/2014 17:54, John Harrison wrote: > This is already part of the seqno/request patch series and has been > right from the start. See email 'drm/i915: Zero fill the request > structure'. > > On 05/12/2014 17:54, Mika Kuoppala wrote: >> Otherwise we might end up referencing uninitialized fields. >> This is apparent when we try to cleanup the preallocated request >> on ring reset, before any request has been submitted to the ring. >> The request->ctx is foobar and we end up freeing the foobarness. >> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=86959 >> References: https://bugs.freedesktop.org/show_bug.cgi?id=86962 >> References: https://bugs.freedesktop.org/show_bug.cgi?id=86992 >> Cc: John Harrison <John.C.Harrison@Intel.com> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c >> b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index 79b4ca5..2c6c6f8 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs >> *ring) >> if (ring->outstanding_lazy_request) >> return 0; >> - request = kmalloc(sizeof(*request), GFP_KERNEL); >> + request = kzalloc(sizeof(*request), GFP_KERNEL); >> if (request == NULL) >> return -ENOMEM; >
John Harrison <John.C.Harrison@Intel.com> writes: > This is already part of the seqno/request patch series and has been > right from the start. See email 'drm/i915: Zero fill the request structure'. Indeed. Sorry for missing this one. My patch can be ignored. But do you agree on failure path? -Mika > On 05/12/2014 17:54, Mika Kuoppala wrote: >> Otherwise we might end up referencing uninitialized fields. >> This is apparent when we try to cleanup the preallocated request >> on ring reset, before any request has been submitted to the ring. >> The request->ctx is foobar and we end up freeing the foobarness. >> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=86959 >> References: https://bugs.freedesktop.org/show_bug.cgi?id=86962 >> References: https://bugs.freedesktop.org/show_bug.cgi?id=86992 >> Cc: John Harrison <John.C.Harrison@Intel.com> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index 79b4ca5..2c6c6f8 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring) >> if (ring->outstanding_lazy_request) >> return 0; >> >> - request = kmalloc(sizeof(*request), GFP_KERNEL); >> + request = kzalloc(sizeof(*request), GFP_KERNEL); >> if (request == NULL) >> return -ENOMEM; >>
On Fri, Dec 05, 2014 at 06:02:35PM +0000, John Harrison wrote: > But yes, the reasoning why it crashes without the zero fill is correct. > Dodgy context pointers that used to be ignored now get processed. Doing the > zero fill keeps it all sane. > > On 05/12/2014 17:54, John Harrison wrote: > >This is already part of the seqno/request patch series and has been right > >from the start. See email 'drm/i915: Zero fill the request structure'. > > > >On 05/12/2014 17:54, Mika Kuoppala wrote: > >>Otherwise we might end up referencing uninitialized fields. > >>This is apparent when we try to cleanup the preallocated request > >>on ring reset, before any request has been submitted to the ring. > >>The request->ctx is foobar and we end up freeing the foobarness. > >> > >>References: https://bugs.freedesktop.org/show_bug.cgi?id=86959 > >>References: https://bugs.freedesktop.org/show_bug.cgi?id=86962 > >>References: https://bugs.freedesktop.org/show_bug.cgi?id=86992 > >>Cc: John Harrison <John.C.Harrison@Intel.com> > >>Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> I've added this explanation plus the note about which commit introduced the regression to John's patch and merged that one. Thanks for tracking this down. -Daniel > >>--- > >> drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > >>b/drivers/gpu/drm/i915/intel_ringbuffer.c > >>index 79b4ca5..2c6c6f8 100644 > >>--- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >>@@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs > >>*ring) > >> if (ring->outstanding_lazy_request) > >> return 0; > >> - request = kmalloc(sizeof(*request), GFP_KERNEL); > >>+ request = kzalloc(sizeof(*request), GFP_KERNEL); > >> if (request == NULL) > >> return -ENOMEM; > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 364/364 364/364
ILK -4 366/366 362/366
SNB -1 450/450 449/450
IVB +17 481/498 498/498
BYT 289/289 289/289
HSW -1 564/564 563/564
BDW 417/417 417/417
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
ILK igt_kms_flip_rcs-wf_vblank-vs-dpms-interruptible DMESG_WARN(1, M26)PASS(4, M37M26) DMESG_WARN(1, M26)
*ILK igt_kms_flip_flip-vs-panning PASS(2, M37M26) DMESG_WARN(1, M26)
ILK igt_kms_flip_plain-flip-ts-check-interruptible DMESG_WARN(1, M26)PASS(1, M37) DMESG_WARN(1, M26)
*ILK igt_kms_flip_rcs-flip-vs-modeset PASS(3, M37M26) DMESG_WARN(1, M26)
SNB igt_kms_force_connector NRUN(4, M35M22)PASS(1, M35) NRUN(1, M35)
IVB igt_kms_3d DMESG_WARN(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-128x128-onscreen NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-128x128-random NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-128x128-sliding NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-256x256-offscreen NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-256x256-onscreen NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-256x256-sliding NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-64x64-offscreen NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-64x64-onscreen NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-64x64-random NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-64x64-sliding NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_cursor_crc_cursor-size-change NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_fence_pin_leak NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_rotation_crc_primary-rotation NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
IVB igt_kms_rotation_crc_sprite-rotation NSPT(1, M34)PASS(14, M4M34M21) PASS(1, M34)
HSW igt_kms_force_connector NRUN(4, M40M19M20)PASS(1, M40) NRUN(1, M40)
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 79b4ca5..2c6c6f8 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2030,7 +2030,7 @@ intel_ring_alloc_request(struct intel_engine_cs *ring) if (ring->outstanding_lazy_request) return 0; - request = kmalloc(sizeof(*request), GFP_KERNEL); + request = kzalloc(sizeof(*request), GFP_KERNEL); if (request == NULL) return -ENOMEM;
Otherwise we might end up referencing uninitialized fields. This is apparent when we try to cleanup the preallocated request on ring reset, before any request has been submitted to the ring. The request->ctx is foobar and we end up freeing the foobarness. References: https://bugs.freedesktop.org/show_bug.cgi?id=86959 References: https://bugs.freedesktop.org/show_bug.cgi?id=86962 References: https://bugs.freedesktop.org/show_bug.cgi?id=86992 Cc: John Harrison <John.C.Harrison@Intel.com> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)