On 10/9/20 12:33 AM, Christian König wrote:
Am 08.10.20 um 23:49 schrieb John Hubbard:
On 10/8/20 4:23 AM, Christian König wrote:
...
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 3d69e51f3e4d..c9d5f1a38af3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
* requires avoiding extraneous references to their filp, hence why
* we prefer to use an anonymous file for their mmaps.
*/
- fput(vma->vm_file);
- vma->vm_file = anon;
+ vma_set_file(vma, anon);
+ fput(anon);
That's one fput() too many, isn't it?
No, the other cases were replacing the vm_file with something pre-allocated and also grabbed a new
reference.
But this case here uses the freshly allocated anon file and so vma_set_file() grabs another extra
reference which we need to drop.
The alternative is to just keep it as it is. Opinions?
I think just a small comment for these cases, is probably about right.
...
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 10b4be1f3e78..a51dc089896e 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
vma_set_anonymous(vma);
}
- if (vma->vm_file)
- fput(vma->vm_file);
- vma->vm_file = asma->file;
+ vma_set_file(vma, asma->file);
+ fput(asma->file);
Same here: that fput() seems wrong, as it was already done within vma_set_file().
No, that case is correct as well. The Android code here has the matching get_file() a few lines up,
see the surrounding code.
I didn't wanted to replace that since it does some strange error handling here, so the result is
that we need to drop the extra reference as again.
We could also keep it like it is or maybe better put a TODO comment on it.
Yeah, I think a comment is a good way to go.
thanks,
--
John Hubbard
NVIDIA
