Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1505243 > unrolled thread

Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up

Started byVivek Goyal <vgoyal@redhat.com>
First post2016-10-20 22:50 +0200
Last post2016-10-21 15:20 +0200
Articles 12 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Vivek Goyal <vgoyal@redhat.com> - 2016-10-20 22:50 +0200
    Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Vivek Goyal <vgoyal@redhat.com> - 2016-10-20 23:00 +0200
      Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Amir Goldstein <amir73il@gmail.com> - 2016-10-21 11:00 +0200
        Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Vivek Goyal <vgoyal@redhat.com> - 2016-10-21 22:20 +0200
          Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Amir Goldstein <amir73il@gmail.com> - 2016-10-22 09:30 +0200
            Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Amir Goldstein <amir73il@gmail.com> - 2016-10-22 17:40 +0200
              Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Miklos Szeredi <miklos@szeredi.hu> - 2016-10-24 10:20 +0200
      Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Miklos Szeredi <miklos@szeredi.hu> - 2016-10-21 11:20 +0200
        Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Vivek Goyal <vgoyal@redhat.com> - 2016-10-21 15:40 +0200
    Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Amir Goldstein <amir73il@gmail.com> - 2016-10-21 11:20 +0200
      Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Miklos Szeredi <miklos@szeredi.hu> - 2016-10-21 11:40 +0200
        Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up Amir Goldstein <amir73il@gmail.com> - 2016-10-21 15:20 +0200

#1505243 — Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up

FromVivek Goyal <vgoyal@redhat.com>
Date2016-10-20 22:50 +0200
SubjectRe: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up
Message-ID<susrL-45X-1@gated-at.bofh.it>
On Wed, Oct 12, 2016 at 03:33:26PM +0200, Miklos Szeredi wrote:
> This is a proof of concept patch to fix the following.
> 
> /ovl is in overlay mount and /ovl/foo exists on the lower layer only.
> 
>  rofd = open("/ovl/foo", O_RDONLY);
>  rwfd = open("/ovl/foo", O_WRONLY); /* this causes copy up */
>  write(rwfd, "bar", 3);
>  read(rofd, buf, 3); 
>  assert(memcmp(buf, "bar", 3) == 0);
> 
> Similar problem exists with an MAP_SHARED mmap created from rofd.
> 
> While this has only caused few problems (yum/dnf failure is the only one I know
> of) and easily worked around in userspace, many see it as a proof that overlayfs
> can never be a proper "POSIX" filesystem.
> 
> To quell those worries, here's a simple patch that should address the above.
> 
> The only VFS change is that f_op is initialized from f_path.dentry->d_inode
> instead of file_inode(filp) in open.  The effect of this is that overlayfs can
> intercept open and other file operations, while the file still effectively
> belongs to the underlying fs.
> 
> The patch does not give up on the nice properties of overlayfs, like sharing the
> page cache with the underlying files.  It does cause copy up in one case where
> previously there wasn't one and that's the O_RDONLY/MAP_SHARED case.  I haven't
> done much research into this, but running some tests in chroot didn't trigger
> this.
> 
> Comments, testing are welcome.

Hi Miklos, 

This looks like a very interesting idea. In fact once file has been copied
up and writen to, and if I do fstat(rofd), it shows the size of copied up
file but one can read the contents. So fixing that anomaly would be nice.

Hopefully O_RDONLY/MAP_SHARED is not a common case and we get away with
this forced copy up penalty.

[..]
> +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to)
> +{
> +	struct file *file = iocb->ki_filp;
> +	bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
> +	ssize_t ret = -EINVAL;
> +
> +	if (likely(!isupper)) {
> +		const struct file_operations *fop = ovl_real_fop(file);
> +
> +		if (likely(fop->read_iter))
> +			ret = fop->read_iter(iocb, to);
> +	} else {
> +		struct file *upperfile = filp_clone_open(file);
> +

IIUC, every read of lower file will call filp_clone_open(). Looking at the
code of filp_clone_open(), I am concerned about the overhead of this call.
Is it significant? Don't want to be paying too much of penalty for read
operation on lower files. That would be a common case for containers.

BTW, I did a quick testing. Using docker launched a fedora container and
called "dnf update" inside that. And later I noticed following on serial
console.

Thanks
Vivek

[  309.075885] ======================================================
[  309.076841] [ INFO: possible circular locking dependency detected ]
[  309.077818] 4.9.0-rc1+ #197 Not tainted
[  309.078411] -------------------------------------------------------
[  309.079377] dnf/2468 is trying to acquire lock:
[  309.080082]  ([  309.080324] &type->s_vfs_rename_key
#2[  309.080942] ){+.+.+.}
, at: [  309.081435] [<ffffffff8129f652>] lock_rename+0x32/0x100
[  309.082261] 
[  309.082261] but task is already holding lock:
[  309.083158]  ([  309.083399] &mm->mmap_sem
){++++++}[  309.083974] , at: 
[  309.084316] [<ffffffff8121df0c>] vm_mmap_pgoff+0x8c/0x100
[  309.085150] 
[  309.085150] which lock already depends on the new lock.
[  309.085150] 
[  309.086393] 
[  309.086393] the existing dependency chain (in reverse order) is:
[  309.088279] 
-> #3[  309.088612]  (
&mm->mmap_sem[  309.089091] ){++++++}
[  309.089470] :
[  309.089735]        [  309.090046] [<ffffffff81112786>] lock_acquire+0xf6/0x1f0
[  309.090884]        [  309.091197] [<ffffffff812316c0>] __might_fault+0x70/0xa0
[  309.092047]        [  309.092357] [<ffffffff812aa585>] filldir+0xb5/0x140
[  309.093128]        [  309.093434] [<ffffffff8132f235>] call_filldir+0x65/0x130
[  309.094273]        [  309.094590] [<ffffffff8132fc0f>] ext4_readdir+0x6cf/0x8a0
[  309.095425]        [  309.095742] [<ffffffff812aa24b>] iterate_dir+0x17b/0x1b0
[  309.096572]        [  309.096878] [<ffffffff812aa76c>] SyS_getdents+0x9c/0x130
[  309.097716]        [  309.098026] [<ffffffff818a4bc1>] entry_SYSCALL_64_fastpath+0x1f/0xc2
[  309.099005] 
-> #2[  309.099304]  (
&type->i_mutex_dir_key[  309.099888] #3
){++++++}[  309.100301] :
[  309.100576]        [  309.100881] [<ffffffff81112786>] lock_acquire+0xf6/0x1f0
[  309.101711]        [  309.102017] [<ffffffff818a1279>] down_write+0x49/0x80
[  309.102798]        [  309.103098] [<ffffffff8129fea5>] vfs_rmdir+0x55/0x140
[  309.103878]        [  309.104179] [<ffffffff812a5bdd>] do_rmdir+0x1bd/0x230
[  309.104958]        [  309.105256] [<ffffffff812a6a12>] SyS_unlinkat+0x22/0x30
[  309.106063]        [  309.106364] [<ffffffff818a4bc1>] entry_SYSCALL_64_fastpath+0x1f/0xc2
[  309.107345] 
-> #1[  309.107661]  (
&type->i_mutex_dir_key[  309.108256] #3
/1[  309.108597] ){+.+.+.}
[  309.108971] :
[  309.109225]        [  309.109532] [<ffffffff81112786>] lock_acquire+0xf6/0x1f0
[  309.110370]        [  309.110686] [<ffffffff8110c8df>] down_write_nested+0x4f/0x80
[  309.111606]        [  309.111916] [<ffffffff8129f701>] lock_rename+0xe1/0x100
[  309.112752]        [  309.113062] [<ffffffff812a7842>] SyS_renameat+0x212/0x3f0
[  309.113918]        [  309.114224] [<ffffffff818a4bc1>] entry_SYSCALL_64_fastpath+0x1f/0xc2
[  309.115218] 
-> #0[  309.115525]  (
&type->s_vfs_rename_key[  309.116138] #2
){+.+.+.}[  309.116564] :
[  309.116837]        [  309.117146] [<ffffffff811121a0>] __lock_acquire+0x1110/0x12a0
[  309.118047]        [  309.118354] [<ffffffff81112786>] lock_acquire+0xf6/0x1f0
[  309.119193]        [  309.119500] [<ffffffff818a0159>] mutex_lock_nested+0x79/0x3c0
[  309.120406]        [  309.120723] [<ffffffff8129f652>] lock_rename+0x32/0x100
[  309.121564]        [  309.121875] [<ffffffffa02d1617>] ovl_copy_up_one+0xf7/0x6a0 [overlay]
[  309.122866]        [  309.123170] [<ffffffffa02d1ccb>] ovl_copy_up+0x10b/0x13d [overlay]
[  309.124136]        [  309.124442] [<ffffffffa02cd7a5>] ovl_mmap+0x55/0x90 [overlay]
[  309.125348]        [  309.125677] [<ffffffff8123f3a4>] mmap_region+0x394/0x630
[  309.126510]        [  309.126829] [<ffffffff8123fa86>] do_mmap+0x446/0x530
[  309.127616]        [  309.127928] [<ffffffff8121df3d>] vm_mmap_pgoff+0xbd/0x100
[  309.128783]        [  309.129092] [<ffffffff8123d5c1>] SyS_mmap_pgoff+0x1c1/0x290
[  309.129973]        [  309.130284] [<ffffffff8103b71b>] SyS_mmap+0x1b/0x30
[  309.131058]        [  309.131368] [<ffffffff818a4bc1>] entry_SYSCALL_64_fastpath+0x1f/0xc2
[  309.132377] 
[  309.132377] other info that might help us debug this:
[  309.132377] 
[  309.133608] Chain exists of:
  [  309.134084] &type->s_vfs_rename_key
#2[  309.134694]  --> 
&type->i_mutex_dir_key[  309.135324] #3
 --> [  309.135705] &mm->mmap_sem
[  309.136131] 
[  309.136131] 
[  309.136599]  Possible unsafe locking scenario:
[  309.136599] 
[  309.137499]        CPU0                    CPU1
[  309.138202]        ----                    ----
[  309.138905]   lock([  309.139211] &mm->mmap_sem
[  309.139646] );
[  309.139914]                                lock([  309.140602] &type->i_mutex_dir_key
#3[  309.141190] );
[  309.141472]                                lock([  309.142175] &mm->mmap_sem
[  309.142610] );
[  309.142877]   lock([  309.143186] &type->s_vfs_rename_key
#2[  309.143796] );
[  309.144084] 
[  309.144084]  *** DEADLOCK ***
[  309.144084] 
[  309.144991] 1 lock held by dnf/2468:
[  309.145543]  #0: [  309.145827]  (
&mm->mmap_sem[  309.146299] ){++++++}
, at: [  309.146782] [<ffffffff8121df0c>] vm_mmap_pgoff+0x8c/0x100
[  309.147634] 
[  309.147634] stack backtrace:
[  309.148310] CPU: 4 PID: 2468 Comm: dnf Not tainted 4.9.0-rc1+ #197
[  309.149257] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.2-20150714_191134- 04/01/2014
[  309.150717]  ffffc900033cf900 ffffffff8144b253 ffffffff828b29f0 ffffffff828e8d50
[  309.151942]  ffffc900033cf940 ffffffff8110f83e 00000000f7ac0000 ffff8801f7ac0860
[  309.153660]  ffff8801f7ac0000 ffff8801f7ac0838 0000000000000001 0000000000000000
[  309.154877] Call Trace:
[  309.155266]  [<ffffffff8144b253>] dump_stack+0x86/0xc3
[  309.156062]  [<ffffffff8110f83e>] print_circular_bug+0x1be/0x210
[  309.156991]  [<ffffffff811121a0>] __lock_acquire+0x1110/0x12a0
[  309.157896]  [<ffffffff8112ee3d>] ? debug_lockdep_rcu_enabled+0x1d/0x20
[  309.158912]  [<ffffffff813c3ed4>] ? avc_has_perm+0x34/0x290
[  309.159768]  [<ffffffff81112786>] lock_acquire+0xf6/0x1f0
[  309.160597]  [<ffffffff8129f652>] ? lock_rename+0x32/0x100
[  309.161438]  [<ffffffff818a0159>] mutex_lock_nested+0x79/0x3c0
[  309.162351]  [<ffffffff8129f652>] ? lock_rename+0x32/0x100
[  309.163202]  [<ffffffff813c92fb>] ? selinux_inode_getattr+0x8b/0xb0
[  309.164162]  [<ffffffff8129f652>] lock_rename+0x32/0x100
[  309.164981]  [<ffffffffa02d1617>] ovl_copy_up_one+0xf7/0x6a0 [overlay]
[  309.165987]  [<ffffffff813c3fd3>] ? avc_has_perm+0x133/0x290
[  309.166864]  [<ffffffff813c3ed4>] ? avc_has_perm+0x34/0x290
[  309.167723]  [<ffffffff810e348a>] ? __might_sleep+0x4a/0x80
[  309.168580]  [<ffffffffa02d1ccb>] ovl_copy_up+0x10b/0x13d [overlay]
[  309.169539]  [<ffffffffa02cd7a5>] ovl_mmap+0x55/0x90 [overlay]
[  309.170436]  [<ffffffff8123f3a4>] mmap_region+0x394/0x630
[  309.171269]  [<ffffffff8123fa86>] do_mmap+0x446/0x530
[  309.172064]  [<ffffffff8121df3d>] vm_mmap_pgoff+0xbd/0x100
[  309.172908]  [<ffffffff8123d5c1>] SyS_mmap_pgoff+0x1c1/0x290
[  309.173777]  [<ffffffff8103b71b>] SyS_mmap+0x1b/0x30
[  309.174539]  [<ffffffff818a4bc1>] entry_SYSCALL_64_fastpath+0x1f/0xc2

[toc] | [next] | [standalone]

#1505250

FromVivek Goyal <vgoyal@redhat.com>
Date2016-10-20 23:00 +0200
Message-ID<susBx-49j-7@gated-at.bofh.it>
In reply to#1505243
On Thu, Oct 20, 2016 at 04:46:30PM -0400, Vivek Goyal wrote:

[..]
> > +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > +{
> > +	struct file *file = iocb->ki_filp;
> > +	bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
> > +	ssize_t ret = -EINVAL;
> > +
> > +	if (likely(!isupper)) {
> > +		const struct file_operations *fop = ovl_real_fop(file);
> > +
> > +		if (likely(fop->read_iter))
> > +			ret = fop->read_iter(iocb, to);
> > +	} else {
> > +		struct file *upperfile = filp_clone_open(file);
> > +
> 
> IIUC, every read of lower file will call filp_clone_open(). Looking at the
> code of filp_clone_open(), I am concerned about the overhead of this call.
> Is it significant? Don't want to be paying too much of penalty for read
> operation on lower files. That would be a common case for containers.
> 

Looks like I read the code in reverse. So if I open a file read-only,
and if it has not been copied up, I will simply call read_iter() on
lower filesystem. But if file has been copied up, then I will call
filp_clone_open() and pay the cost. And this will continue till this
file is closed by caller. 

When file is opened again, by that time it is upper file and we will
install real fop in file (instead of overlay fop).

Vivek

[toc] | [prev] | [next] | [standalone]

#1505584

FromAmir Goldstein <amir73il@gmail.com>
Date2016-10-21 11:00 +0200
Message-ID<suDQd-3aT-15@gated-at.bofh.it>
In reply to#1505250
On Thu, Oct 20, 2016 at 11:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Oct 20, 2016 at 04:46:30PM -0400, Vivek Goyal wrote:
>
> [..]
>> > +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to)
>> > +{
>> > +   struct file *file = iocb->ki_filp;
>> > +   bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
>> > +   ssize_t ret = -EINVAL;
>> > +
>> > +   if (likely(!isupper)) {
>> > +           const struct file_operations *fop = ovl_real_fop(file);
>> > +
>> > +           if (likely(fop->read_iter))
>> > +                   ret = fop->read_iter(iocb, to);
>> > +   } else {
>> > +           struct file *upperfile = filp_clone_open(file);
>> > +
>>
>> IIUC, every read of lower file will call filp_clone_open(). Looking at the
>> code of filp_clone_open(), I am concerned about the overhead of this call.
>> Is it significant? Don't want to be paying too much of penalty for read
>> operation on lower files. That would be a common case for containers.
>>
>
> Looks like I read the code in reverse. So if I open a file read-only,
> and if it has not been copied up, I will simply call read_iter() on
> lower filesystem. But if file has been copied up, then I will call
> filp_clone_open() and pay the cost. And this will continue till this
> file is closed by caller.
>

I wonder if that cost could be reduced by calling replace_fd() or
some variant of it to install the cloned file onto the rofd after the
first access??

> When file is opened again, by that time it is upper file and we will
> install real fop in file (instead of overlay fop).
>
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1506264

FromVivek Goyal <vgoyal@redhat.com>
Date2016-10-21 22:20 +0200
Message-ID<suOsh-26F-5@gated-at.bofh.it>
In reply to#1505584
On Fri, Oct 21, 2016 at 11:53:41AM +0300, Amir Goldstein wrote:
> On Thu, Oct 20, 2016 at 11:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Oct 20, 2016 at 04:46:30PM -0400, Vivek Goyal wrote:
> >
> > [..]
> >> > +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to)
> >> > +{
> >> > +   struct file *file = iocb->ki_filp;
> >> > +   bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
> >> > +   ssize_t ret = -EINVAL;
> >> > +
> >> > +   if (likely(!isupper)) {
> >> > +           const struct file_operations *fop = ovl_real_fop(file);
> >> > +
> >> > +           if (likely(fop->read_iter))
> >> > +                   ret = fop->read_iter(iocb, to);
> >> > +   } else {
> >> > +           struct file *upperfile = filp_clone_open(file);
> >> > +
> >>
> >> IIUC, every read of lower file will call filp_clone_open(). Looking at the
> >> code of filp_clone_open(), I am concerned about the overhead of this call.
> >> Is it significant? Don't want to be paying too much of penalty for read
> >> operation on lower files. That would be a common case for containers.
> >>
> >
> > Looks like I read the code in reverse. So if I open a file read-only,
> > and if it has not been copied up, I will simply call read_iter() on
> > lower filesystem. But if file has been copied up, then I will call
> > filp_clone_open() and pay the cost. And this will continue till this
> > file is closed by caller.
> >
> 
> I wonder if that cost could be reduced by calling replace_fd() or
> some variant of it to install the cloned file onto the rofd after the
> first access??

Hmm.., Interesting. Will something like following work? This applies on
top of Miklos's patch. It seems to work for me. It might be completely
broken/racy though. Somebody who understands this code well, will have
to have a look.

---
 fs/file.c            |   41 +++++++++++++++++++++++++++++++++++++++++
 fs/overlayfs/inode.c |    1 +
 2 files changed, 42 insertions(+)

Index: rhvgoyal-linux/fs/overlayfs/inode.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/inode.c	2016-10-21 15:43:05.391488406 -0400
+++ rhvgoyal-linux/fs/overlayfs/inode.c	2016-10-21 16:07:57.409420795 -0400
@@ -416,6 +416,7 @@ static ssize_t ovl_read_iter(struct kioc
 		if (IS_ERR(upperfile)) {
 			ret = PTR_ERR(upperfile);
 		} else {
+			replace_file(file, upperfile);
 			ret = vfs_iter_read(upperfile, to, &iocb->ki_pos);
 			fput(upperfile);
 		}
Index: rhvgoyal-linux/fs/file.c
===================================================================
--- rhvgoyal-linux.orig/fs/file.c	2016-10-21 15:43:05.391488406 -0400
+++ rhvgoyal-linux/fs/file.c	2016-10-21 16:08:18.168420795 -0400
@@ -864,6 +864,47 @@ Ebusy:
 	return -EBUSY;
 }
 
+
+int replace_file(struct file *old_file, struct file *new_file)
+{
+#define MAX_TO_FREE	8
+	int n, idx = 0;
+	struct files_struct *files = current->files;
+	struct fdtable *fdt;
+	struct file *to_free[MAX_TO_FREE];
+	bool retry = false;
+
+try_again:
+	spin_lock(&files->file_lock);
+	for (n = 0, fdt = files_fdtable(files); n < fdt->max_fds; n++) {
+                struct file *file;
+                file = rcu_dereference_check_fdtable(files, fdt->fd[n]);
+                if (!file)
+                        continue;
+		if (file == old_file) {
+			get_file(new_file);
+			rcu_assign_pointer(fdt->fd[n], new_file);
+			to_free[idx++] = file;
+			if (idx >= MAX_TO_FREE) {
+				retry = true;
+				break;
+			}
+		}
+        }
+	spin_unlock(&files->file_lock);
+	while (idx) {
+		filp_close(to_free[--idx], files);
+	}
+
+	if (retry) {
+		retry = false;
+		idx = 0;
+		goto try_again;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(replace_file);
+
 int replace_fd(unsigned fd, struct file *file, unsigned flags)
 {
 	int err;

[toc] | [prev] | [next] | [standalone]

#1506393

FromAmir Goldstein <amir73il@gmail.com>
Date2016-10-22 09:30 +0200
Message-ID<suYUG-lu-11@gated-at.bofh.it>
In reply to#1506264
On Fri, Oct 21, 2016 at 11:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Oct 21, 2016 at 11:53:41AM +0300, Amir Goldstein wrote:
>> On Thu, Oct 20, 2016 at 11:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Thu, Oct 20, 2016 at 04:46:30PM -0400, Vivek Goyal wrote:
>> >
>> > [..]
>> >> > +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to)
>> >> > +{
>> >> > +   struct file *file = iocb->ki_filp;
>> >> > +   bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
>> >> > +   ssize_t ret = -EINVAL;
>> >> > +
>> >> > +   if (likely(!isupper)) {
>> >> > +           const struct file_operations *fop = ovl_real_fop(file);
>> >> > +
>> >> > +           if (likely(fop->read_iter))
>> >> > +                   ret = fop->read_iter(iocb, to);
>> >> > +   } else {
>> >> > +           struct file *upperfile = filp_clone_open(file);
>> >> > +
>> >>
>> >> IIUC, every read of lower file will call filp_clone_open(). Looking at the
>> >> code of filp_clone_open(), I am concerned about the overhead of this call.
>> >> Is it significant? Don't want to be paying too much of penalty for read
>> >> operation on lower files. That would be a common case for containers.
>> >>
>> >
>> > Looks like I read the code in reverse. So if I open a file read-only,
>> > and if it has not been copied up, I will simply call read_iter() on
>> > lower filesystem. But if file has been copied up, then I will call
>> > filp_clone_open() and pay the cost. And this will continue till this
>> > file is closed by caller.
>> >
>>
>> I wonder if that cost could be reduced by calling replace_fd() or
>> some variant of it to install the cloned file onto the rofd after the
>> first access??
>
> Hmm.., Interesting. Will something like following work? This applies on
> top of Miklos's patch. It seems to work for me. It might be completely
> broken/racy though. Somebody who understands this code well, will have
> to have a look.
>

The idea sounded scary already when I suggested it :)
See below what I think is scary about this implementation...

Thanks for following through.


> ---
>  fs/file.c            |   41 +++++++++++++++++++++++++++++++++++++++++
>  fs/overlayfs/inode.c |    1 +
>  2 files changed, 42 insertions(+)
>
> Index: rhvgoyal-linux/fs/overlayfs/inode.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/overlayfs/inode.c    2016-10-21 15:43:05.391488406 -0400
> +++ rhvgoyal-linux/fs/overlayfs/inode.c 2016-10-21 16:07:57.409420795 -0400
> @@ -416,6 +416,7 @@ static ssize_t ovl_read_iter(struct kioc
>                 if (IS_ERR(upperfile)) {
>                         ret = PTR_ERR(upperfile);
>                 } else {
> +                       replace_file(file, upperfile);

When fdtable is not shared (single threaded process), after this call
I think that file pointer
may be free (?), because file is not reference counted.
Although I did not see any code in VFS callers trying to dereference
the file pointer after
calling read_iter(), this seems like a dangerous practice, so will
need to a way to fix that.

>                         ret = vfs_iter_read(upperfile, to, &iocb->ki_pos);
>                         fput(upperfile);
>                 }
> Index: rhvgoyal-linux/fs/file.c
> ===================================================================
> --- rhvgoyal-linux.orig/fs/file.c       2016-10-21 15:43:05.391488406 -0400
> +++ rhvgoyal-linux/fs/file.c    2016-10-21 16:08:18.168420795 -0400
> @@ -864,6 +864,47 @@ Ebusy:
>         return -EBUSY;
>  }
>
> +
> +int replace_file(struct file *old_file, struct file *new_file)
> +{
> +#define MAX_TO_FREE    8
> +       int n, idx = 0;
> +       struct files_struct *files = current->files;
> +       struct fdtable *fdt;
> +       struct file *to_free[MAX_TO_FREE];
> +       bool retry = false;
> +
> +try_again:
> +       spin_lock(&files->file_lock);
> +       for (n = 0, fdt = files_fdtable(files); n < fdt->max_fds; n++) {
> +                struct file *file;
> +                file = rcu_dereference_check_fdtable(files, fdt->fd[n]);
> +                if (!file)
> +                        continue;
> +               if (file == old_file) {
> +                       get_file(new_file);
> +                       rcu_assign_pointer(fdt->fd[n], new_file);
> +                       to_free[idx++] = file;
> +                       if (idx >= MAX_TO_FREE) {
> +                               retry = true;
> +                               break;
> +                       }
> +               }
> +        }
> +       spin_unlock(&files->file_lock);
> +       while (idx) {
> +               filp_close(to_free[--idx], files);
> +       }
> +
> +       if (retry) {
> +               retry = false;
> +               idx = 0;
> +               goto try_again;
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL(replace_file);
> +
>  int replace_fd(unsigned fd, struct file *file, unsigned flags)
>  {
>         int err;

[toc] | [prev] | [next] | [standalone]

#1506558

FromAmir Goldstein <amir73il@gmail.com>
Date2016-10-22 17:40 +0200
Message-ID<sv6yR-5ej-7@gated-at.bofh.it>
In reply to#1506393
On Sat, Oct 22, 2016 at 10:24 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, Oct 21, 2016 at 11:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> On Fri, Oct 21, 2016 at 11:53:41AM +0300, Amir Goldstein wrote:
>>> On Thu, Oct 20, 2016 at 11:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>> > On Thu, Oct 20, 2016 at 04:46:30PM -0400, Vivek Goyal wrote:
>>> >
>>> > [..]
>>> >> > +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to)
>>> >> > +{
>>> >> > +   struct file *file = iocb->ki_filp;
>>> >> > +   bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
>>> >> > +   ssize_t ret = -EINVAL;
>>> >> > +
>>> >> > +   if (likely(!isupper)) {
>>> >> > +           const struct file_operations *fop = ovl_real_fop(file);
>>> >> > +
>>> >> > +           if (likely(fop->read_iter))
>>> >> > +                   ret = fop->read_iter(iocb, to);
>>> >> > +   } else {
>>> >> > +           struct file *upperfile = filp_clone_open(file);
>>> >> > +
>>> >>
>>> >> IIUC, every read of lower file will call filp_clone_open(). Looking at the
>>> >> code of filp_clone_open(), I am concerned about the overhead of this call.
>>> >> Is it significant? Don't want to be paying too much of penalty for read
>>> >> operation on lower files. That would be a common case for containers.
>>> >>
>>> >
>>> > Looks like I read the code in reverse. So if I open a file read-only,
>>> > and if it has not been copied up, I will simply call read_iter() on
>>> > lower filesystem. But if file has been copied up, then I will call
>>> > filp_clone_open() and pay the cost. And this will continue till this
>>> > file is closed by caller.
>>> >
>>>
>>> I wonder if that cost could be reduced by calling replace_fd() or
>>> some variant of it to install the cloned file onto the rofd after the
>>> first access??
>>
>> Hmm.., Interesting. Will something like following work? This applies on
>> top of Miklos's patch. It seems to work for me. It might be completely
>> broken/racy though. Somebody who understands this code well, will have
>> to have a look.
>>
>
> The idea sounded scary already when I suggested it :)
> See below what I think is scary about this implementation...
>
> Thanks for following through.
>
>
>> ---
>>  fs/file.c            |   41 +++++++++++++++++++++++++++++++++++++++++
>>  fs/overlayfs/inode.c |    1 +
>>  2 files changed, 42 insertions(+)
>>
>> Index: rhvgoyal-linux/fs/overlayfs/inode.c
>> ===================================================================
>> --- rhvgoyal-linux.orig/fs/overlayfs/inode.c    2016-10-21 15:43:05.391488406 -0400
>> +++ rhvgoyal-linux/fs/overlayfs/inode.c 2016-10-21 16:07:57.409420795 -0400
>> @@ -416,6 +416,7 @@ static ssize_t ovl_read_iter(struct kioc
>>                 if (IS_ERR(upperfile)) {
>>                         ret = PTR_ERR(upperfile);
>>                 } else {
>> +                       replace_file(file, upperfile);
>
> When fdtable is not shared (single threaded process), after this call
> I think that file pointer
> may be free (?), because file is not reference counted.
> Although I did not see any code in VFS callers trying to dereference
> the file pointer after
> calling read_iter(), this seems like a dangerous practice, so will
> need to a way to fix that.
>

My bad. file pointer is freed in work_task_run(), so replace_file()
should be just as safe as replace_fd() and do_dup2().

[toc] | [prev] | [next] | [standalone]

#1506987

FromMiklos Szeredi <miklos@szeredi.hu>
Date2016-10-24 10:20 +0200
Message-ID<svIEa-50a-29@gated-at.bofh.it>
In reply to#1506558
On Sat, Oct 22, 2016 at 5:39 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Sat, Oct 22, 2016 at 10:24 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, Oct 21, 2016 at 11:13 PM, Vivek Goyal <vgoyal@redhat.com> wrote:

>>> ---
>>>  fs/file.c            |   41 +++++++++++++++++++++++++++++++++++++++++
>>>  fs/overlayfs/inode.c |    1 +
>>>  2 files changed, 42 insertions(+)
>>>
>>> Index: rhvgoyal-linux/fs/overlayfs/inode.c
>>> ===================================================================
>>> --- rhvgoyal-linux.orig/fs/overlayfs/inode.c    2016-10-21 15:43:05.391488406 -0400
>>> +++ rhvgoyal-linux/fs/overlayfs/inode.c 2016-10-21 16:07:57.409420795 -0400
>>> @@ -416,6 +416,7 @@ static ssize_t ovl_read_iter(struct kioc
>>>                 if (IS_ERR(upperfile)) {
>>>                         ret = PTR_ERR(upperfile);
>>>                 } else {
>>> +                       replace_file(file, upperfile);

I think it's a cool idea.  But I'm not even going to look at the
implementation for now, because it's such a rare corner case, that
trying to optimize it should really be the last thing we do after
everything else is working fine (and only if it actually turns out to
be a thing that somebody actually cares about).

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1505620

FromMiklos Szeredi <miklos@szeredi.hu>
Date2016-10-21 11:20 +0200
Message-ID<suE9B-3xe-45@gated-at.bofh.it>
In reply to#1505250
On Thu, Oct 20, 2016 at 04:54:08PM -0400, Vivek Goyal wrote:
> On Thu, Oct 20, 2016 at 04:46:30PM -0400, Vivek Goyal wrote:
> 
> [..]
> > > +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > > +{
> > > +	struct file *file = iocb->ki_filp;
> > > +	bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
> > > +	ssize_t ret = -EINVAL;
> > > +
> > > +	if (likely(!isupper)) {
> > > +		const struct file_operations *fop = ovl_real_fop(file);
> > > +
> > > +		if (likely(fop->read_iter))
> > > +			ret = fop->read_iter(iocb, to);
> > > +	} else {
> > > +		struct file *upperfile = filp_clone_open(file);
> > > +
> > 
> > IIUC, every read of lower file will call filp_clone_open(). Looking at the
> > code of filp_clone_open(), I am concerned about the overhead of this call.
> > Is it significant? Don't want to be paying too much of penalty for read
> > operation on lower files. That would be a common case for containers.
> > 
> 
> Looks like I read the code in reverse. So if I open a file read-only,
> and if it has not been copied up, I will simply call read_iter() on
> lower filesystem. But if file has been copied up, then I will call
> filp_clone_open() and pay the cost. And this will continue till this
> file is closed by caller. 
> 
> When file is opened again, by that time it is upper file and we will
> install real fop in file (instead of overlay fop).

Right.

The lockdep issue seems to be real, we can't take i_mutex and s_vfs_rename_mutex
while mmap_sem is locked.  Fortunately copy up doesn't need mmap_sem, so we can
do it while unlocked and retry the mmap.

Here's an incremental workaround patch.

I don't like adding such workarounds to the VFS/MM but they are really cheap for
the non-overlay case and there doesn't appear to be an alternative in this case.

Thanks,
Miklos

---
 fs/overlayfs/inode.c |   19 +++++--------------
 mm/util.c            |   22 ++++++++++++++++++++++
 2 files changed, 27 insertions(+), 14 deletions(-)

--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -419,21 +419,12 @@ static int ovl_mmap(struct file *file, s
 	bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
 	int err;
 
-	/*
-	 * Treat MAP_SHARED as hint about future writes to the file (through
-	 * another file descriptor).  Caller might not have had such an intent,
-	 * but we hope MAP_PRIVATE will be used in most such cases.
-	 *
-	 * If we don't copy up now and the file is modified, it becomes really
-	 * difficult to change the mapping to match that of the file's content
-	 * later.
-	 */
 	if (unlikely(isupper || vma->vm_flags & VM_MAYSHARE)) {
-		if (!isupper) {
-			err = ovl_copy_up(file->f_path.dentry);
-			if (err)
-				goto out;
-		}
+		/*
+		 * File should have been copied up by now. See vm_mmap_pgoff().
+		 */
+		if (WARN_ON(!isupper))
+			return -EIO;
 
 		file = filp_clone_open(file);
 		err = PTR_ERR(file);
--- a/mm/util.c
+++ b/mm/util.c
@@ -297,6 +297,28 @@ unsigned long vm_mmap_pgoff(struct file
 
 	ret = security_mmap_file(file, prot, flag);
 	if (!ret) {
+		/*
+		 * Special treatment for overlayfs:
+		 *
+		 * Take MAP_SHARED/PROT_READ as hint about future writes to the
+		 * file (through another file descriptor).  Caller might not
+		 * have had such an intent, but we hope MAP_PRIVATE will be used
+		 * in most such cases.
+		 *
+		 * If we don't copy up now and the file is modified, it becomes
+		 * really difficult to change the mapping to match that of the
+		 * file's content later.
+		 *
+		 * Copy up needs to be done without mmap_sem since it takes vfs
+		 * locks which would potentially deadlock under mmap_sem.
+		 */
+		if ((flag & MAP_SHARED) && !(prot & PROT_WRITE)) {
+			void *p = d_real(file->f_path.dentry, NULL, O_WRONLY);
+
+			if (IS_ERR(p))
+				return PTR_ERR(p);
+		}
+
 		if (down_write_killable(&mm->mmap_sem))
 			return -EINTR;
 		ret = do_mmap_pgoff(file, addr, len, prot, flag, pgoff,

[toc] | [prev] | [next] | [standalone]

#1505960

FromVivek Goyal <vgoyal@redhat.com>
Date2016-10-21 15:40 +0200
Message-ID<suIdc-67f-39@gated-at.bofh.it>
In reply to#1505620
On Fri, Oct 21, 2016 at 11:12:11AM +0200, Miklos Szeredi wrote:
> On Thu, Oct 20, 2016 at 04:54:08PM -0400, Vivek Goyal wrote:
> > On Thu, Oct 20, 2016 at 04:46:30PM -0400, Vivek Goyal wrote:
> > 
> > [..]
> > > > +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to)
> > > > +{
> > > > +	struct file *file = iocb->ki_filp;
> > > > +	bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
> > > > +	ssize_t ret = -EINVAL;
> > > > +
> > > > +	if (likely(!isupper)) {
> > > > +		const struct file_operations *fop = ovl_real_fop(file);
> > > > +
> > > > +		if (likely(fop->read_iter))
> > > > +			ret = fop->read_iter(iocb, to);
> > > > +	} else {
> > > > +		struct file *upperfile = filp_clone_open(file);
> > > > +
> > > 
> > > IIUC, every read of lower file will call filp_clone_open(). Looking at the
> > > code of filp_clone_open(), I am concerned about the overhead of this call.
> > > Is it significant? Don't want to be paying too much of penalty for read
> > > operation on lower files. That would be a common case for containers.
> > > 
> > 
> > Looks like I read the code in reverse. So if I open a file read-only,
> > and if it has not been copied up, I will simply call read_iter() on
> > lower filesystem. But if file has been copied up, then I will call
> > filp_clone_open() and pay the cost. And this will continue till this
> > file is closed by caller. 
> > 
> > When file is opened again, by that time it is upper file and we will
> > install real fop in file (instead of overlay fop).
> 
> Right.
> 
> The lockdep issue seems to be real, we can't take i_mutex and s_vfs_rename_mutex
> while mmap_sem is locked.  Fortunately copy up doesn't need mmap_sem, so we can
> do it while unlocked and retry the mmap.
> 
> Here's an incremental workaround patch.
> 
> I don't like adding such workarounds to the VFS/MM but they are really cheap for
> the non-overlay case and there doesn't appear to be an alternative in this case.

This incremental patch does fix the locking warning issue I was seeing.

Vivek

[toc] | [prev] | [next] | [standalone]

#1505627

FromAmir Goldstein <amir73il@gmail.com>
Date2016-10-21 11:20 +0200
Message-ID<suE9B-3xe-61@gated-at.bofh.it>
In reply to#1505243
On Thu, Oct 20, 2016 at 11:46 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Oct 12, 2016 at 03:33:26PM +0200, Miklos Szeredi wrote:
>> This is a proof of concept patch to fix the following.
>>
>> /ovl is in overlay mount and /ovl/foo exists on the lower layer only.
>>
>>  rofd = open("/ovl/foo", O_RDONLY);
>>  rwfd = open("/ovl/foo", O_WRONLY); /* this causes copy up */
>>  write(rwfd, "bar", 3);
>>  read(rofd, buf, 3);
>>  assert(memcmp(buf, "bar", 3) == 0);
>>
>> Similar problem exists with an MAP_SHARED mmap created from rofd.
>>
>> While this has only caused few problems (yum/dnf failure is the only one I know
>> of) and easily worked around in userspace, many see it as a proof that overlayfs
>> can never be a proper "POSIX" filesystem.
>>
>> To quell those worries, here's a simple patch that should address the above.
>>
>> The only VFS change is that f_op is initialized from f_path.dentry->d_inode
>> instead of file_inode(filp) in open.  The effect of this is that overlayfs can
>> intercept open and other file operations, while the file still effectively
>> belongs to the underlying fs.
>>
>> The patch does not give up on the nice properties of overlayfs, like sharing the
>> page cache with the underlying files.  It does cause copy up in one case where
>> previously there wasn't one and that's the O_RDONLY/MAP_SHARED case.  I haven't
>> done much research into this, but running some tests in chroot didn't trigger
>> this.
>>
>> Comments, testing are welcome.
>
> Hi Miklos,
>
> This looks like a very interesting idea. In fact once file has been copied
> up and writen to, and if I do fstat(rofd), it shows the size of copied up
> file but one can read the contents. So fixing that anomaly would be nice.
>

I think it would be a good idea in general to stabilize the overlay ino/dev
throughout copy-up, same as Miklos suggested to do for directories, to
all files:
pure upper uses upper ino + overlayfs dev
non-pure upper uses lower ino + overlayfs dev

[toc] | [prev] | [next] | [standalone]

#1505714

FromMiklos Szeredi <miklos@szeredi.hu>
Date2016-10-21 11:40 +0200
Message-ID<suEsX-3Ew-55@gated-at.bofh.it>
In reply to#1505627
On Fri, Oct 21, 2016 at 11:13 AM, Amir Goldstein <amir73il@gmail.com> wrote:

> I think it would be a good idea in general to stabilize the overlay ino/dev
> throughout copy-up, same as Miklos suggested to do for directories, to
> all files:
> pure upper uses upper ino + overlayfs dev
> non-pure upper uses lower ino + overlayfs dev

Making st_ino, st_dev and d_ino behave consistently would be the next big step.

The above scheme only works if lower and upper are on the same
filesystem.  Otherwise there can be collisions between the lower and
upper inode numbers.  Perhaps you meant:

- pure upper uses upper ino + upper dev
- non-pure upper uses lower ino + overlayfs dev

It works for the single lower layer case, but again breaks if there
are multiple lower layers.   And d_ino in a merged directory could
still get us into trouble.  And find -xdev would not do what you'd
expect with a "normal" filesystem.

So there doesn't appear to be any easy solutions to this...

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1505914

FromAmir Goldstein <amir73il@gmail.com>
Date2016-10-21 15:20 +0200
Message-ID<suHTP-60l-7@gated-at.bofh.it>
In reply to#1505714
On Fri, Oct 21, 2016 at 12:30 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Oct 21, 2016 at 11:13 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>
>> I think it would be a good idea in general to stabilize the overlay ino/dev
>> throughout copy-up, same as Miklos suggested to do for directories, to
>> all files:
>> pure upper uses upper ino + overlayfs dev
>> non-pure upper uses lower ino + overlayfs dev
>
> Making st_ino, st_dev and d_ino behave consistently would be the next big step.
>
> The above scheme only works if lower and upper are on the same
> filesystem.  Otherwise there can be collisions between the lower and
> upper inode numbers.  Perhaps you meant:
>
> - pure upper uses upper ino + upper dev
> - non-pure upper uses lower ino + overlayfs dev
>
> It works for the single lower layer case, but again breaks if there
> are multiple lower layers.   And d_ino in a merged directory could
> still get us into trouble.  And find -xdev would not do what you'd
> expect with a "normal" filesystem.
>
> So there doesn't appear to be any easy solutions to this...
>

Not for the general case there isn't, but I was actually thinking of
the docker case
and there is a lot that can be done for the use case of lower and upper on the
same fs to make overlayfs more compliant.
Since it's quite a common use case, perhaps its worth the special treatment.

Amir.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web