From: Rob Clark robdclark@chromium.org
vm_open() is not allowed to fail. Fortunately we are guaranteed that the pages are already pinned, and only need to increment the refcnt. So just increment it directly.
Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") Cc: stable@vger.kernel.org Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 110a9eac2af8..9885ba64127f 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -571,12 +571,20 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); - int ret;
WARN_ON(shmem->base.import_attach);
- ret = drm_gem_shmem_get_pages(shmem); - WARN_ON_ONCE(ret != 0); + mutex_lock(&shmem->pages_lock); + + /* + * We should have already pinned the pages, vm_open() just grabs + * an additional reference for the new mm the vma is getting + * copied into. + */ + WARN_ON_ONCE(!shmem->pages_use_count); + + shmem->pages_use_count++; + mutex_unlock(&shmem->pages_lock);
drm_gem_vm_open(vma); }
On Tue, Nov 29, 2022 at 12:02:42PM -0800, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
vm_open() is not allowed to fail. Fortunately we are guaranteed that the pages are already pinned, and only need to increment the refcnt. So just increment it directly.
I don't know anything about drm or gem, but I am wondering _how_ this would be guaranteed. Would it be through the pin function ? Just wondering, because that function does not seem to be mandatory.
Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") Cc: stable@vger.kernel.org Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 110a9eac2af8..9885ba64127f 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -571,12 +571,20 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
- int ret;
WARN_ON(shmem->base.import_attach);
- ret = drm_gem_shmem_get_pages(shmem);
- WARN_ON_ONCE(ret != 0);
- mutex_lock(&shmem->pages_lock);
- /*
* We should have already pinned the pages, vm_open() just grabs
should or guaranteed ? This sounds a bit weaker than the commit description.
* an additional reference for the new mm the vma is getting* copied into.*/- WARN_ON_ONCE(!shmem->pages_use_count);
- shmem->pages_use_count++;
- mutex_unlock(&shmem->pages_lock);
The previous code, in that situation, would not increment pages_use_count, and it would not set not set shmem->pages. Hopefully, it would not try to do anything with the pages it was unable to get. The new code assumes that shmem->pages is valid even if pages_use_count is 0, while at the same time taking into account that this can possibly happen (or the WARN_ON_ONCE would not be needed).
Again, I don't know anything about gem and drm, but it seems to me that there might now be a severe problem later on if the WARN_ON_ONCE() ever triggers.
Thanks, Guenter
drm_gem_vm_open(vma); }
On Tue, Nov 29, 2022 at 12:32 PM Guenter Roeck linux@roeck-us.net wrote:
On Tue, Nov 29, 2022 at 12:02:42PM -0800, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
vm_open() is not allowed to fail. Fortunately we are guaranteed that the pages are already pinned, and only need to increment the refcnt. So just increment it directly.
I don't know anything about drm or gem, but I am wondering _how_ this would be guaranteed. Would it be through the pin function ? Just wondering, because that function does not seem to be mandatory.
We've pinned the pages already in mmap.. vm->open() is perhaps not the best name for the callback function, but it is called for copying an existing vma into a new process (and for some other cases which do not apply here because VM_DONTEXPAND).
(Other drivers pin pages in the fault handler, where there is actually potential to return an error, but that change was a bit more like re-writing shmem helper ;-))
BR, -R
Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") Cc: stable@vger.kernel.org Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 110a9eac2af8..9885ba64127f 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -571,12 +571,20 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
int ret; WARN_ON(shmem->base.import_attach);ret = drm_gem_shmem_get_pages(shmem);WARN_ON_ONCE(ret != 0);
mutex_lock(&shmem->pages_lock);/** We should have already pinned the pages, vm_open() just grabsshould or guaranteed ? This sounds a bit weaker than the commit description.
* an additional reference for the new mm the vma is getting* copied into.*/WARN_ON_ONCE(!shmem->pages_use_count);shmem->pages_use_count++;mutex_unlock(&shmem->pages_lock);The previous code, in that situation, would not increment pages_use_count, and it would not set not set shmem->pages. Hopefully, it would not try to do anything with the pages it was unable to get. The new code assumes that shmem->pages is valid even if pages_use_count is 0, while at the same time taking into account that this can possibly happen (or the WARN_ON_ONCE would not be needed).
Again, I don't know anything about gem and drm, but it seems to me that there might now be a severe problem later on if the WARN_ON_ONCE() ever triggers.
Thanks, Guenter
drm_gem_vm_open(vma);}
On 11/29/22 12:47, Rob Clark wrote:
On Tue, Nov 29, 2022 at 12:32 PM Guenter Roeck linux@roeck-us.net wrote:
On Tue, Nov 29, 2022 at 12:02:42PM -0800, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
vm_open() is not allowed to fail. Fortunately we are guaranteed that the pages are already pinned, and only need to increment the refcnt. So just increment it directly.
I don't know anything about drm or gem, but I am wondering _how_ this would be guaranteed. Would it be through the pin function ? Just wondering, because that function does not seem to be mandatory.
We've pinned the pages already in mmap.. vm->open() is perhaps not the best name for the callback function, but it is called for copying an existing vma into a new process (and for some other cases which do not apply here because VM_DONTEXPAND).
(Other drivers pin pages in the fault handler, where there is actually potential to return an error, but that change was a bit more like re-writing shmem helper ;-))
Maybe add a bit of that (where the pinning happened) to the commit description and to the patch itself ?
BR, -R
Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") Cc: stable@vger.kernel.org Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 110a9eac2af8..9885ba64127f 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -571,12 +571,20 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
int ret; WARN_ON(shmem->base.import_attach);ret = drm_gem_shmem_get_pages(shmem);WARN_ON_ONCE(ret != 0);
mutex_lock(&shmem->pages_lock);/** We should have already pinned the pages, vm_open() just grabsshould or guaranteed ? This sounds a bit weaker than the commit description.
like ... the pages were already pinned in (mmap function).
* an additional reference for the new mm the vma is getting* copied into.*/WARN_ON_ONCE(!shmem->pages_use_count);
If the code can't be trusted and still needs the warning, how about something like the following ?
if (WARN_ON_ONCE(!shmem->pages_use_count)) { mutex_unlock(&shmem->pages_lock); return; }
Thanks, Guenter
shmem->pages_use_count++;mutex_unlock(&shmem->pages_lock);The previous code, in that situation, would not increment pages_use_count, and it would not set not set shmem->pages. Hopefully, it would not try to do anything with the pages it was unable to get. The new code assumes that shmem->pages is valid even if pages_use_count is 0, while at the same time taking into account that this can possibly happen (or the WARN_ON_ONCE would not be needed).
Again, I don't know anything about gem and drm, but it seems to me that there might now be a severe problem later on if the WARN_ON_ONCE() ever triggers.
Thanks, Guenter
drm_gem_vm_open(vma);}
On Tue, Nov 29, 2022 at 12:47:42PM -0800, Rob Clark wrote:
On Tue, Nov 29, 2022 at 12:32 PM Guenter Roeck linux@roeck-us.net wrote:
On Tue, Nov 29, 2022 at 12:02:42PM -0800, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
vm_open() is not allowed to fail. Fortunately we are guaranteed that the pages are already pinned, and only need to increment the refcnt. So just increment it directly.
I don't know anything about drm or gem, but I am wondering _how_ this would be guaranteed. Would it be through the pin function ? Just wondering, because that function does not seem to be mandatory.
We've pinned the pages already in mmap.. vm->open() is perhaps not the best name for the callback function, but it is called for copying an existing vma into a new process (and for some other cases which do not apply here because VM_DONTEXPAND).
(Other drivers pin pages in the fault handler, where there is actually potential to return an error, but that change was a bit more like re-writing shmem helper ;-))
Yhea vm_ops->open should really be called vm_ops->dupe or ->copy or something like that ... -Daniel
BR, -R
Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") Cc: stable@vger.kernel.org Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 110a9eac2af8..9885ba64127f 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -571,12 +571,20 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
int ret; WARN_ON(shmem->base.import_attach);ret = drm_gem_shmem_get_pages(shmem);WARN_ON_ONCE(ret != 0);
mutex_lock(&shmem->pages_lock);/** We should have already pinned the pages, vm_open() just grabsshould or guaranteed ? This sounds a bit weaker than the commit description.
* an additional reference for the new mm the vma is getting* copied into.*/WARN_ON_ONCE(!shmem->pages_use_count);shmem->pages_use_count++;mutex_unlock(&shmem->pages_lock);The previous code, in that situation, would not increment pages_use_count, and it would not set not set shmem->pages. Hopefully, it would not try to do anything with the pages it was unable to get. The new code assumes that shmem->pages is valid even if pages_use_count is 0, while at the same time taking into account that this can possibly happen (or the WARN_ON_ONCE would not be needed).
Again, I don't know anything about gem and drm, but it seems to me that there might now be a severe problem later on if the WARN_ON_ONCE() ever triggers.
Thanks, Guenter
drm_gem_vm_open(vma);}
On Tue, Nov 29, 2022 at 12:02:42PM -0800, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
vm_open() is not allowed to fail. Fortunately we are guaranteed that the pages are already pinned, and only need to increment the refcnt. So just increment it directly.
Please mention hare that the only issue is the mutex_lock_interruptible, and the only way we've found to hit this is if you send a signal to the original process in a fork() at _just_ the right time.
With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") Cc: stable@vger.kernel.org Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 110a9eac2af8..9885ba64127f 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -571,12 +571,20 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
int ret; WARN_ON(shmem->base.import_attach);ret = drm_gem_shmem_get_pages(shmem);WARN_ON_ONCE(ret != 0);
mutex_lock(&shmem->pages_lock);/** We should have already pinned the pages, vm_open() just grabs* an additional reference for the new mm the vma is getting* copied into.*/WARN_ON_ONCE(!shmem->pages_use_count);shmem->pages_use_count++;mutex_unlock(&shmem->pages_lock); drm_gem_vm_open(vma);}
2.38.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
linux-stable-mirror@lists.linaro.org
-
Daniel Vetter -
Guenter Roeck -
Rob Clark