All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Hellstrom <thellstrom@vmware.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Dave Airlie <airlied@linux.ie>,
	Maarten Lankhorst <maarten.lankhorst@canonical.com>,
	Ingo Molnar <mingo@kernel.org>
Subject: Re: [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
Date: Thu, 12 Sep 2013 22:48:15 +0200	[thread overview]
Message-ID: <5232288F.4070904@vmware.com> (raw)
In-Reply-To: <alpine.DEB.2.02.1309122238430.4089@ionos.tec.linutronix.de>

On 09/12/2013 10:39 PM, Thomas Gleixner wrote:
> On Thu, 12 Sep 2013, Daniel Vetter wrote:
>
>> On Thu, Sep 12, 2013 at 10:20 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>> I think for ttm drivers it's just execbuf being exploitable. But on
>>>> drm/i915 we've
>>>> had the same issue with the pwrite/pread ioctls, so a simple
>>>> glBufferData(glMap) kind of recursion from gl clients blew the kernel
>>>> to pieces ...
>>> And the only answer you folks came up with is set_need_resched() and
>>> yield()? Oh well....
>> The yield was for a different lifelock, and that one is also fixed by
>> now. The fault handler deadlock was fixed in the usual "drop locks and
>> jump into slowpath" fasion, at least in drm/i915.
> So we can remove that whole yield/set_need_resched() mess completely ?
>
> Thanks,
>
> 	tglx
No.

The while(trylock) is there to address a potential locking inversion 
deadlock. If the trylock fails, the code returns to user-space which 
retries the fault. This code needs to stay until we can come up with 
either a way to drop the mmap_sem and sleep before returning to 
user-space, or a bunch of code is fixed with a different locking order.

The set_need_resched() can (and should according to Peter) go.

/Thomas

WARNING: multiple messages have this Message-ID (diff)
From: Thomas Hellstrom <thellstrom@vmware.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Peter Zijlstra <peterz@infradead.org>,
	Dave Airlie <airlied@linux.ie>,
	Maarten Lankhorst <maarten.lankhorst@canonical.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>
Subject: Re: [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE
Date: Thu, 12 Sep 2013 22:48:15 +0200	[thread overview]
Message-ID: <5232288F.4070904@vmware.com> (raw)
In-Reply-To: <alpine.DEB.2.02.1309122238430.4089@ionos.tec.linutronix.de>

On 09/12/2013 10:39 PM, Thomas Gleixner wrote:
> On Thu, 12 Sep 2013, Daniel Vetter wrote:
>
>> On Thu, Sep 12, 2013 at 10:20 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>>> I think for ttm drivers it's just execbuf being exploitable. But on
>>>> drm/i915 we've
>>>> had the same issue with the pwrite/pread ioctls, so a simple
>>>> glBufferData(glMap) kind of recursion from gl clients blew the kernel
>>>> to pieces ...
>>> And the only answer you folks came up with is set_need_resched() and
>>> yield()? Oh well....
>> The yield was for a different lifelock, and that one is also fixed by
>> now. The fault handler deadlock was fixed in the usual "drop locks and
>> jump into slowpath" fasion, at least in drm/i915.
> So we can remove that whole yield/set_need_resched() mess completely ?
>
> Thanks,
>
> 	tglx
No.

The while(trylock) is there to address a potential locking inversion 
deadlock. If the trylock fails, the code returns to user-space which 
retries the fault. This code needs to stay until we can come up with 
either a way to drop the mmap_sem and sleep before returning to 
user-space, or a bunch of code is fixed with a different locking order.

The set_need_resched() can (and should according to Peter) go.

/Thomas

  reply	other threads:[~2013-09-12 20:48 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-12 15:06 [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE Peter Zijlstra
2013-09-12 15:11 ` Maarten Lankhorst
2013-09-12 15:14   ` Peter Zijlstra
2013-09-12 15:14     ` Peter Zijlstra
2013-09-12 15:36 ` Daniel Vetter
2013-09-12 15:43   ` Peter Zijlstra
2013-09-12 15:43     ` Peter Zijlstra
2013-09-12 15:58     ` Daniel Vetter
2013-09-12 16:22       ` Peter Zijlstra
2013-09-12 16:35         ` Chris Wilson
2013-09-12 16:35           ` Chris Wilson
2013-09-12 20:30           ` Peter Zijlstra
2013-09-12 20:37             ` Thomas Gleixner
2013-09-12 19:52         ` Daniel Vetter
2013-09-12 19:58           ` Thomas Gleixner
2013-09-12 20:04             ` Daniel Vetter
2013-09-12 20:20               ` Thomas Gleixner
2013-09-12 20:23                 ` Daniel Vetter
2013-09-12 20:39                   ` Thomas Gleixner
2013-09-12 20:48                     ` Thomas Hellstrom [this message]
2013-09-12 20:48                       ` Thomas Hellstrom
2013-09-12 16:33       ` Thomas Hellstrom
2013-09-12 16:33         ` Thomas Hellstrom
2013-09-12 15:45   ` Maarten Lankhorst
2013-09-12 15:45     ` Maarten Lankhorst
2013-09-12 16:44     ` Thomas Hellstrom
2013-09-12 19:48       ` Daniel Vetter
2013-09-12 21:50       ` Maarten Lankhorst
2013-09-13  5:33         ` Thomas Hellstrom
2013-09-13  8:26           ` Daniel Vetter
2013-10-08 14:14           ` [RFC PATCH] drm/radeon: fixup locking inversion between mmap_sem and reservations Maarten Lankhorst
2013-10-08 14:33             ` Jerome Glisse
2013-10-08 14:45               ` Christian König
2013-10-08 14:55                 ` Jerome Glisse
2013-10-08 16:29                   ` Thomas Hellstrom
2013-10-08 16:47                     ` Jerome Glisse
2013-10-08 16:58                       ` Thomas Hellstrom
2013-10-09 12:36                         ` [RFC PATCH v2] drm/radeon: fixup locking inversion between, " Maarten Lankhorst
2013-10-09 10:58                       ` [RFC PATCH] drm/radeon: fixup locking inversion between " Maarten Lankhorst
2013-10-08 14:45               ` Maarten Lankhorst
2013-10-08 14:57                 ` Jerome Glisse
2013-09-13  6:44         ` [BUG] completely bonkers use of set_need_resched + VM_FAULT_NOPAGE Thomas Hellstrom
2013-09-13  6:44           ` Thomas Hellstrom
2013-09-13  7:16           ` Maarten Lankhorst
2013-09-13  7:46             ` Thomas Hellstrom
2013-09-13  7:51               ` Maarten Lankhorst
2013-09-13  8:23                 ` Thomas Hellstrom
2013-09-13  8:23                   ` Thomas Hellstrom
2013-09-13  8:32                   ` Daniel Vetter
2013-09-13  8:39                     ` Thomas Hellstrom
2013-09-13  8:58                   ` Maarten Lankhorst
2013-09-13  9:21                     ` Thomas Hellstrom
2013-09-13  8:29               ` Peter Zijlstra
2013-09-13  8:41                 ` Daniel Vetter
2013-09-13  9:00                   ` Peter Zijlstra
2013-09-23 15:33                     ` [RFC PATCH] drm/nouveau: fix nested locking in mmap handler Maarten Lankhorst
2013-09-24  7:22                       ` Thomas Hellstrom
2013-09-24  7:34                         ` Maarten Lankhorst
2013-09-24  8:20                           ` Ingo Molnar
2013-09-24  9:03                           ` Thomas Hellstrom
2013-09-24  9:36                             ` Daniel Vetter
2013-09-24 10:11                               ` Maarten Lankhorst
2013-09-24 10:33                                 ` Thomas Hellstrom
2013-09-24 11:32                                   ` Maarten Lankhorst
2013-09-24 17:04                                     ` Thomas Hellstrom
2013-09-24  9:43                             ` Maarten Lankhorst
2013-09-24  9:43                               ` Maarten Lankhorst
2013-09-24 17:53                               ` Thomas Hellstrom

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5232288F.4070904@vmware.com \
    --to=thellstrom@vmware.com \
    --cc=airlied@linux.ie \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@canonical.com \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.