[PATCH 1/2] io-mapping: ensure io_mapping_map_atomic _is_ atomic

[PATCH 1/2] io-mapping: ensure io_mapping_map_atomic _is_ atomic

[Daniel Vetter]

For the !HAVE_ATOMIC_IOMAP case the stub functions did not call
pagefault_disable/_enable. The i915 driver relies on the map
actually being atomic, otherwise it can deadlock with it's own
pagefault handler in the gtt pwrite fastpath.

This is exercised by gem_mmap_gtt from the intel-gpu-toosl gem
testsuite.

v2: Chris Wilson noted the lack of an include.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115
Cc: stable@kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 include/linux/io-mapping.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/io-mapping.h b/include/linux/io-mapping.h
index 8cdcc2a1..1feeb52 100644
--- a/include/linux/io-mapping.h
+++ b/include/linux/io-mapping.h
@@ -117,6 +117,8 @@ io_mapping_unmap(void __iomem *vaddr)
 
 #else
 
+#include <linux/uaccess.h>
+
 /* this struct isn't actually defined anywhere */
 struct io_mapping;
 
@@ -138,12 +140,14 @@ static inline void __iomem *
 io_mapping_map_atomic_wc(struct io_mapping *mapping,
 			 unsigned long offset)
 {
+	pagefault_disable();
 	return ((char __force __iomem *) mapping) + offset;
 }
 
 static inline void
 io_mapping_unmap_atomic(void __iomem *vaddr)
 {
+	pagefault_enable();
 }
 
 /* Non-atomic map/unmap */
-- 
1.7.6.2

[PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Daniel Vetter]

The helper functions used are designed for pagecache io and splice,
i.e. they prefault at most PAGE_SIZE bytes spanning at most 2 pages.

pread/pwrite want to write/read much more to avoid dropping the
struct_mutex lock in between. So write our helper function to prefault.
We're the only user of these pagemap.h helpers that want this behaviour,
so keep these new helpers private.

Based on a patch by Chris Wilson. In addition to his approach this
alos tries to prefault the last page in case the user address range
crosses a page boundary at the end (and hence might sit on n+1 pages
for at most n*PAGE_SIZE of date).

As a nice side-effect this rather reliably papers over the current
code's inability to handle non-struct page-backed user memory in the
slow paths. Because the real fix is grossly invasive, I think this
patch is the right thing for backporting.

Cc: stable@kernel.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |   59 ++++++++++++++++++++++++++++++++++++--
 1 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5f0f46e..42dc922 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -503,6 +503,32 @@ out:
 	return ret;
 }
 
+static inline int __prefault_writeable(char __user *uaddr, int size)
+{
+	int ret;
+	char __user *end = uaddr + size - 1;
+
+	if (unlikely(size == 0))
+		return 0;
+
+	/*
+	 * Writing zeroes into userspace here is OK, because we know that if
+	 * the zero gets there, we'll be overwriting it.
+	 */
+	while (uaddr <= end) {
+		ret = __put_user(0, uaddr);
+		if (ret != 0)
+			return ret;
+		uaddr += PAGE_SIZE;
+	}
+	if (ret == 0) {
+		if (((unsigned long)uaddr & PAGE_MASK) !=
+				((unsigned long)end & PAGE_MASK))
+		 	ret = __put_user(0, end);
+	}
+	return ret;
+}
+
 /**
  * Reads data from the object referenced by handle.
  *
@@ -524,8 +550,8 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
 		       args->size))
 		return -EFAULT;
 
-	ret = fault_in_pages_writeable((char __user *)(uintptr_t)args->data_ptr,
-				       args->size);
+	ret = __prefault_writeable((char __user *)(uintptr_t)args->data_ptr,
+				   args->size);
 	if (ret)
 		return -EFAULT;
 
@@ -943,6 +969,31 @@ out:
 	return ret;
 }
 
+static inline int __prefault_readable(const char __user *uaddr, int size)
+{
+	volatile char c;
+	int ret;
+	const char __user *end = uaddr + size - 1;
+
+	if (unlikely(size == 0))
+		return 0;
+
+	while (uaddr <= end) {
+		ret = __get_user(c, uaddr);
+		if (ret != 0)
+			return ret;
+		uaddr += PAGE_SIZE;
+	}
+	if (ret == 0) {
+		if (((unsigned long)uaddr & PAGE_MASK) !=
+				((unsigned long)end & PAGE_MASK)) {
+		 	ret = __get_user(c, end);
+			(void)c;
+		}
+	}
+	return ret;
+}
+
 /**
  * Writes data to the object referenced by handle.
  *
@@ -964,8 +1015,8 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		       args->size))
 		return -EFAULT;
 
-	ret = fault_in_pages_readable((char __user *)(uintptr_t)args->data_ptr,
-				      args->size);
+	ret = __prefault_readable((char __user *)(uintptr_t)args->data_ptr,
+				  args->size);
 	if (ret)
 		return -EFAULT;
 
-- 
1.7.6.2

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Chris Wilson]

On Wed, 28 Sep 2011 11:57:24 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The helper functions used are designed for pagecache io and splice,
> i.e. they prefault at most PAGE_SIZE bytes spanning at most 2 pages.
> 
> pread/pwrite want to write/read much more to avoid dropping the
> struct_mutex lock in between. So write our helper function to prefault.
> We're the only user of these pagemap.h helpers that want this behaviour,
> so keep these new helpers private.
> 
> Based on a patch by Chris Wilson. In addition to his approach this
> alos tries to prefault the last page in case the user address range
> crosses a page boundary at the end (and hence might sit on n+1 pages
> for at most n*PAGE_SIZE of date).
> 
> As a nice side-effect this rather reliably papers over the current
> code's inability to handle non-struct page-backed user memory in the
> slow paths. Because the real fix is grossly invasive, I think this
> patch is the right thing for backporting.
> 
> Cc: stable@kernel.org
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   59 ++++++++++++++++++++++++++++++++++++--
>  1 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5f0f46e..42dc922 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -503,6 +503,32 @@ out:
>  	return ret;
>  }
>  
> +static inline int __prefault_writeable(char __user *uaddr, int size)
> +{
> +	int ret;
> +	char __user *end = uaddr + size - 1;
> +
> +	if (unlikely(size == 0))
> +		return 0;
> +
> +	/*
> +	 * Writing zeroes into userspace here is OK, because we know that if
> +	 * the zero gets there, we'll be overwriting it.
> +	 */
> +	while (uaddr <= end) {
> +		ret = __put_user(0, uaddr);
> +		if (ret != 0)
> +			return ret;
> +		uaddr += PAGE_SIZE;
> +	}
> +	if (ret == 0) {
> +		if (((unsigned long)uaddr & PAGE_MASK) !=
> +				((unsigned long)end & PAGE_MASK))
This is a little ugly. Perhaps,

  #define page_align(addr) ((unsigned long)addr & PAGE_MASK)
  if (page_align(uaddr) != page_align(end))

Otherwise, both are
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Daniel Vetter]

Hi Keith,

This patch isn't in your -next pull. This papers over a spurious -EFAULT
in the pwrite/pread paths that actually gets hit in the wild. The real fix
in the form of a almost complete rewrite of the pwrite/pread paths won't
be ready for 3.2.

Do you want me to implement Chris' beautification suggestion? The code
as-is is copied over from the pagecache prefault helper. Otherwise please
consider merging for 3.2.

Yours, Daniel

On Wed, Sep 28, 2011 at 12:24:04PM +0100, Chris Wilson wrote:
> > +	if (ret == 0) {
> > +		if (((unsigned long)uaddr & PAGE_MASK) !=
> > +				((unsigned long)end & PAGE_MASK))
> This is a little ugly. Perhaps,
> 
>   #define page_align(addr) ((unsigned long)addr & PAGE_MASK)
>   if (page_align(uaddr) != page_align(end))
> 
> Otherwise, both are
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Daniel Vetter]

Oosp, forgotten to actually put Keith on the To: ... not enough coffee,
yet.
-Daniel

On Sun, Oct 23, 2011 at 12:18:30PM +0200, Daniel Vetter wrote:
> Hi Keith,
> 
> This patch isn't in your -next pull. This papers over a spurious -EFAULT
> in the pwrite/pread paths that actually gets hit in the wild. The real fix
> in the form of a almost complete rewrite of the pwrite/pread paths won't
> be ready for 3.2.
> 
> Do you want me to implement Chris' beautification suggestion? The code
> as-is is copied over from the pagecache prefault helper. Otherwise please
> consider merging for 3.2.
> 
> Yours, Daniel
> 
> On Wed, Sep 28, 2011 at 12:24:04PM +0100, Chris Wilson wrote:
> > > +	if (ret == 0) {
> > > +		if (((unsigned long)uaddr & PAGE_MASK) !=
> > > +				((unsigned long)end & PAGE_MASK))
> > This is a little ugly. Perhaps,
> > 
> >   #define page_align(addr) ((unsigned long)addr & PAGE_MASK)
> >   if (page_align(uaddr) != page_align(end))
> > 
> > Otherwise, both are
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > -Chris
> > 
> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
> 
> -- 
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 609 bytes --]

On Sun, 23 Oct 2011 12:18:30 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> Hi Keith,
> 
> This patch isn't in your -next pull. This papers over a spurious -EFAULT
> in the pwrite/pread paths that actually gets hit in the wild. The real fix
> in the form of a almost complete rewrite of the pwrite/pread paths won't
> be ready for 3.2.

We had several comments wondering whether writing zeros was OK as this
occurs before some potential error returns that should leave the buffer
unmodified. I didn't have a better suggestion, but that seems pretty
sketchy to me.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Daniel Vetter]

On Sun, Oct 23, 2011 at 12:23:47PM -0700, Keith Packard wrote:
> On Sun, 23 Oct 2011 12:18:30 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > Hi Keith,
> > 
> > This patch isn't in your -next pull. This papers over a spurious -EFAULT
> > in the pwrite/pread paths that actually gets hit in the wild. The real fix
> > in the form of a almost complete rewrite of the pwrite/pread paths won't
> > be ready for 3.2.
> 
> We had several comments wondering whether writing zeros was OK as this
> occurs before some potential error returns that should leave the buffer
> unmodified. I didn't have a better suggestion, but that seems pretty
> sketchy to me.

This patch only fixes things up so that we prefault the entire page range
and not just the first PAGE_SIZE bytes (i.e. at most 2 pages). So I don't
see the risk of extending the current behaviour to all pages. Userspace
can already see these zero writes, but only when doing something stupid.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Mon, 24 Oct 2011 00:11:57 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> This patch only fixes things up so that we prefault the entire page range
> and not just the first PAGE_SIZE bytes (i.e. at most 2 pages). So I don't
> see the risk of extending the current behaviour to all pages. Userspace
> can already see these zero writes, but only when doing something stupid.

When we posted a patch to instead fix fault_in_pages_writeable, Andrew
complained that we'd have modified memory even on a short read, which
wasn't considered polite. Could we read/write the same value and avoid
that problem?

Also, we should be fixing fault_in_pages_* going forward, rather than
kludging in more code. And, we'd get to remove the version in ntfs,
which should end in a patch that removes more code than it adds...

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 857 bytes --]

On Mon, 24 Oct 2011 00:11:57 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> This patch only fixes things up so that we prefault the entire page range
> and not just the first PAGE_SIZE bytes (i.e. at most 2 pages). So I don't
> see the risk of extending the current behaviour to all pages. Userspace
> can already see these zero writes, but only when doing something stupid.

When we posted a patch to instead fix fault_in_pages_writeable, Andrew
complained that we'd have modified memory even on a short read, which
wasn't considered polite. Could we read/write the same value and avoid
that problem?

Also, we should be fixing fault_in_pages_* going forward, rather than
kludging in more code. And, we'd get to remove the version in ntfs,
which should end in a patch that removes more code than it adds...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Daniel Vetter]

On Thu, Nov 03, 2011 at 02:06:55PM -0700, Keith Packard wrote:
> On Mon, 24 Oct 2011 00:11:57 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > This patch only fixes things up so that we prefault the entire page range
> > and not just the first PAGE_SIZE bytes (i.e. at most 2 pages). So I don't
> > see the risk of extending the current behaviour to all pages. Userspace
> > can already see these zero writes, but only when doing something stupid.
> 
> When we posted a patch to instead fix fault_in_pages_writeable, Andrew
> complained that we'd have modified memory even on a short read, which
> wasn't considered polite. Could we read/write the same value and avoid
> that problem?

Hm, that might be a solution. My current plan was to ditch the prefault
for writing to userspace and beat my pwrite/pread patches into shape for
submission - the bug report only concerns -EFAULT due to handing in a gtt
mapping in pwrite, afaik.

otoh gem objects never change their size and we return -EINVAL if the read
would go past the end of it. And userspace should also never see short
reads due to signals, because the libdrm ioctl automatically restarts the
syscall - and that part is more or less abi. So in practice for our case,
I think it just doesn't matter because userspace really only sees these
zero writes when doing something buggy.

> Also, we should be fixing fault_in_pages_* going forward, rather than
> kludging in more code. And, we'd get to remove the version in ntfs,
> which should end in a patch that removes more code than it adds...

Hm, haven't noticed the version in nfs. The version in pagemap.h does what
all the other users of it want, namely prefault at most PAGE_SIZE bytes
(from at most two pages, in case the user pointer crosses a page boundary).
Which is why I've left it as is.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Daniel Vetter]

On Thu, Nov 03, 2011 at 02:06:55PM -0700, Keith Packard wrote:
> On Mon, 24 Oct 2011 00:11:57 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > This patch only fixes things up so that we prefault the entire page range
> > and not just the first PAGE_SIZE bytes (i.e. at most 2 pages). So I don't
> > see the risk of extending the current behaviour to all pages. Userspace
> > can already see these zero writes, but only when doing something stupid.
> 
> When we posted a patch to instead fix fault_in_pages_writeable, Andrew
> complained that we'd have modified memory even on a short read, which
> wasn't considered polite. Could we read/write the same value and avoid
> that problem?

Hm, that might be a solution. My current plan was to ditch the prefault
for writing to userspace and beat my pwrite/pread patches into shape for
submission - the bug report only concerns -EFAULT due to handing in a gtt
mapping in pwrite, afaik.

otoh gem objects never change their size and we return -EINVAL if the read
would go past the end of it. And userspace should also never see short
reads due to signals, because the libdrm ioctl automatically restarts the
syscall - and that part is more or less abi. So in practice for our case,
I think it just doesn't matter because userspace really only sees these
zero writes when doing something buggy.

> Also, we should be fixing fault_in_pages_* going forward, rather than
> kludging in more code. And, we'd get to remove the version in ntfs,
> which should end in a patch that removes more code than it adds...

Hm, haven't noticed the version in nfs. The version in pagemap.h does what
all the other users of it want, namely prefault at most PAGE_SIZE bytes
(from at most two pages, in case the user pointer crosses a page boundary).
Which is why I've left it as is.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 495 bytes --]

On Wed, 28 Sep 2011 11:57:24 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> +	char __user *end = uaddr + size - 1;

...

> +	if (ret == 0) {
> +		if (((unsigned long)uaddr & PAGE_MASK) !=
> +				((unsigned long)end & PAGE_MASK))
> +		 	ret = __put_user(0, end);
> +	}

This is wrong -- if size == PAGE_SIZE, then we'll be doing an extra
write at 'end' every time (and, I imagine that's a common case).

And, you mentioned a 'better' fix?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Daniel Vetter]

On Thu, Nov 17, 2011 at 21:52, Keith Packard <keithp@keithp.com> wrote:
> On Wed, 28 Sep 2011 11:57:24 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
>> +     char __user *end = uaddr + size - 1;
>
> ...
>
>> +     if (ret == 0) {
>> +             if (((unsigned long)uaddr & PAGE_MASK) !=
>> +                             ((unsigned long)end & PAGE_MASK))
>> +                     ret = __put_user(0, end);
>> +     }
>
> This is wrong -- if size == PAGE_SIZE, then we'll be doing an extra
> write at 'end' every time (and, I imagine that's a common case).

We want to prefault the last byte if the pfn of the last prefault
address doesn't match the pfn of the last byte of the userspace
address range. Only happens when userspace hands in badly aligned
address, not every time. I've rechecked and I think the code actually
does what I want it to do.


> And, you mentioned a 'better' fix?

Chris was despised at the lack of beauty of the code and I agree. Due
to the ppgtt trip to Poland I haven't gotten around to do it actually.
Actually I've just noticed that this is might be the old pwrite/pread
series (mail here still sucks). The new one fixes up the prefault
helpers in pagemap.h (instead of reinventing the wheel for i915.ko),
but they have the same issue of profound ugliness.
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1119 bytes --]

On Fri, 18 Nov 2011 10:12:38 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> We want to prefault the last byte if the pfn of the last prefault
> address doesn't match the pfn of the last byte of the userspace
> address range. Only happens when userspace hands in badly aligned
> address, not every time. I've rechecked and I think the code actually
> does what I want it to do.

Sorry, I was confused by PAGE_MASK (again); assumed it was 0xfff instead
of ~0xfff.

> Chris was despised at the lack of beauty of the code and I agree. Due
> to the ppgtt trip to Poland I haven't gotten around to do it actually.
> Actually I've just noticed that this is might be the old pwrite/pread
> series (mail here still sucks). The new one fixes up the prefault
> helpers in pagemap.h (instead of reinventing the wheel for i915.ko),
> but they have the same issue of profound ugliness.

Right, that was in a different mail thread and is still awaiting
cleanups it seems. Sounds like you're having adventures in Poland at
least. I'll pend this until you've gotten it finished and ready to
merge.

--
keith.packard@intel.com


[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx