($INBOX_DIR/description missing)
 help / color / Atom feed
* [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	[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, back to index

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

($INBOX_DIR/description missing)

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-unionfs/0 linux-unionfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-unionfs linux-unionfs/ https://lore.kernel.org/linux-unionfs \
		linux-unionfs@vger.kernel.org
	public-inbox-index linux-unionfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-unionfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git