Message ID | 20221129200242.298120-3-robdclark@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/shmem-helper: Fix a couple of error path bugs | expand |
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 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 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 grabs >> >> should 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 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: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
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); }