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

Groups > linux.kernel > #1583574 > unrolled thread

[PATCH 0/9] overlay: fix inconsistency of ro file after copy-up

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/9] overlay: fix inconsistency of ro file after copy-up Miklos Szeredi <mszeredi@redhat.com> - 2017-02-17 17:20 +0100
    [PATCH 9/9] vfs: intercept fsync on overlay files Miklos Szeredi <mszeredi@redhat.com> - 2017-02-17 17:20 +0100
    [PATCH 8/9] vfs: use helper for calling f_op->fsync() Miklos Szeredi <mszeredi@redhat.com> - 2017-02-17 17:20 +0100
    [PATCH 4/9] vfs: intercept reads to overlay files Miklos Szeredi <mszeredi@redhat.com> - 2017-02-17 17:20 +0100
      Re: [PATCH 4/9] vfs: intercept reads to overlay files Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-19 10:10 +0100
        Re: [PATCH 4/9] vfs: intercept reads to overlay files Miklos Szeredi <mszeredi@redhat.com> - 2017-02-19 10:30 +0100
      Re: [PATCH 4/9] vfs: intercept reads to overlay files "zhangyi (F)" <yi.zhang@huawei.com> - 2017-02-20 09:10 +0100
        Re: [PATCH 4/9] vfs: intercept reads to overlay files Miklos Szeredi <miklos@szeredi.hu> - 2017-02-20 10:00 +0100
    Re: [PATCH 0/9] overlay: fix inconsistency of ro file after copy-up Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-19 10:20 +0100
      Re: [PATCH 0/9] overlay: fix inconsistency of ro file after copy-up Miklos Szeredi <miklos@szeredi.hu> - 2017-02-20 16:20 +0100

#1583574 — [PATCH 0/9] overlay: fix inconsistency of ro file after copy-up

A file is opened for read-only, opened read-write (resulting in a copy up)
and modified.  The data read back from the the read-only fd will be stale
in this case (the read-only file descriptor still refers to the lower,
unmodified file).

This patchset fixes issues related to this corner case.  This is a
requirement from various parties for accepting overlayfs as a "POSIX"
filesystem.

When an operation (read, mmap, fsync) is done on an overlay fd opened
read-only that is referring to a lower file, check if it has been copied up
in the mean time.  If so, open the upper file and use that for the operation.

To make the performance impact minimal for non-overlay case, use a flag in
file->f_mode to indicate that this is an overlay file.

Git tree is here:

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git #overlay-ro-rw-fix

Miklos Szeredi (9):
  vfs: extract common parts of {compat_,}do_readv_writev()
  vfs: pass type instead of fn to do_{loop,iter}_readv_writev()
  vfs: use helpers for calling f_op->{read,write}_iter()
  vfs: intercept reads to overlay files
  mm: ovl: copy-up on MAP_SHARED
  mm: use helper for calling f_op->mmap()
  ovl: intercept mmap on overlay files
  vfs: use helper for calling f_op->fsync()
  vfs: intercept fsync on overlay files

 drivers/block/loop.c                   |   4 +-
 drivers/gpu/drm/i915/i915_gem_dmabuf.c |   2 +-
 drivers/gpu/drm/vgem/vgem_drv.c        |   2 +-
 fs/Makefile                            |   2 +-
 fs/aio.c                               |   4 +-
 fs/coda/file.c                         |   2 +-
 fs/open.c                              |   2 +
 fs/overlay_util.c                      |  77 +++++++++++++++++++++
 fs/read_write.c                        | 121 +++++++++++++--------------------
 fs/splice.c                            |   2 +-
 fs/sync.c                              |   2 +-
 include/linux/fs.h                     |  42 ++++++++++++
 include/linux/overlay_util.h           |  17 +++++
 ipc/shm.c                              |   4 +-
 mm/mmap.c                              |   5 +-
 mm/nommu.c                             |   4 +-
 mm/util.c                              |  22 ++++++
 17 files changed, 227 insertions(+), 87 deletions(-)
 create mode 100644 fs/overlay_util.c
 create mode 100644 include/linux/overlay_util.h

-- 
2.5.5

[toc] | [next] | [standalone]

#1583579 — [PATCH 9/9] vfs: intercept fsync on overlay files

Fsync is really an inode operation (AFAICS) so a doing it on a O_RDONLY
file descriptor should flush any data written through an O_WRONLY file
descriptor for example.

To make this work correctly in case the file is copied up after being
opened for read, intercept the fsync operation, similarly to read_iter and
mmap.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlay_util.c            | 19 +++++++++++++++++++
 include/linux/fs.h           |  3 +++
 include/linux/overlay_util.h |  2 ++
 3 files changed, 24 insertions(+)

diff --git a/fs/overlay_util.c b/fs/overlay_util.c
index 063f8c5719d1..dae9d23b1519 100644
--- a/fs/overlay_util.c
+++ b/fs/overlay_util.c
@@ -37,6 +37,25 @@ ssize_t overlay_read_iter(struct file *file, struct kiocb *kio,
 }
 EXPORT_SYMBOL(overlay_read_iter);
 
+int overlay_fsync(struct file *file, loff_t start, loff_t end,
+		  int datasync)
+{
+	int ret;
+
+	if (likely(overlay_file_consistent(file)))
+		return file->f_op->fsync(file, start, end, datasync);
+
+	file = filp_clone_open(file);
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	ret = vfs_fsync_range(file, start, end, datasync);
+	fput(file);
+
+	return ret;
+}
+EXPORT_SYMBOL(overlay_fsync);
+
 int overlay_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	if (unlikely(!overlay_file_consistent(file))) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1293005de2e3..1cfa3b33659c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1748,6 +1748,9 @@ static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
 static inline int call_fsync(struct file *file, loff_t start, loff_t end,
 			     int datasync)
 {
+	if (unlikely(is_overlay_file(file)))
+		return overlay_fsync(file, start, end, datasync);
+
 	return file->f_op->fsync(file, start, end, datasync);
 }
 
diff --git a/include/linux/overlay_util.h b/include/linux/overlay_util.h
index 2cd66bc316cc..06a0dd923404 100644
--- a/include/linux/overlay_util.h
+++ b/include/linux/overlay_util.h
@@ -11,5 +11,7 @@ struct vm_area_struct;
 extern ssize_t overlay_read_iter(struct file *file, struct kiocb *kio,
 				 struct iov_iter *iter);
 extern int overlay_mmap(struct file *file, struct vm_area_struct *vma);
+extern int overlay_fsync(struct file *file, loff_t start, loff_t end,
+			 int datasync);
 
 #endif /* _LINUX_OVERLAY_FS_H */
-- 
2.5.5

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

#1583580 — [PATCH 8/9] vfs: use helper for calling f_op->fsync()

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/sync.c          | 2 +-
 include/linux/fs.h | 6 ++++++
 ipc/shm.c          | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/sync.c b/fs/sync.c
index 2a54c1f22035..11ba023434b1 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -192,7 +192,7 @@ int vfs_fsync_range(struct file *file, loff_t start, loff_t end, int datasync)
 		spin_unlock(&inode->i_lock);
 		mark_inode_dirty_sync(inode);
 	}
-	return file->f_op->fsync(file, start, end, datasync);
+	return call_fsync(file, start, end, datasync);
 }
 EXPORT_SYMBOL(vfs_fsync_range);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6e74b726c3ca..1293005de2e3 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1745,6 +1745,12 @@ static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
 	return file->f_op->mmap(file, vma);
 }
 
+static inline int call_fsync(struct file *file, loff_t start, loff_t end,
+			     int datasync)
+{
+	return file->f_op->fsync(file, start, end, datasync);
+}
+
 ssize_t rw_copy_check_uvector(int type, const struct iovec __user * uvector,
 			      unsigned long nr_segs, unsigned long fast_segs,
 			      struct iovec *fast_pointer,
diff --git a/ipc/shm.c b/ipc/shm.c
index 4329fe3ef594..258aff2e03bb 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -452,7 +452,7 @@ static int shm_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 
 	if (!sfd->file->f_op->fsync)
 		return -EINVAL;
-	return sfd->file->f_op->fsync(sfd->file, start, end, datasync);
+	return call_fsync(sfd->file, start, end, datasync);
 }
 
 static long shm_fallocate(struct file *file, int mode, loff_t offset,
-- 
2.5.5

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

#1583581 — [PATCH 4/9] vfs: intercept reads to overlay files

...in order to handle the corner case when the file is copyied up after
being opened read-only.

Can be verified with the following script:

 - 8< - - - - - 8< - - - - - 8< - - - - - 8< - - - -
cd /
rm -rf /tmp/ovl-rorw-test
mkdir /tmp/ovl-rorw-test
cd /tmp/ovl-rorw-test
mkdir -p mnt lower upper work
echo baba > lower/foo
mount -t overlay overlay -olowerdir=lower,upperdir=upper,workdir=work mnt
exec 3< mnt/foo
echo bubu > mnt/foo
cat <&3
exec 3>&-
umount mnt
 - 8< - - - - - 8< - - - - - 8< - - - - - 8< - - - -

Correct output is "bubu", incorrect output is "baba".

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/Makefile                  |  2 +-
 fs/open.c                    |  2 ++
 fs/overlay_util.c            | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h           | 13 +++++++++++++
 include/linux/overlay_util.h | 13 +++++++++++++
 5 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 fs/overlay_util.c
 create mode 100644 include/linux/overlay_util.h

diff --git a/fs/Makefile b/fs/Makefile
index 7bbaca9c67b1..8c8f197d7c75 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -11,7 +11,7 @@ obj-y :=	open.o read_write.o file_table.o super.o \
 		attr.o bad_inode.o file.o filesystems.o namespace.o \
 		seq_file.o xattr.o libfs.o fs-writeback.o \
 		pnode.o splice.o sync.o utimes.o \
-		stack.o fs_struct.o statfs.o fs_pin.o nsfs.o
+		stack.o fs_struct.o statfs.o fs_pin.o nsfs.o overlay_util.o
 
 ifeq ($(CONFIG_BLOCK),y)
 obj-y +=	buffer.o block_dev.o direct-io.o mpage.o
diff --git a/fs/open.c b/fs/open.c
index 9921f70bc5ca..4916ccff29f5 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -762,6 +762,8 @@ static int do_dentry_open(struct file *f,
 	if ((f->f_mode & FMODE_WRITE) &&
 	     likely(f->f_op->write || f->f_op->write_iter))
 		f->f_mode |= FMODE_CAN_WRITE;
+	if (unlikely(d_inode(f->f_path.dentry) != inode))
+		f->f_mode |= FMODE_OVERLAY;
 
 	f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
diff --git a/fs/overlay_util.c b/fs/overlay_util.c
new file mode 100644
index 000000000000..0daff19bad0b
--- /dev/null
+++ b/fs/overlay_util.c
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#if IS_ENABLED(CONFIG_OVERLAY_FS)
+
+#include <linux/overlay_util.h>
+#include <linux/fs.h>
+#include <linux/file.h>
+#include "internal.h"
+
+static bool overlay_file_consistent(struct file *file)
+{
+	return d_real_inode(file->f_path.dentry) == file_inode(file);
+}
+
+ssize_t overlay_read_iter(struct file *file, struct kiocb *kio,
+			  struct iov_iter *iter)
+{
+	ssize_t ret;
+
+	if (likely(overlay_file_consistent(file)))
+		return file->f_op->read_iter(kio, iter);
+
+	file = filp_clone_open(file);
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	ret = vfs_iter_read(file, iter, &kio->ki_pos);
+	fput(file);
+
+	return ret;
+}
+EXPORT_SYMBOL(overlay_read_iter);
+
+#endif /* IS_ENABLED(CONFIG_OVERLAY_FS) */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index efdaad954b70..d186d5390e99 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -31,6 +31,7 @@
 #include <linux/workqueue.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/delayed_call.h>
+#include <linux/overlay_util.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
@@ -143,6 +144,9 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 /* File was opened by fanotify and shouldn't generate fanotify events */
 #define FMODE_NONOTIFY		((__force fmode_t)0x4000000)
 
+/* File comes from overlay filesystem */
+#define FMODE_OVERLAY		((__force fmode_t)0x8000000)
+
 /*
  * Flag for rw_copy_check_uvector and compat_rw_copy_check_uvector
  * that indicates that they should check the contents of the iovec are
@@ -1712,9 +1716,18 @@ struct inode_operations {
 	int (*set_acl)(struct inode *, struct posix_acl *, int);
 } ____cacheline_aligned;
 
+
+static inline bool is_overlay_file(struct file *file)
+{
+	return IS_ENABLED(CONFIG_OVERLAY_FS) && file->f_mode & FMODE_OVERLAY;
+}
+
 static inline ssize_t call_read_iter(struct file *file, struct kiocb *kio,
 				     struct iov_iter *iter)
 {
+	if (unlikely(is_overlay_file(file)))
+		return overlay_read_iter(file, kio, iter);
+
 	return file->f_op->read_iter(kio, iter);
 }
 
diff --git a/include/linux/overlay_util.h b/include/linux/overlay_util.h
new file mode 100644
index 000000000000..886be9003bf3
--- /dev/null
+++ b/include/linux/overlay_util.h
@@ -0,0 +1,13 @@
+#ifndef _LINUX_OVERLAY_FS_H
+#define _LINUX_OVERLAY_FS_H
+
+#include <linux/types.h>
+
+struct file;
+struct kiocb;
+struct iov_iter;
+
+extern ssize_t overlay_read_iter(struct file *file, struct kiocb *kio,
+				 struct iov_iter *iter);
+
+#endif /* _LINUX_OVERLAY_FS_H */
-- 
2.5.5

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

#1584081 — Re: [PATCH 4/9] vfs: intercept reads to overlay files

On Fri, Feb 17, 2017 at 05:09:33PM +0100, Miklos Szeredi wrote:
> ...in order to handle the corner case when the file is copyied up after
> being opened read-only.

> --- /dev/null
> +++ b/fs/overlay_util.c
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright (C) 2017 Red Hat, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +#if IS_ENABLED(CONFIG_OVERLAY_FS)

This is crap - it should be handled in fs/Makefile, not with IS_ENABLED.

> +#include <linux/overlay_util.h>
> +#include <linux/fs.h>
> +#include <linux/file.h>
> +#include "internal.h"
> +
> +static bool overlay_file_consistent(struct file *file)
> +{
> +	return d_real_inode(file->f_path.dentry) == file_inode(file);
> +}
> +
> +ssize_t overlay_read_iter(struct file *file, struct kiocb *kio,
> +			  struct iov_iter *iter)
> +{
> +	ssize_t ret;
> +
> +	if (likely(overlay_file_consistent(file)))
> +		return file->f_op->read_iter(kio, iter);
> +
> +	file = filp_clone_open(file);
> +	if (IS_ERR(file))
> +		return PTR_ERR(file);
> +
> +	ret = vfs_iter_read(file, iter, &kio->ki_pos);
> +	fput(file);

You do realize that a bunch of such calls will breed arseloads of struct file,
right?  Freeing is delayed...

> +static inline bool is_overlay_file(struct file *file)
> +{
> +	return IS_ENABLED(CONFIG_OVERLAY_FS) && file->f_mode & FMODE_OVERLAY;
> +}
> +
>  static inline ssize_t call_read_iter(struct file *file, struct kiocb *kio,
>  				     struct iov_iter *iter)
>  {
> +	if (unlikely(is_overlay_file(file)))
> +		return overlay_read_iter(file, kio, iter);
> +
>  	return file->f_op->read_iter(kio, iter);
>  }

1) that IS_ENABLED is fairly pointless and it's not obvious that nobody
else will use that flag

2) what that check should include is overlay_file_consistent(), with
no method call in overlay_read_iter().

3) anything that does a plenty of calls of kernel_read() is going to be
very unpleasantly surprised by the effects of that thing.

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

#1584084 — Re: [PATCH 4/9] vfs: intercept reads to overlay files

On Sun, Feb 19, 2017 at 10:05 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Feb 17, 2017 at 05:09:33PM +0100, Miklos Szeredi wrote:
>> ...in order to handle the corner case when the file is copyied up after
>> being opened read-only.
>
>> --- /dev/null
>> +++ b/fs/overlay_util.c
>> @@ -0,0 +1,39 @@
>> +/*
>> + * Copyright (C) 2017 Red Hat, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + */
>> +#if IS_ENABLED(CONFIG_OVERLAY_FS)
>
> This is crap - it should be handled in fs/Makefile, not with IS_ENABLED.

This is needed if overlay is built in or a module.  Couldn't figure
out how makefile could handle that.

>
>> +#include <linux/overlay_util.h>
>> +#include <linux/fs.h>
>> +#include <linux/file.h>
>> +#include "internal.h"
>> +
>> +static bool overlay_file_consistent(struct file *file)
>> +{
>> +     return d_real_inode(file->f_path.dentry) == file_inode(file);
>> +}
>> +
>> +ssize_t overlay_read_iter(struct file *file, struct kiocb *kio,
>> +                       struct iov_iter *iter)
>> +{
>> +     ssize_t ret;
>> +
>> +     if (likely(overlay_file_consistent(file)))
>> +             return file->f_op->read_iter(kio, iter);
>> +
>> +     file = filp_clone_open(file);
>> +     if (IS_ERR(file))
>> +             return PTR_ERR(file);
>> +
>> +     ret = vfs_iter_read(file, iter, &kio->ki_pos);
>> +     fput(file);
>
> You do realize that a bunch of such calls will breed arseloads of struct file,
> right?  Freeing is delayed...

No, I hadn't realized that.  Could we force freeing file here?

>
>> +static inline bool is_overlay_file(struct file *file)
>> +{
>> +     return IS_ENABLED(CONFIG_OVERLAY_FS) && file->f_mode & FMODE_OVERLAY;
>> +}
>> +
>>  static inline ssize_t call_read_iter(struct file *file, struct kiocb *kio,
>>                                    struct iov_iter *iter)
>>  {
>> +     if (unlikely(is_overlay_file(file)))
>> +             return overlay_read_iter(file, kio, iter);
>> +
>>       return file->f_op->read_iter(kio, iter);
>>  }
>
> 1) that IS_ENABLED is fairly pointless and it's not obvious that nobody
> else will use that flag

It's mean to be a micro-optimization for the CONFIG_OVERLAYFS=n case.

>
> 2) what that check should include is overlay_file_consistent(), with
> no method call in overlay_read_iter().

This is again a micro-optimization for the case when this is not an
overlay file.  Which is the very very likely case.

What's the problem with putting that test in the non-inline function?

>
> 3) anything that does a plenty of calls of kernel_read() is going to be
> very unpleasantly surprised by the effects of that thing.

Why is that?

Thanks,
Miklos

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

#1584397 — Re: [PATCH 4/9] vfs: intercept reads to overlay files

On 2017/2/18 00:10, Miklos Szeredi wrote:
> diff --git a/fs/open.c b/fs/open.c
> index 9921f70bc5ca..4916ccff29f5 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -762,6 +762,8 @@ static int do_dentry_open(struct file *f,
>  	if ((f->f_mode & FMODE_WRITE) &&
>  	     likely(f->f_op->write || f->f_op->write_iter))
>  		f->f_mode |= FMODE_CAN_WRITE;
> +	if (unlikely(d_inode(f->f_path.dentry) != inode))
> +		f->f_mode |= FMODE_OVERLAY;

Can we just add flag to the "readonly && not copied" file, not all overlayfs files?
Beacuse we just want to check the ro file before copied-up.

>  	f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
>  
> diff --git a/fs/overlay_util.c b/fs/overlay_util.c new file mode 100644 index 000000000000..0daff19bad0b
> --- /dev/null
> +++ b/fs/overlay_util.c
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright (C) 2017 Red Hat, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify 
> +it
> + * under the terms of the GNU General Public License version 2 as 
> +published by
> + * the Free Software Foundation.
> + */
> +#if IS_ENABLED(CONFIG_OVERLAY_FS)
> +
> +#include <linux/overlay_util.h>
> +#include <linux/fs.h>
> +#include <linux/file.h>
> +#include "internal.h"
> +
> +static bool overlay_file_consistent(struct file *file) {
> +	return d_real_inode(file->f_path.dentry) == file_inode(file); }
> +
> +ssize_t overlay_read_iter(struct file *file, struct kiocb *kio,
> +			  struct iov_iter *iter)
> +{
> +	ssize_t ret;
> +
> +	if (likely(overlay_file_consistent(file)))
> +		return file->f_op->read_iter(kio, iter);
> +
> +	file = filp_clone_open(file);
> +	if (IS_ERR(file))
> +		return PTR_ERR(file);
> +
> +	ret = vfs_iter_read(file, iter, &kio->ki_pos);
> +	fput(file);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(overlay_read_iter);

Can we try to replace the old file with the new file, and then clear the f_mode flag we added?
If so, we can avoid opening file for each reading call and avoid copied file consistency check.

Thanks.

zhangyi

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

#1584423 — Re: [PATCH 4/9] vfs: intercept reads to overlay files

On Mon, Feb 20, 2017 at 8:47 AM, zhangyi (F) <yi.zhang@huawei.com> wrote:
> On 2017/2/18 00:10, Miklos Szeredi wrote:
>> diff --git a/fs/open.c b/fs/open.c
>> index 9921f70bc5ca..4916ccff29f5 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -762,6 +762,8 @@ static int do_dentry_open(struct file *f,
>>       if ((f->f_mode & FMODE_WRITE) &&
>>            likely(f->f_op->write || f->f_op->write_iter))
>>               f->f_mode |= FMODE_CAN_WRITE;
>> +     if (unlikely(d_inode(f->f_path.dentry) != inode))
>> +             f->f_mode |= FMODE_OVERLAY;
>
> Can we just add flag to the "readonly && not copied" file, not all overlayfs files?
> Beacuse we just want to check the ro file before copied-up.

Can't do it without adding new infrastructure.  Likely not worth it.

>
>>       f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
>>
>> diff --git a/fs/overlay_util.c b/fs/overlay_util.c new file mode 100644 index 000000000000..0daff19bad0b
>> --- /dev/null
>> +++ b/fs/overlay_util.c
>> @@ -0,0 +1,39 @@
>> +/*
>> + * Copyright (C) 2017 Red Hat, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> +it
>> + * under the terms of the GNU General Public License version 2 as
>> +published by
>> + * the Free Software Foundation.
>> + */
>> +#if IS_ENABLED(CONFIG_OVERLAY_FS)
>> +
>> +#include <linux/overlay_util.h>
>> +#include <linux/fs.h>
>> +#include <linux/file.h>
>> +#include "internal.h"
>> +
>> +static bool overlay_file_consistent(struct file *file) {
>> +     return d_real_inode(file->f_path.dentry) == file_inode(file); }
>> +
>> +ssize_t overlay_read_iter(struct file *file, struct kiocb *kio,
>> +                       struct iov_iter *iter)
>> +{
>> +     ssize_t ret;
>> +
>> +     if (likely(overlay_file_consistent(file)))
>> +             return file->f_op->read_iter(kio, iter);
>> +
>> +     file = filp_clone_open(file);
>> +     if (IS_ERR(file))
>> +             return PTR_ERR(file);
>> +
>> +     ret = vfs_iter_read(file, iter, &kio->ki_pos);
>> +     fput(file);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(overlay_read_iter);
>
> Can we try to replace the old file with the new file, and then clear the f_mode flag we added?
> If so, we can avoid opening file for each reading call and avoid copied file consistency check.

Could probably try replacing file in fd table.  But no point in doing
so without having an actual real life use case that would benefit from
that.  AFAIK there's no such thing.

And there are other pointers to file that can't be replaced, so this
fallback would need to be kept around.

Thanks,
Miklos

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

#1584082

On Fri, Feb 17, 2017 at 05:09:29PM +0100, Miklos Szeredi wrote:
> A file is opened for read-only, opened read-write (resulting in a copy up)
> and modified.  The data read back from the the read-only fd will be stale
> in this case (the read-only file descriptor still refers to the lower,
> unmodified file).
> 
> This patchset fixes issues related to this corner case.  This is a
> requirement from various parties for accepting overlayfs as a "POSIX"
> filesystem.
> 
> When an operation (read, mmap, fsync) is done on an overlay fd opened
> read-only that is referring to a lower file, check if it has been copied up
> in the mean time.  If so, open the upper file and use that for the operation.
> 
> To make the performance impact minimal for non-overlay case, use a flag in
> file->f_mode to indicate that this is an overlay file.

This is one hell of a DoS vector - it's really easy to eat tons of struct
file that way.  Preparatory parts of that series make sense on their own,
but your "let's allocate a struct file, call ->open() and schedule that
struct file for closing upon the exit to userland on each kernel_read()"
is not.

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

#1584659

On Sun, Feb 19, 2017 at 10:14 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:

> This is one hell of a DoS vector - it's really easy to eat tons of struct
> file that way.  Preparatory parts of that series make sense on their own,
> but your "let's allocate a struct file, call ->open() and schedule that
> struct file for closing upon the exit to userland on each kernel_read()"
> is not.

Found a couple of instances of this pattern (haven't looked very hard,
possibly there's more):

nfsd_read()
cachefiles_write_page()

How come this hasn't been a problem for them?

Would flush_delayed_fput() work here?  I couldn't really find what the
locking issues with synchronous fputs were.

Thanks,
Miklos

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web