linux-kernel - Re: [PATCH 1/4] mm: introduce vma_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Oct 2020 09:33:04 +0200
From:   Christian König <ckoenig.leichtzumerken@...il.com>
To:     John Hubbard <jhubbard@...dia.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, linaro-mm-sig@...ts.linaro.org,
        dri-devel@...ts.freedesktop.org, linux-media@...r.kernel.org,
        chris@...is-wilson.co.uk, airlied@...hat.com,
        akpm@...ux-foundation.org, daniel@...ll.ch, sumit.semwal@...aro.org
Subject: Re: [PATCH 1/4] mm: introduce vma_set_file function v2

Am 08.10.20 um 23:49 schrieb John Hubbard:
> On 10/8/20 4:23 AM, Christian König wrote:
>> Add the new vma_set_file() function to allow changing
>> vma->vm_file with the necessary refcount dance.
>>
>> v2: add more users of this.
>>
>> Signed-off-by: Christian König <christian.koenig@....com>
>> ---
>>   drivers/dma-buf/dma-buf.c                  | 16 +++++-----------
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c      |  4 +---
>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  3 +--
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
>>   drivers/gpu/drm/msm/msm_gem.c              |  4 +---
>>   drivers/gpu/drm/omapdrm/omap_gem.c         |  3 +--
>>   drivers/gpu/drm/vgem/vgem_drv.c            |  3 +--
>>   drivers/staging/android/ashmem.c           |  5 ++---
>>   include/linux/mm.h                         |  2 ++
>>   mm/mmap.c                                  | 16 ++++++++++++++++
>>   10 files changed, 32 insertions(+), 28 deletions(-)
>
> Looks like a nice cleanup. Two comments below.
>
> ...
>
>> 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?

>
>
> ...
>
>> 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.

Regards,
Christian.

>
>
>
> thanks,