| Message ID | 4yfdzisxkb3j3tig2astee5zd46ppt2jwhqffkhes2dwm3g5nb@snadyfwzl7g4 (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] drm/i915: Fix NULL pointer dereference in capture_engine | expand |
On Tue, 03 Dec 2024, Eugene Kobyak <eugene.kobyak@intel.com> wrote: > When the intel_context structure contains NULL, > it raises a NULL pointer dereference error in drm_info(). Blank line here between commit message body and trailers. > Fixes: e8a3319c31a1 ("drm/i915: Allow error capture without a request") > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12309 > Cc: John Harrison <John.C.Harrison@Intel.com> > Cc: <stable@vger.kernel.org> # v6.3+ > Signed-off-by: Eugene Kobyak <eugene.kobyak@intel.com> > --- > v2: > - return drm_info to separate condition > v3: > - create separate condition which generate string if intel_context exist > v4: > - rollback and add check intel_context in log condition > v5: > - create separate string with guc_id if intel_context exist > drivers/gpu/drm/i915/i915_gpu_error.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index 135ded17334e..4ca6b9872a48 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -1643,9 +1643,12 @@ capture_engine(struct intel_engine_cs *engine, > return NULL; > > intel_engine_get_hung_entity(engine, &ce, &rq); > - if (rq && !i915_request_started(rq)) > - drm_info(&engine->gt->i915->drm, "Got hung context on %s with active request %lld:%lld [0x%04X] not yet started\n", > - engine->name, rq->fence.context, rq->fence.seqno, ce->guc_id.id); > + if (rq && !i915_request_started(rq)) { > + char guc_id[11]; > + ce ? snprintf(guc_id, sizeof(guc_id), " [0x%04x] ", ce->guc_id.id) : snprintf(guc_id, sizeof(guc_id), " "); I don't know if there's a separate coding style entry saying you shouldn't use a ternary operator like that, but you shouldn't use a ternary operator like that. BR, Jani. > + drm_info(&engine->gt->i915->drm, "Got hung context on %s with active request %lld:%lld%snot yet started\n", > + engine->name, rq->fence.context, rq->fence.seqno, guc_id); > + } > > if (rq) { > capture = intel_engine_coredump_add_request(ee, rq, ATOMIC_MAYFAIL);
Hi Eugene, > Cc: <stable@vger.kernel.org> # v6.3+ Next time cc also the stable kernel mailing list for real, please. > + drm_info(&engine->gt->i915->drm, "Got hung context on %s with active request %lld:%lld%snot yet started\n", > + engine->name, rq->fence.context, rq->fence.seqno, guc_id); this is very ugly: %lld:%lld%snot I understand you leave a space from the seqno and "not", but the form is unreadable. Could it be better: if (ce) drm_info(.... ->guc_id); else drm_info(...); <-- same thing without the guc_id It looks like for making it easier we are making it harder. If you decide to go this way, perhaps you can add a comment saying that ce might be NULL, but if it's not you want to keep the guc_id information. Next time cc also the stable kernel mailing list, please. Thanks, Andi
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 135ded17334e..4ca6b9872a48 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -1643,9 +1643,12 @@ capture_engine(struct intel_engine_cs *engine, return NULL; intel_engine_get_hung_entity(engine, &ce, &rq); - if (rq && !i915_request_started(rq)) - drm_info(&engine->gt->i915->drm, "Got hung context on %s with active request %lld:%lld [0x%04X] not yet started\n", - engine->name, rq->fence.context, rq->fence.seqno, ce->guc_id.id); + if (rq && !i915_request_started(rq)) { + char guc_id[11]; + ce ? snprintf(guc_id, sizeof(guc_id), " [0x%04x] ", ce->guc_id.id) : snprintf(guc_id, sizeof(guc_id), " "); + drm_info(&engine->gt->i915->drm, "Got hung context on %s with active request %lld:%lld%snot yet started\n", + engine->name, rq->fence.context, rq->fence.seqno, guc_id); + } if (rq) { capture = intel_engine_coredump_add_request(ee, rq, ATOMIC_MAYFAIL);
When the intel_context structure contains NULL, it raises a NULL pointer dereference error in drm_info(). Fixes: e8a3319c31a1 ("drm/i915: Allow error capture without a request") Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12309 Cc: John Harrison <John.C.Harrison@Intel.com> Cc: <stable@vger.kernel.org> # v6.3+ Signed-off-by: Eugene Kobyak <eugene.kobyak@intel.com> --- v2: - return drm_info to separate condition v3: - create separate condition which generate string if intel_context exist v4: - rollback and add check intel_context in log condition v5: - create separate string with guc_id if intel_context exist drivers/gpu/drm/i915/i915_gpu_error.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)