[resent, because i got the lkml-adress wrong the first time]
Hi Eric!
To put this mail into context:
http://bugs.freedesktop.org/show_bug.cgi?id=20985
I finally poked a little bit at the code, since i figured you would be
glad if i came up with something.
I think I understood the problem and made a correct patch, but kernel
is new territory for me. So please doublecheck if i made the correct
choices for the error-returns.
Can you make shure that this patch (if acceptable) goes into mainline?
Sincerely,
Florian
p.s.: does somebody know where the actual implementation of
copy_[from/to]_user for my core2duo @64bit is? it is a mistery!
From bce4dab0061fa195360f3b88ce22c44895d3d197 Mon Sep 17 00:00:00 2001
From: Florian Mickler <[email protected]>
Date: Mon, 6 Apr 2009 22:55:41 +0200
Subject: [PATCH] drm/i915: Fix use of uninitialized var in 40a5f0de
i915_gem_put_relocs_to_user returned an uninitialized value which
got returned to userspace. This caused libdrm in my setup to never
get out of a do{}while() loop retrying i915_gem_execbuffer.
result was hanging X, overheating of cpu and 2-3gb of logfile-spam.
This patch adresses the issue by
1. initializing vars in this file where necessary
2. correcting wrongly interpreted return values of copy_[from/to]_user
Signed-off-by: Florian Mickler <[email protected]>
Signed-off-by: Eric Anholt <[email protected]>
---
On Mon, 2009-04-06 at 22:55 +0200, Florian Mickler wrote:
> [resent, because i got the lkml-adress wrong the first time]
> Hi Eric!
>
> To put this mail into context:
> http://bugs.freedesktop.org/show_bug.cgi?id=20985
>
> I finally poked a little bit at the code, since i figured you would be
> glad if i came up with something.
>
> I think I understood the problem and made a correct patch, but kernel
> is new territory for me. So please doublecheck if i made the correct
> choices for the error-returns.
>
> Can you make shure that this patch (if acceptable) goes into mainline?
Nice catch! Thanks. I did some cleanup that brings it more in line
with style elsewhere in the code and cuts some of the gratuitous looking
changes. Would you be OK with these changes rolled into your original
diff?
drivers/gpu/drm/i915/i915_gem.c | 34 ++++++++++++++++++++++------------
1 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 33ab07b..6f7d0e2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -141,15 +141,18 @@ fast_shmem_read(struct page **pages,
int length)
{
char __iomem *vaddr;
- int ret;
+ int unwritten;
vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT], KM_USER0);
if (vaddr == NULL)
return -ENOMEM;
- ret = __copy_to_user_inatomic(data, vaddr + page_offset, length);
+ unwritten = __copy_to_user_inatomic(data, vaddr + page_offset, length);
kunmap_atomic(vaddr, KM_USER0);
- return ret;
+ if (unwritten)
+ return -EFAULT;
+
+ return 0;
}
static inline int
@@ -3000,13 +3003,13 @@ i915_gem_get_relocs_from_user(struct drm_i915_gem_exec_object *exec_list,
drm_free(*relocs, reloc_count * sizeof(**relocs),
DRM_MEM_DRIVER);
*relocs = NULL;
- return ret;
+ return -EFAULT;
}
reloc_index += exec_list[i].relocation_count;
}
- return ret;
+ return 0;
}
static int
@@ -3015,23 +3018,28 @@ i915_gem_put_relocs_to_user(struct drm_i915_gem_exec_object *exec_list,
struct drm_i915_gem_relocation_entry *relocs)
{
uint32_t reloc_count = 0, i;
- int ret;
+ int ret = 0;
for (i = 0; i < buffer_count; i++) {
struct drm_i915_gem_relocation_entry __user *user_relocs;
+ int unwritten;
user_relocs = (void __user *)(uintptr_t)exec_list[i].relocs_ptr;
- if (ret == 0) {
- ret = copy_to_user(user_relocs,
- &relocs[reloc_count],
- exec_list[i].relocation_count *
- sizeof(*relocs));
+ unwritten = copy_to_user(user_relocs,
+ &relocs[reloc_count],
+ exec_list[i].relocation_count *
+ sizeof(*relocs));
+
+ if (unwritten) {
+ ret = -EFAULT;
+ goto err;
}
reloc_count += exec_list[i].relocation_count;
}
+err:
drm_free(relocs, reloc_count * sizeof(*relocs), DRM_MEM_DRIVER);
return ret;
@@ -3306,10 +3314,12 @@ err:
(uintptr_t) args->buffers_ptr,
exec_list,
sizeof(*exec_list) * args->buffer_count);
- if (ret)
+ if (ret) {
+ ret = -EFAULT;
DRM_ERROR("failed to copy %d exec entries "
"back to user (%d)\n",
args->buffer_count, ret);
+ }
}
/* Copy the updated relocations out regardless of current error
--
1.6.2.1
--
Eric Anholt
[email protected] [email protected]
On Mon, 06 Apr 2009 19:03:55 -0700
Eric Anholt <[email protected]> wrote:
> Nice catch! Thanks. I did some cleanup that brings it more in line
> with style elsewhere in the code and cuts some of the gratuitous
> looking changes. Would you be OK with these changes rolled into your
> original diff?
i take it, you appended the endresult?
i'm ok with it, it's less invasive. but i think your
i915_gem_put_relocs part is wrong. (see below)
>
> drivers/gpu/drm/i915/i915_gem.c | 34
> ++++++++++++++++++++++------------ 1 files changed, 22 insertions(+),
> 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 33ab07b..6f7d0e2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -141,15 +141,18 @@ fast_shmem_read(struct page **pages,
> int length)
> {
> char __iomem *vaddr;
> - int ret;
> + int unwritten;
>
> vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT],
> KM_USER0); if (vaddr == NULL)
> return -ENOMEM;
> - ret = __copy_to_user_inatomic(data, vaddr + page_offset,
> length);
> + unwritten = __copy_to_user_inatomic(data, vaddr +
> page_offset, length); kunmap_atomic(vaddr, KM_USER0);
>
> - return ret;
> + if (unwritten)
> + return -EFAULT;
> +
> + return 0;
> }
yep thats ok.
>
> static inline int
> @@ -3000,13 +3003,13 @@ i915_gem_get_relocs_from_user(struct
> drm_i915_gem_exec_object *exec_list, drm_free(*relocs, reloc_count *
> sizeof(**relocs), DRM_MEM_DRIVER);
> *relocs = NULL;
> - return ret;
> + return -EFAULT;
> }
>
> reloc_index += exec_list[i].relocation_count;
> }
>
> - return ret;
> + return 0;
> }
>
right.
> static int
> @@ -3015,23 +3018,28 @@ i915_gem_put_relocs_to_user(struct
> drm_i915_gem_exec_object *exec_list, struct
> drm_i915_gem_relocation_entry *relocs) {
> uint32_t reloc_count = 0, i;
> - int ret;
> + int ret = 0;
>
> for (i = 0; i < buffer_count; i++) {
> struct drm_i915_gem_relocation_entry __user
> *user_relocs;
> + int unwritten;
>
> user_relocs = (void __user
> *)(uintptr_t)exec_list[i].relocs_ptr;
> - if (ret == 0) {
> - ret = copy_to_user(user_relocs,
> - &relocs[reloc_count],
> -
> exec_list[i].relocation_count *
> - sizeof(*relocs));
> + unwritten = copy_to_user(user_relocs,
> + &relocs[reloc_count],
> +
> exec_list[i].relocation_count *
> + sizeof(*relocs));
> +
> + if (unwritten) {
> + ret = -EFAULT;
> + goto err;
> }
>
> reloc_count += exec_list[i].relocation_count;
> }
>
i wondered too at first about the if (ret == 0) part, but you need the
whole reloc_count to free everything in the next part:
> +err:
> drm_free(relocs, reloc_count * sizeof(*relocs),
> DRM_MEM_DRIVER);
> return ret;
so i think, this would be a memleak in the error-case (if it ever
happens)
> @@ -3306,10 +3314,12 @@ err:
> (uintptr_t) args->buffers_ptr,
> exec_list,
> sizeof(*exec_list) *
> args->buffer_count);
> - if (ret)
> + if (ret) {
> + ret = -EFAULT;
> DRM_ERROR("failed to copy %d exec entries "
> "back to user (%d)\n",
> args->buffer_count, ret);
> + }
> }
>
> /* Copy the updated relocations out regardless of current
> error
On Tue, 2009-04-07 at 09:23 +0200, Florian Mickler wrote:
> On Mon, 06 Apr 2009 19:03:55 -0700
> Eric Anholt <[email protected]> wrote:
>
> > Nice catch! Thanks. I did some cleanup that brings it more in line
> > with style elsewhere in the code and cuts some of the gratuitous
> > looking changes. Would you be OK with these changes rolled into your
> > original diff?
>
> i take it, you appended the endresult?
>
> i'm ok with it, it's less invasive. but i think your
> i915_gem_put_relocs part is wrong. (see below)
>
>
> >
> > drivers/gpu/drm/i915/i915_gem.c | 34
> > ++++++++++++++++++++++------------ 1 files changed, 22 insertions(+),
> > 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 33ab07b..6f7d0e2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -141,15 +141,18 @@ fast_shmem_read(struct page **pages,
> > int length)
> > {
> > char __iomem *vaddr;
> > - int ret;
> > + int unwritten;
> >
> > vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT],
> > KM_USER0); if (vaddr == NULL)
> > return -ENOMEM;
> > - ret = __copy_to_user_inatomic(data, vaddr + page_offset,
> > length);
> > + unwritten = __copy_to_user_inatomic(data, vaddr +
> > page_offset, length); kunmap_atomic(vaddr, KM_USER0);
> >
> > - return ret;
> > + if (unwritten)
> > + return -EFAULT;
> > +
> > + return 0;
> > }
>
> yep thats ok.
>
> >
> > static inline int
> > @@ -3000,13 +3003,13 @@ i915_gem_get_relocs_from_user(struct
> > drm_i915_gem_exec_object *exec_list, drm_free(*relocs, reloc_count *
> > sizeof(**relocs), DRM_MEM_DRIVER);
> > *relocs = NULL;
> > - return ret;
> > + return -EFAULT;
> > }
> >
> > reloc_index += exec_list[i].relocation_count;
> > }
> >
> > - return ret;
> > + return 0;
> > }
> >
>
> right.
>
> > static int
> > @@ -3015,23 +3018,28 @@ i915_gem_put_relocs_to_user(struct
> > drm_i915_gem_exec_object *exec_list, struct
> > drm_i915_gem_relocation_entry *relocs) {
> > uint32_t reloc_count = 0, i;
> > - int ret;
> > + int ret = 0;
> >
> > for (i = 0; i < buffer_count; i++) {
> > struct drm_i915_gem_relocation_entry __user
> > *user_relocs;
> > + int unwritten;
> >
> > user_relocs = (void __user
> > *)(uintptr_t)exec_list[i].relocs_ptr;
> > - if (ret == 0) {
> > - ret = copy_to_user(user_relocs,
> > - &relocs[reloc_count],
> > -
> > exec_list[i].relocation_count *
> > - sizeof(*relocs));
> > + unwritten = copy_to_user(user_relocs,
> > + &relocs[reloc_count],
> > +
> > exec_list[i].relocation_count *
> > + sizeof(*relocs));
> > +
> > + if (unwritten) {
> > + ret = -EFAULT;
> > + goto err;
> > }
> >
> > reloc_count += exec_list[i].relocation_count;
> > }
> >
>
> i wondered too at first about the if (ret == 0) part, but you need the
> whole reloc_count to free everything in the next part:
>
> > +err:
> > drm_free(relocs, reloc_count * sizeof(*relocs),
> > DRM_MEM_DRIVER);
> > return ret;
>
>
> so i think, this would be a memleak in the error-case (if it ever
> happens)
drm_free's other arguments are unused memory debug leftovers. I've got
a patch I need to push at airlied to remove
drm_malloc/drm_calloc/drm_free.
--
Eric Anholt
[email protected] [email protected]
On Tue, 07 Apr 2009 09:21:40 -0700
Eric Anholt <[email protected]> wrote:
> drm_free's other arguments are unused memory debug leftovers. I've
> got a patch I need to push at airlied to remove
> drm_malloc/drm_calloc/drm_free.
>
in that case it is of course a non issue. but would you mind
to add a note like 'this adds a memleak to i915_gem_put_relocs_to_user
which will be fixed in a followup patch', or just rebase it onto that
patch?
anyhow, below patch is what i'm currently running.. if you rebase
the other version onto that patch of your's removing the
allocation thats fine too. if not, probably ok too. it's just that i
have too much time to care about this :)
sincerely,
Florian
p.s.: thx for doing this great work!
From 868c938874d66aa3e507d8312cefc7730487f442 Mon Sep 17 00:00:00 2001
From: Florian Mickler <[email protected]>
Date: Tue, 7 Apr 2009 18:41:32 +0200
Subject: [PATCH] Fix: use of uninitialized var
i915_gem_put_relocs_to_user returned an uninitialized value which
got returned to userspace. This caused libdrm in my setup to never
get out of a do{}while() loop retrying i915_gem_execbuffer.
result was hanging X, overheating of cpu and 2-3gb of logfile-spam.
This patch adresses the issue by
1. initializing vars in this file where necessary
2. correcting wrongly interpreted return values of
copy_[from/to]_user
Nr. 2 helps libdrm from getting out of its loop and Nr. 1 helps
i915_gem_execbuffer to not think there was an error.
Signed-off-by: Florian Mickler <[email protected]>
---
drivers/gpu/drm/i915/i915_gem.c | 22 +++++++++++++++-------
1 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c index 1449b45..e73844a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -143,15 +143,18 @@ fast_shmem_read(struct page **pages,
int length)
{
char __iomem *vaddr;
- int ret;
+ int unwritten;
vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT], KM_USER0);
if (vaddr == NULL)
return -ENOMEM;
- ret = __copy_to_user_inatomic(data, vaddr + page_offset,
length);
+ unwritten = __copy_to_user_inatomic(data, vaddr + page_offset,
length); kunmap_atomic(vaddr, KM_USER0);
- return ret;
+ if (unwritten)
+ return -EFAULT;
+
+ return 0;
}
static inline int
@@ -3002,13 +3005,13 @@ i915_gem_get_relocs_from_user(struct
drm_i915_gem_exec_object *exec_list, drm_free(*relocs, reloc_count *
sizeof(**relocs), DRM_MEM_DRIVER);
*relocs = NULL;
- return ret;
+ return -EFAULT;
}
reloc_index += exec_list[i].relocation_count;
}
- return ret;
+ return 0;
}
static int
@@ -3017,7 +3020,7 @@ i915_gem_put_relocs_to_user(struct
drm_i915_gem_exec_object *exec_list, struct
drm_i915_gem_relocation_entry *relocs) {
uint32_t reloc_count = 0, i;
- int ret;
+ int ret = 0;
for (i = 0; i < buffer_count; i++) {
struct drm_i915_gem_relocation_entry __user
*user_relocs; @@ -3036,6 +3039,9 @@ i915_gem_put_relocs_to_user(struct
drm_i915_gem_exec_object *exec_list,
drm_free(relocs, reloc_count * sizeof(*relocs),
DRM_MEM_DRIVER);
+ if (ret != 0)
+ ret = -EFAULT;
+
return ret;
}
@@ -3308,10 +3314,12 @@ err:
(uintptr_t) args->buffers_ptr,
exec_list,
sizeof(*exec_list) *
args->buffer_count);
- if (ret)
+ if (ret) {
+ ret = -EFAULT;
DRM_ERROR("failed to copy %d exec entries "
"back to user (%d)\n",
args->buffer_count, ret);
+ }
}
/* Copy the updated relocations out regardless of current error
--
1.6.2
On Tue, 2009-04-07 at 22:14 +0200, Florian Mickler wrote:
> On Tue, 07 Apr 2009 09:21:40 -0700
> Eric Anholt <[email protected]> wrote:
>
>
> > drm_free's other arguments are unused memory debug leftovers. I've
> > got a patch I need to push at airlied to remove
> > drm_malloc/drm_calloc/drm_free.
> >
> in that case it is of course a non issue. but would you mind
> to add a note like 'this adds a memleak to i915_gem_put_relocs_to_user
> which will be fixed in a followup patch', or just rebase it onto that
> patch?
Just to be clear, there is no memleak:
/** Wrapper around kfree() */
static __inline__ void drm_free(void *pt, size_t size, int area)
{
kfree(pt);
}
The arg would only get used if DRM_DEBUG_MEMORY was set, but there's no
way in the kernel to do so. I don't think anybody's used it in years,
and I'm sure there would be broken drm_free arguments since it's
untested. It was never very useful even back in the day, since most
everything ended up lumped under DRM_MEM_DRIVER.
--
Eric Anholt
[email protected] [email protected]
On Tue, 07 Apr 2009 19:33:59 -0700
Eric Anholt <[email protected]> wrote:
> On Tue, 2009-04-07 at 22:14 +0200, Florian Mickler wrote:
> > On Tue, 07 Apr 2009 09:21:40 -0700
> > Eric Anholt <[email protected]> wrote:
> >
> >
> > > drm_free's other arguments are unused memory debug leftovers.
> > > I've got a patch I need to push at airlied to remove
> > > drm_malloc/drm_calloc/drm_free.
> > >
> > in that case it is of course a non issue. but would you mind
> > to add a note like 'this adds a memleak to
> > i915_gem_put_relocs_to_user which will be fixed in a followup
> > patch', or just rebase it onto that patch?
>
> Just to be clear, there is no memleak:
>
> /** Wrapper around kfree() */
> static __inline__ void drm_free(void *pt, size_t size, int area)
> {
> kfree(pt);
> }
>
> The arg would only get used if DRM_DEBUG_MEMORY was set, but there's
> no way in the kernel to do so. I don't think anybody's used it in
> years, and I'm sure there would be broken drm_free arguments since
> it's untested. It was never very useful even back in the day, since
> most everything ended up lumped under DRM_MEM_DRIVER.
>
wtf? alright :)
thx for clarifying. i should have just looked.
bye,
Florian