Re: [PATCH] ovl: restore vma->vm_file to old file

["Christian König" <christian.koenig@amd.com>]

Am 21.04.21 um 11:47 schrieb Miklos Szeredi:
> On Tue, Apr 20, 2021 at 4:08 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>> In the error case of ->mmap() we should also restore vma->vm_file
>> to old file in order to keep correct file reference in error path.
>>
>> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>> ---
>>   fs/overlayfs/file.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
>> index 6e454a294046..046a7adb02c5 100644
>> --- a/fs/overlayfs/file.c
>> +++ b/fs/overlayfs/file.c
>> @@ -439,6 +439,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
>>          if (ret) {
>>                  /* Drop reference count from new vm_file value */
>>                  fput(realfile);
>> +               vma->vm_file = file;
> That's interesting: commit 1527f926fd04 ("mm: mmap: fix fput in error
> path v2") which went into 5.11-rc1 seems to have broke the refcounting
> in overlayfs in the name of cleaning up a workaround.   Wondering if
> there's any other damage done by this "fix"?

Can you give wider context? In other words why did the patch broke the 
reference counting in overlayfs?

> Changing refcounting rules in core kernel is no easy matter, a full
> audit of ->mmap instances (>200) should have been done beforehand.

Which is pretty much what was done, see the follow up commit:

commit 295992fb815e791d14b18ef7cdbbaf1a76211a31 (able/vma_file)
Author: Christian König <christian.koenig@amd.com>
Date:   Mon Sep 14 15:09:33 2020 +0200

     mm: introduce vma_set_file function v5

     Add the new vma_set_file() function to allow changing
     vma->vm_file with the necessary refcount dance.

It just looks like I missed the case in overlayfs while doing this.

Sorry for the noise.

Regards,
Christian.

>
> I suggest reverting this commit as a first step.
>
> Thanks,
> Miklos