From 3428030da004a1128cbdcf93dc03e16f184d845b Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Tue, 22 Jan 2019 07:01:39 +0200 Subject: ovl: fix missing upper fs freeze protection on copy up for ioctl Generalize the helper ovl_open_maybe_copy_up() and use it to copy up file with data before FS_IOC_SETFLAGS ioctl. The FS_IOC_SETFLAGS ioctl is a bit of an odd ball in vfs, which probably caused the confusion. File may be open O_RDONLY, but ioctl modifies the file. VFS does not call mnt_want_write_file() nor lock inode mutex, but fs-specific code for FS_IOC_SETFLAGS does. So ovl_ioctl() calls mnt_want_write_file() for the overlay file, and fs-specific code calls mnt_want_write_file() for upper fs file, but there was no call for ovl_want_write() for copy up duration which prevents overlayfs from copying up on a frozen upper fs. Fixes: dab5ca8fd9dd ("ovl: add lsattr/chattr support") Cc: # v4.19 Signed-off-by: Amir Goldstein Acked-by: Vivek Goyal Signed-off-by: Miklos Szeredi --- fs/overlayfs/copy_up.c | 6 +++--- fs/overlayfs/file.c | 5 ++--- fs/overlayfs/overlayfs.h | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) (limited to 'fs/overlayfs') diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 68b3303e4b46..56feaa739979 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -909,14 +909,14 @@ static bool ovl_open_need_copy_up(struct dentry *dentry, int flags) return true; } -int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags) +int ovl_maybe_copy_up(struct dentry *dentry, int flags) { int err = 0; - if (ovl_open_need_copy_up(dentry, file_flags)) { + if (ovl_open_need_copy_up(dentry, flags)) { err = ovl_want_write(dentry); if (!err) { - err = ovl_copy_up_flags(dentry, file_flags); + err = ovl_copy_up_flags(dentry, flags); ovl_drop_write(dentry); } } diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 84dd957efa24..50e4407398d8 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -116,11 +116,10 @@ static int ovl_real_fdget(const struct file *file, struct fd *real) static int ovl_open(struct inode *inode, struct file *file) { - struct dentry *dentry = file_dentry(file); struct file *realfile; int err; - err = ovl_open_maybe_copy_up(dentry, file->f_flags); + err = ovl_maybe_copy_up(file_dentry(file), file->f_flags); if (err) return err; @@ -390,7 +389,7 @@ static long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) if (ret) return ret; - ret = ovl_copy_up_with_data(file_dentry(file)); + ret = ovl_maybe_copy_up(file_dentry(file), O_WRONLY); if (!ret) { ret = ovl_real_ioctl(file, cmd, arg); diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index 9c6018287d57..d26efed9f80a 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -421,7 +421,7 @@ extern const struct file_operations ovl_file_operations; int ovl_copy_up(struct dentry *dentry); int ovl_copy_up_with_data(struct dentry *dentry); int ovl_copy_up_flags(struct dentry *dentry, int flags); -int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags); +int ovl_maybe_copy_up(struct dentry *dentry, int flags); int ovl_copy_xattr(struct dentry *old, struct dentry *new); int ovl_set_attr(struct dentry *upper, struct kstat *stat); struct ovl_fh *ovl_encode_real_fh(struct dentry *real, bool is_upper); -- cgit v1.2.3-58-ga151 From 9e46b840c7053b5f7a245e98cd239b60d189a96c Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Wed, 27 Feb 2019 13:32:11 +0200 Subject: ovl: support stacked SEEK_HOLE/SEEK_DATA Overlay file f_pos is the master copy that is preserved through copy up and modified on read/write, but only real fs knows how to SEEK_HOLE/SEEK_DATA and real fs may impose limitations that are more strict than ->s_maxbytes for specific files, so we use the real file to perform seeks. We do not call real fs for SEEK_CUR:0 query and for SEEK_SET:0 requests. Fixes: d1d04ef8572b ("ovl: stack file ops") Reported-by: Eddie Horng Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/file.c | 44 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4 deletions(-) (limited to 'fs/overlayfs') diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 50e4407398d8..ddfd93f13cc5 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -144,11 +144,47 @@ static int ovl_release(struct inode *inode, struct file *file) static loff_t ovl_llseek(struct file *file, loff_t offset, int whence) { - struct inode *realinode = ovl_inode_real(file_inode(file)); + struct inode *inode = file_inode(file); + struct fd real; + const struct cred *old_cred; + ssize_t ret; + + /* + * The two special cases below do not need to involve real fs, + * so we can optimizing concurrent callers. + */ + if (offset == 0) { + if (whence == SEEK_CUR) + return file->f_pos; + + if (whence == SEEK_SET) + return vfs_setpos(file, 0, 0); + } + + ret = ovl_real_fdget(file, &real); + if (ret) + return ret; - return generic_file_llseek_size(file, offset, whence, - realinode->i_sb->s_maxbytes, - i_size_read(realinode)); + /* + * Overlay file f_pos is the master copy that is preserved + * through copy up and modified on read/write, but only real + * fs knows how to SEEK_HOLE/SEEK_DATA and real fs may impose + * limitations that are more strict than ->s_maxbytes for specific + * files, so we use the real file to perform seeks. + */ + inode_lock(inode); + real.file->f_pos = file->f_pos; + + old_cred = ovl_override_creds(inode->i_sb); + ret = vfs_llseek(real.file, offset, whence); + revert_creds(old_cred); + + file->f_pos = real.file->f_pos; + inode_unlock(inode); + + fdput(real); + + return ret; } static void ovl_file_accessed(struct file *file) -- cgit v1.2.3-58-ga151 From d989903058a83e8536cc7aadf9256a47d5c173fe Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Wed, 24 Apr 2019 19:39:50 +0300 Subject: ovl: do not generate duplicate fsnotify events for "fake" path Overlayfs "fake" path is used for stacked file operations on underlying files. Operations on files with "fake" path must not generate fsnotify events with path data, because those events have already been generated at overlayfs layer and because the reported event->fd for fanotify marks on underlying inode/filesystem will have the wrong path (the overlayfs path). Link: https://lore.kernel.org/linux-fsdevel/20190423065024.12695-1-jencce.kernel@gmail.com/ Reported-by: Murphy Zhou Fixes: d1d04ef8572b ("ovl: stack file ops") Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/file.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'fs/overlayfs') diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index ddfd93f13cc5..7d2f01957e40 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -29,10 +29,11 @@ static struct file *ovl_open_realfile(const struct file *file, struct inode *inode = file_inode(file); struct file *realfile; const struct cred *old_cred; + int flags = file->f_flags | O_NOATIME | FMODE_NONOTIFY; old_cred = ovl_override_creds(inode->i_sb); - realfile = open_with_fake_path(&file->f_path, file->f_flags | O_NOATIME, - realinode, current_cred()); + realfile = open_with_fake_path(&file->f_path, flags, realinode, + current_cred()); revert_creds(old_cred); pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n", @@ -50,7 +51,7 @@ static int ovl_change_flags(struct file *file, unsigned int flags) int err; /* No atime modificaton on underlying */ - flags |= O_NOATIME; + flags |= O_NOATIME | FMODE_NONOTIFY; /* If some flag changed that cannot be changed then something's amiss */ if (WARN_ON((file->f_flags ^ flags) & ~OVL_SETFL_MASK)) -- cgit v1.2.3-58-ga151 From 98487de318a6f33312471ae1e2afa16fbf8361fe Mon Sep 17 00:00:00 2001 From: Jiufei Xue Date: Mon, 6 May 2019 15:41:02 +0800 Subject: ovl: check the capability before cred overridden We found that it return success when we set IMMUTABLE_FL flag to a file in docker even though the docker didn't have the capability CAP_LINUX_IMMUTABLE. The commit d1d04ef8572b ("ovl: stack file ops") and dab5ca8fd9dd ("ovl: add lsattr/chattr support") implemented chattr operations on a regular overlay file. ovl_real_ioctl() overridden the current process's subjective credentials with ofs->creator_cred which have the capability CAP_LINUX_IMMUTABLE so that it will return success in vfs_ioctl()->cap_capable(). Fix this by checking the capability before cred overridden. And here we only care about APPEND_FL and IMMUTABLE_FL, so get these information from inode. [SzM: move check and call to underlying fs inside inode locked region to prevent two such calls from racing with each other] Signed-off-by: Jiufei Xue Signed-off-by: Miklos Szeredi --- fs/overlayfs/file.c | 79 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 61 insertions(+), 18 deletions(-) (limited to 'fs/overlayfs') diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index 7d2f01957e40..540a8b845145 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -11,6 +11,7 @@ #include #include #include +#include #include "overlayfs.h" static char ovl_whatisit(struct inode *inode, struct inode *realinode) @@ -408,10 +409,68 @@ static long ovl_real_ioctl(struct file *file, unsigned int cmd, return ret; } -static long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +static unsigned int ovl_get_inode_flags(struct inode *inode) +{ + unsigned int flags = READ_ONCE(inode->i_flags); + unsigned int ovl_iflags = 0; + + if (flags & S_SYNC) + ovl_iflags |= FS_SYNC_FL; + if (flags & S_APPEND) + ovl_iflags |= FS_APPEND_FL; + if (flags & S_IMMUTABLE) + ovl_iflags |= FS_IMMUTABLE_FL; + if (flags & S_NOATIME) + ovl_iflags |= FS_NOATIME_FL; + + return ovl_iflags; +} + +static long ovl_ioctl_set_flags(struct file *file, unsigned long arg) { long ret; struct inode *inode = file_inode(file); + unsigned int flags; + unsigned int old_flags; + + if (!inode_owner_or_capable(inode)) + return -EACCES; + + if (get_user(flags, (int __user *) arg)) + return -EFAULT; + + ret = mnt_want_write_file(file); + if (ret) + return ret; + + inode_lock(inode); + + /* Check the capability before cred override */ + ret = -EPERM; + old_flags = ovl_get_inode_flags(inode); + if (((flags ^ old_flags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) && + !capable(CAP_LINUX_IMMUTABLE)) + goto unlock; + + ret = ovl_maybe_copy_up(file_dentry(file), O_WRONLY); + if (ret) + goto unlock; + + ret = ovl_real_ioctl(file, FS_IOC_SETFLAGS, arg); + + ovl_copyflags(ovl_inode_real(inode), inode); +unlock: + inode_unlock(inode); + + mnt_drop_write_file(file); + + return ret; + +} + +static long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{ + long ret; switch (cmd) { case FS_IOC_GETFLAGS: @@ -419,23 +478,7 @@ static long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) break; case FS_IOC_SETFLAGS: - if (!inode_owner_or_capable(inode)) - return -EACCES; - - ret = mnt_want_write_file(file); - if (ret) - return ret; - - ret = ovl_maybe_copy_up(file_dentry(file), O_WRONLY); - if (!ret) { - ret = ovl_real_ioctl(file, cmd, arg); - - inode_lock(inode); - ovl_copyflags(ovl_inode_real(inode), inode); - inode_unlock(inode); - } - - mnt_drop_write_file(file); + ret = ovl_ioctl_set_flags(file, arg); break; default: -- cgit v1.2.3-58-ga151 From acf3062a7e1ccf67c6f7e7c28671a6708fde63b0 Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Thu, 28 Mar 2019 17:38:29 +0200 Subject: ovl: relax WARN_ON() for overlapping layers use case This nasty little syzbot repro: https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000 Creates overlay mounts where the same directory is both in upper and lower layers. Simplified example: mkdir foo work mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work" The repro runs several threads in parallel that attempt to chdir into foo and attempt to symlink/rename/exec/mkdir the file bar. The repro hits a WARN_ON() I placed in ovl_instantiate(), which suggests that an overlay inode already exists in cache and is hashed by the pointer of the real upper dentry that ovl_create_real() has just created. At the point of the WARN_ON(), for overlay dir inode lock is held and upper dir inode lock, so at first, I did not see how this was possible. On a closer look, I see that after ovl_create_real(), because of the overlapping upper and lower layers, a lookup by another thread can find the file foo/bar that was just created in upper layer, at overlay path foo/foo/bar and hash the an overlay inode with the new real dentry as lower dentry. This is possible because the overlay directory foo/foo is not locked and the upper dentry foo/bar is in dcache, so ovl_lookup() can find it without taking upper dir inode shared lock. Overlapping layers is considered a wrong setup which would result in unexpected behavior, but it shouldn't crash the kernel and it shouldn't trigger WARN_ON() either, so relax this WARN_ON() and leave a pr_warn() instead to cover all cases of failure to get an overlay inode. The error returned from failure to insert new inode to cache with inode_insert5() was changed to -EEXIST, to distinguish from the error -ENOMEM returned on failure to get/allocate inode with iget5_locked(). Reported-by: syzbot+9c69c282adc4edd2b540@syzkaller.appspotmail.com Fixes: 01b39dcc9568 ("ovl: use inode_insert5() to hash a newly...") Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/dir.c | 2 +- fs/overlayfs/inode.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) (limited to 'fs/overlayfs') diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index 82c129bfe58d..93872bb50230 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -260,7 +260,7 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode, * hashed directory inode aliases. */ inode = ovl_get_inode(dentry->d_sb, &oip); - if (WARN_ON(IS_ERR(inode))) + if (IS_ERR(inode)) return PTR_ERR(inode); } else { WARN_ON(ovl_inode_real(inode) != d_inode(newdentry)); diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c index 3b7ed5d2279c..b48273e846ad 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -832,7 +832,7 @@ struct inode *ovl_get_inode(struct super_block *sb, int fsid = bylower ? oip->lowerpath->layer->fsid : 0; bool is_dir, metacopy = false; unsigned long ino = 0; - int err = -ENOMEM; + int err = oip->newinode ? -EEXIST : -ENOMEM; if (!realinode) realinode = d_inode(lowerdentry); @@ -917,6 +917,7 @@ out: return inode; out_err: + pr_warn_ratelimited("overlayfs: failed to get inode (%i)\n", err); inode = ERR_PTR(err); goto out; } -- cgit v1.2.3-58-ga151