diff mbox series

[5.10,154/167] Revert "drm/i915: Propagate errors on awaiting already signaled fences"

Message ID 20210726153844.582795218@linuxfoundation.org
State New
Headers show
Series None | expand

Commit Message

Greg Kroah-Hartman July 26, 2021, 3:39 p.m. UTC 
From: Jason Ekstrand <jason@jlekstrand.net>

commit 3761baae908a7b5012be08d70fa553cc2eb82305 upstream.

This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever
since that commit, we've been having issues where a hang in one client
can propagate to another.  In particular, a hang in an app can propagate
to the X server which causes the whole desktop to lock up.

Error propagation along fences sound like a good idea, but as your bug
shows, surprising consequences, since propagating errors across security
boundaries is not a good thing.

What we do have is track the hangs on the ctx, and report information to
userspace using RESET_STATS. That's how arb_robustness works. Also, if my
understanding is still correct, the EIO from execbuf is when your context
is banned (because not recoverable or too many hangs). And in all these
cases it's up to userspace to figure out what is all impacted and should
be reported to the application, that's not on the kernel to guess and
automatically propagate.

What's more, we're also building more features on top of ctx error
reporting with RESET_STATS ioctl: Encrypted buffers use the same, and the
userspace fence wait also relies on that mechanism. So it is the path
going forward for reporting gpu hangs and resets to userspace.

So all together that's why I think we should just bury this idea again as
not quite the direction we want to go to, hence why I think the revert is
the right option here.

For backporters: Please note that you _must_ have a backport of
https://lore.kernel.org/dri-devel/20210602164149.391653-2-jason@jlekstrand.net/
for otherwise backporting just this patch opens up a security bug.

v2: Augment commit message. Also restore Jason's sob that I
accidentally lost.

v3: Add a note for backporters

Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Reported-by: Marcin Slusarz <marcin.slusarz@intel.com>
Cc: <stable@vger.kernel.org> # v5.6+
Cc: Jason Ekstrand <jason.ekstrand@intel.com>
Cc: Marcin Slusarz <marcin.slusarz@intel.com>
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080
Fixes: 9e31c1fe45d5 ("drm/i915: Propagate errors on awaiting already signaled fences")
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20210714193419.1459723-3-jason@jlekstrand.net
(cherry picked from commit 93a2711cddd5760e2f0f901817d71c93183c3b87)
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/gpu/drm/i915/i915_request.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Pavel Machek July 27, 2021, 9:35 p.m. UTC | #1 
Hi!

According to changelog, this introduces security hole.

> From: Jason Ekstrand <jason@jlekstrand.net>

> 

> commit 3761baae908a7b5012be08d70fa553cc2eb82305 upstream.

> 

> This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever

> since that commit, we've been having issues where a hang in one

> client


Hmm. Sounds like problem I'm seeing in mainline. So... good to know.

> For backporters: Please note that you _must_ have a backport of

> https://lore.kernel.org/dri-devel/20210602164149.391653-2-jason@jlekstrand.net/

> for otherwise backporting just this patch opens up a security bug.


AFAICT we don't have that c9d9fdbc108af8915d3f497bbdf3898bf8f321b8
drm/i915: Revert "drm/i915/gem: Asynchronous cmdparser" in 5.10 tree.

Hmm, and it needs follow up fix:
6e0b6528d783b2b87bd9e1bea97cf4dac87540d7 drm/i915: Correct the docs
for intel_engine_cmd_parser.

(Someone please double check this).

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Greg Kroah-Hartman July 28, 2021, 11:59 a.m. UTC | #2 
On Tue, Jul 27, 2021 at 11:35:46PM +0200, Pavel Machek wrote:
> Hi!

> 

> According to changelog, this introduces security hole.

> 

> > From: Jason Ekstrand <jason@jlekstrand.net>

> > 

> > commit 3761baae908a7b5012be08d70fa553cc2eb82305 upstream.

> > 

> > This reverts commit 9e31c1fe45d555a948ff66f1f0e3fe1f83ca63f7.  Ever

> > since that commit, we've been having issues where a hang in one

> > client

> 

> Hmm. Sounds like problem I'm seeing in mainline. So... good to know.

> 

> > For backporters: Please note that you _must_ have a backport of

> > https://lore.kernel.org/dri-devel/20210602164149.391653-2-jason@jlekstrand.net/

> > for otherwise backporting just this patch opens up a security bug.

> 

> AFAICT we don't have that c9d9fdbc108af8915d3f497bbdf3898bf8f321b8

> drm/i915: Revert "drm/i915/gem: Asynchronous cmdparser" in 5.10 tree.

> 

> Hmm, and it needs follow up fix:

> 6e0b6528d783b2b87bd9e1bea97cf4dac87540d7 drm/i915: Correct the docs

> for intel_engine_cmd_parser.

> 

> (Someone please double check this).


thanks, let me drop this for now.

Jason, you sent me a single patch to backport, should this be 3 patches
instead?

thanks,

greg k-h
diff mbox series

Patch

--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1285,10 +1285,8 @@  i915_request_await_execution(struct i915
 
 	do {
 		fence = *child++;
-		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
-			i915_sw_fence_set_error_once(&rq->submit, fence->error);
+		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 			continue;
-		}
 
 		if (fence->context == rq->fence.context)
 			continue;
@@ -1386,10 +1384,8 @@  i915_request_await_dma_fence(struct i915
 
 	do {
 		fence = *child++;
-		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
-			i915_sw_fence_set_error_once(&rq->submit, fence->error);
+		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 			continue;
-		}
 
 		/*
 		 * Requests on the same timeline are explicitly ordered, along