* [PATCH] ovl: restore vma->vm_file to old file
@ 2021-04-20 2:07 Chengguang Xu
2021-04-21 9:47 ` Miklos Szeredi
0 siblings, 1 reply; 6+ messages in thread
From: Chengguang Xu @ 2021-04-20 2:07 UTC (permalink / raw)
To: miklos; +Cc: linux-unionfs, Chengguang Xu
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;
} else {
/* Drop reference count from previous vm_file value */
fput(file);
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ovl: restore vma->vm_file to old file
2021-04-20 2:07 [PATCH] ovl: restore vma->vm_file to old file Chengguang Xu
@ 2021-04-21 9:47 ` Miklos Szeredi
2021-04-21 11:03 ` Christian König
0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2021-04-21 9:47 UTC (permalink / raw)
To: Chengguang Xu, Christian König, Jason Gunthorpe,
Andrew Morton, Al Viro, Linus Torvalds
Cc: overlayfs
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"?
Changing refcounting rules in core kernel is no easy matter, a full
audit of ->mmap instances (>200) should have been done beforehand.
I suggest reverting this commit as a first step.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ovl: restore vma->vm_file to old file
2021-04-21 9:47 ` Miklos Szeredi
@ 2021-04-21 11:03 ` Christian König
2021-04-21 11:14 ` Miklos Szeredi
0 siblings, 1 reply; 6+ messages in thread
From: Christian König @ 2021-04-21 11:03 UTC (permalink / raw)
To: Miklos Szeredi, Chengguang Xu, Jason Gunthorpe, Andrew Morton,
Al Viro, Linus Torvalds
Cc: overlayfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ovl: restore vma->vm_file to old file
2021-04-21 11:03 ` Christian König
@ 2021-04-21 11:14 ` Miklos Szeredi
2021-04-21 11:25 ` Christian König
0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2021-04-21 11:14 UTC (permalink / raw)
To: Christian König
Cc: Chengguang Xu, Jason Gunthorpe, Andrew Morton, Al Viro,
Linus Torvalds, overlayfs
On Wed, Apr 21, 2021 at 1:03 PM Christian König
<christian.koenig@amd.com> wrote:
>
> 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?
In the error case overlayfs would put the reference on realfile (which
is vma->vm_file at that point) and mmap_region() would put the
reference to the original file (which was vma->vm_file before being
overridden).
After your commit mmap_region() puts the ref on the override vm_file,
but not on the original file.
>
> > 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.
Yes. And apparently a number of other cases where vm_file is assigned...
Thanks,
Miklos
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ovl: restore vma->vm_file to old file
2021-04-21 11:14 ` Miklos Szeredi
@ 2021-04-21 11:25 ` Christian König
2021-04-21 12:15 ` Miklos Szeredi
0 siblings, 1 reply; 6+ messages in thread
From: Christian König @ 2021-04-21 11:25 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Chengguang Xu, Jason Gunthorpe, Andrew Morton, Al Viro,
Linus Torvalds, overlayfs
Am 21.04.21 um 13:14 schrieb Miklos Szeredi:
> On Wed, Apr 21, 2021 at 1:03 PM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 21.04.21 um 11:47 schrieb Miklos Szeredi:
>> [SNIP]
>> Can you give wider context? In other words why did the patch broke the
>> reference counting in overlayfs?
> In the error case overlayfs would put the reference on realfile (which
> is vma->vm_file at that point) and mmap_region() would put the
> reference to the original file (which was vma->vm_file before being
> overridden).
>
> After your commit mmap_region() puts the ref on the override vm_file,
> but not on the original file.
Ah, of course. Double checking the mmap callback implementation of
overlayfs that is rather obvious.
>>> 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.
> Yes. And apparently a number of other cases where vm_file is assigned...
Yeah, I wasn't aware that filesystems do that as well and only
concentrated on the drivers.
Just did a "grep -R 'vm_file[[:space:]]*=' on the full kernel source and
it only showed one more case in fs/coda/file.c.
Do you see any other occurrences I potentially missed?
Otherwise I would say I provide patches for those two cases in a minute.
Thanks,
Christian.
>
> Thanks,
> Miklos
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ovl: restore vma->vm_file to old file
2021-04-21 11:25 ` Christian König
@ 2021-04-21 12:15 ` Miklos Szeredi
0 siblings, 0 replies; 6+ messages in thread
From: Miklos Szeredi @ 2021-04-21 12:15 UTC (permalink / raw)
To: Christian König
Cc: Chengguang Xu, Jason Gunthorpe, Andrew Morton, Al Viro,
Linus Torvalds, overlayfs
On Wed, Apr 21, 2021 at 1:25 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 21.04.21 um 13:14 schrieb Miklos Szeredi:
> > On Wed, Apr 21, 2021 at 1:03 PM Christian König
> > <christian.koenig@amd.com> wrote:
> >> Am 21.04.21 um 11:47 schrieb Miklos Szeredi:
> >> [SNIP]
> >> Can you give wider context? In other words why did the patch broke the
> >> reference counting in overlayfs?
> > In the error case overlayfs would put the reference on realfile (which
> > is vma->vm_file at that point) and mmap_region() would put the
> > reference to the original file (which was vma->vm_file before being
> > overridden).
> >
> > After your commit mmap_region() puts the ref on the override vm_file,
> > but not on the original file.
>
> Ah, of course. Double checking the mmap callback implementation of
> overlayfs that is rather obvious.
>
> >>> 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.
> > Yes. And apparently a number of other cases where vm_file is assigned...
>
> Yeah, I wasn't aware that filesystems do that as well and only
> concentrated on the drivers.
>
> Just did a "grep -R 'vm_file[[:space:]]*=' on the full kernel source and
> it only showed one more case in fs/coda/file.c.
>
> Do you see any other occurrences I potentially missed?
No, the others seem to be okay.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-21 12:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-20 2:07 [PATCH] ovl: restore vma->vm_file to old file Chengguang Xu
2021-04-21 9:47 ` Miklos Szeredi
2021-04-21 11:03 ` Christian König
2021-04-21 11:14 ` Miklos Szeredi
2021-04-21 11:25 ` Christian König
2021-04-21 12:15 ` Miklos Szeredi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).