Following patches fix overlayfs bugs. The patches impact vfs code, so
posting for review to a wider audience.
They can also be found on the overlayfs-next branch:
git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next
Hoping to get these bugfixes into 4.14.
Thanks,
Miklos
---
Miklos Szeredi (3):
vfs: add flags to d_real()
ovl: fix relatime for directories
ovl: don't allow writing ioctl on lower layer
Documentation/filesystems/Locking | 2 +-
Documentation/filesystems/vfs.txt | 2 +-
fs/inode.c | 3 +-
fs/internal.h | 2 ++
fs/namespace.c | 64 +++++++++++++++++++++++++++++++++++++--
fs/open.c | 8 ++---
fs/overlayfs/super.c | 10 ++++--
fs/xattr.c | 9 +++---
include/linux/dcache.h | 15 ++++++---
include/linux/fs.h | 2 +-
10 files changed, 95 insertions(+), 22 deletions(-)
--
2.5.5
Need to treat non-regular overlayfs files the same as regular files when
checking for an atime update.
Add a d_real() flag to make it return the real dentry for all file types.
Reported-by: "zhangyi (F)" <[email protected]>
Signed-off-by: Miklos Szeredi <[email protected]>
---
fs/inode.c | 3 ++-
fs/overlayfs/super.c | 3 +++
include/linux/dcache.h | 3 +++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/inode.c b/fs/inode.c
index 50370599e371..16e85d3a46d1 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1570,7 +1570,8 @@ static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
bool rcu)
{
if (!rcu) {
- struct inode *realinode = d_real_inode(dentry);
+ struct inode *realinode =
+ d_inode(d_real(dentry, NULL, 0, D_REAL_ALL));
if (unlikely(inode != realinode) &&
(!timespec_equal(&inode->i_mtime, &realinode->i_mtime) ||
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 19e89ce39017..059ee2f19361 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -75,6 +75,9 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
struct dentry *real;
int err;
+ if (flags & D_REAL_ALL)
+ return ovl_dentry_real(dentry);
+
if (!d_is_reg(dentry)) {
if (!inode || inode == d_inode(dentry))
return dentry;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index fd0721e520f4..9594ef3382ff 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -562,6 +562,9 @@ static inline struct dentry *d_backing_dentry(struct dentry *upper)
return upper;
}
+/* d_real() flags */
+#define D_REAL_ALL 0x1 /* return real dentry for all file types */
+
/**
* d_real - Return the real dentry
* @dentry: the dentry to query
--
2.5.5
Problem with ioctl() is that it's a file operation, yet often used as an
inode operation (i.e. modify the inode despite the file being opened for
read-only).
mnt_want_write_file() is used by filesystems in such cases to get write
access on an arbitrary open file.
Since overlayfs lets filesystems do all file operations, including ioctl,
this can lead to mnt_want_write_file() returning OK for a lower file and
modification of that lower file.
This patch prevents modification by checking if the file is from an
overlayfs lower layer and returning EPERM in that case.
Need to introduce a mnt_want_write_file_path() variant that still does the
old thing for inode operations that can do the copy up + modification
correctly in such cases (fchown, fsetxattr, fremovexattr).
This does not address the correctness of such ioctls on overlayfs (the
correct way would be to copy up and attempt to perform ioctl on upper
file).
In theory this could be a regression. We very much hope that nobody is
relying on such a hack in any sane setup.
While this patch meddles in VFS code, it has no effect on non-overlayfs
filesystems.
Reported-by: "zhangyi (F)" <[email protected]>
Signed-off-by: Miklos Szeredi <[email protected]>
---
fs/internal.h | 2 ++
fs/namespace.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++---
fs/open.c | 4 ++--
fs/overlayfs/super.c | 3 +++
fs/xattr.c | 9 +++----
include/linux/dcache.h | 1 +
6 files changed, 74 insertions(+), 9 deletions(-)
diff --git a/fs/internal.h b/fs/internal.h
index 9676fe11c093..60cdbcd2887b 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -71,8 +71,10 @@ extern void __init mnt_init(void);
extern int __mnt_want_write(struct vfsmount *);
extern int __mnt_want_write_file(struct file *);
+extern int mnt_want_write_file_path(struct file *);
extern void __mnt_drop_write(struct vfsmount *);
extern void __mnt_drop_write_file(struct file *);
+extern void mnt_drop_write_file_path(struct file *);
/*
* fs_struct.c
diff --git a/fs/namespace.c b/fs/namespace.c
index f8893dc6a989..df0f7521979a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -431,13 +431,18 @@ int __mnt_want_write_file(struct file *file)
}
/**
- * mnt_want_write_file - get write access to a file's mount
+ * mnt_want_write_file_path - get write access to a file's mount
* @file: the file who's mount on which to take a write
*
* This is like mnt_want_write, but it takes a file and can
* do some optimisations if the file is open for write already
+ *
+ * Called by the vfs for cases when we have an open file at hand, but will do an
+ * inode operation on it (important distinction for files opened on overlayfs,
+ * since the file operations will come from the real underlying file, while
+ * inode operations come from the overlay).
*/
-int mnt_want_write_file(struct file *file)
+int mnt_want_write_file_path(struct file *file)
{
int ret;
@@ -447,6 +452,53 @@ int mnt_want_write_file(struct file *file)
sb_end_write(file->f_path.mnt->mnt_sb);
return ret;
}
+
+static inline int may_write_real(struct file *file)
+{
+ struct dentry *dentry = file->f_path.dentry;
+ struct dentry *upperdentry;
+
+ /* Writable file? */
+ if (file->f_mode & FMODE_WRITER)
+ return 0;
+
+ /* Not overlayfs? */
+ if (likely(!(dentry->d_flags & DCACHE_OP_REAL)))
+ return 0;
+
+ /* File refers to upper, writable layer? */
+ upperdentry = d_real(dentry, NULL, 0, D_REAL_UPPER);
+ if (upperdentry && file_inode(file) == d_inode(upperdentry))
+ return 0;
+
+ /* Lower layer: can't write to real file, sorry... */
+ return -EPERM;
+}
+
+/**
+ * mnt_want_write_file - get write access to a file's mount
+ * @file: the file who's mount on which to take a write
+ *
+ * This is like mnt_want_write, but it takes a file and can
+ * do some optimisations if the file is open for write already
+ *
+ * Mostly called by filesystems from their ioctl operation before performing
+ * modification. On overlayfs this needs to check if the file is on a read-only
+ * lower layer and deny access in that case.
+ */
+int mnt_want_write_file(struct file *file)
+{
+ int ret;
+
+ ret = may_write_real(file);
+ if (!ret) {
+ sb_start_write(file_inode(file)->i_sb);
+ ret = __mnt_want_write_file(file);
+ if (ret)
+ sb_end_write(file_inode(file)->i_sb);
+ }
+ return ret;
+}
EXPORT_SYMBOL_GPL(mnt_want_write_file);
/**
@@ -484,10 +536,16 @@ void __mnt_drop_write_file(struct file *file)
__mnt_drop_write(file->f_path.mnt);
}
-void mnt_drop_write_file(struct file *file)
+void mnt_drop_write_file_path(struct file *file)
{
mnt_drop_write(file->f_path.mnt);
}
+
+void mnt_drop_write_file(struct file *file)
+{
+ __mnt_drop_write(file->f_path.mnt);
+ sb_end_write(file_inode(file)->i_sb);
+}
EXPORT_SYMBOL(mnt_drop_write_file);
static int mnt_make_readonly(struct mount *mnt)
diff --git a/fs/open.c b/fs/open.c
index 6d5c9a9b8c8d..7ea118471dce 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -670,12 +670,12 @@ SYSCALL_DEFINE3(fchown, unsigned int, fd, uid_t, user, gid_t, group)
if (!f.file)
goto out;
- error = mnt_want_write_file(f.file);
+ error = mnt_want_write_file_path(f.file);
if (error)
goto out_fput;
audit_file(f.file);
error = chown_common(&f.file->f_path, user, group);
- mnt_drop_write_file(f.file);
+ mnt_drop_write_file_path(f.file);
out_fput:
fdput(f);
out:
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 059ee2f19361..0d53373b2ffc 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -78,6 +78,9 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
if (flags & D_REAL_ALL)
return ovl_dentry_real(dentry);
+ if (flags & D_REAL_UPPER)
+ return ovl_dentry_upper(dentry);
+
if (!d_is_reg(dentry)) {
if (!inode || inode == d_inode(dentry))
return dentry;
diff --git a/fs/xattr.c b/fs/xattr.c
index 464c94bf65f9..d7c2cf7817bb 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -23,6 +23,7 @@
#include <linux/posix_acl_xattr.h>
#include <linux/uaccess.h>
+#include "internal.h"
static const char *
strcmp_prefix(const char *a, const char *a_prefix)
@@ -496,10 +497,10 @@ SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name,
if (!f.file)
return error;
audit_file(f.file);
- error = mnt_want_write_file(f.file);
+ error = mnt_want_write_file_path(f.file);
if (!error) {
error = setxattr(f.file->f_path.dentry, name, value, size, flags);
- mnt_drop_write_file(f.file);
+ mnt_drop_write_file_path(f.file);
}
fdput(f);
return error;
@@ -728,10 +729,10 @@ SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)
if (!f.file)
return error;
audit_file(f.file);
- error = mnt_want_write_file(f.file);
+ error = mnt_want_write_file_path(f.file);
if (!error) {
error = removexattr(f.file->f_path.dentry, name);
- mnt_drop_write_file(f.file);
+ mnt_drop_write_file_path(f.file);
}
fdput(f);
return error;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 9594ef3382ff..bf5b7d57414b 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -564,6 +564,7 @@ static inline struct dentry *d_backing_dentry(struct dentry *upper)
/* d_real() flags */
#define D_REAL_ALL 0x1 /* return real dentry for all file types */
+#define D_REAL_UPPER 0x2 /* return upper dentry or NULL if non-upper */
/**
* d_real - Return the real dentry
--
2.5.5
Add a separate flags argument (in addition to the open flags) to control
the behavior of d_real().
Signed-off-by: Miklos Szeredi <[email protected]>
---
Documentation/filesystems/Locking | 2 +-
Documentation/filesystems/vfs.txt | 2 +-
fs/open.c | 4 ++--
fs/overlayfs/super.c | 4 ++--
include/linux/dcache.h | 11 ++++++-----
include/linux/fs.h | 2 +-
6 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index fe25787ff6d4..75d2d57e2c44 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -22,7 +22,7 @@ prototypes:
struct vfsmount *(*d_automount)(struct path *path);
int (*d_manage)(const struct path *, bool);
struct dentry *(*d_real)(struct dentry *, const struct inode *,
- unsigned int);
+ unsigned int, unsigned int);
locking rules:
rename_lock ->d_lock may block rcu-walk
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 73e7d91f03dc..7f20c1bdfb67 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -990,7 +990,7 @@ struct dentry_operations {
struct vfsmount *(*d_automount)(struct path *);
int (*d_manage)(const struct path *, bool);
struct dentry *(*d_real)(struct dentry *, const struct inode *,
- unsigned int);
+ unsigned int, unsigned int);
};
d_revalidate: called when the VFS needs to revalidate a dentry. This
diff --git a/fs/open.c b/fs/open.c
index 35bb784763a4..6d5c9a9b8c8d 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -96,7 +96,7 @@ long vfs_truncate(const struct path *path, loff_t length)
* write access on the upper inode, not on the overlay inode. For
* non-overlay filesystems d_real() is an identity function.
*/
- upperdentry = d_real(path->dentry, NULL, O_WRONLY);
+ upperdentry = d_real(path->dentry, NULL, O_WRONLY, 0);
error = PTR_ERR(upperdentry);
if (IS_ERR(upperdentry))
goto mnt_drop_write_and_out;
@@ -857,7 +857,7 @@ EXPORT_SYMBOL(file_path);
int vfs_open(const struct path *path, struct file *file,
const struct cred *cred)
{
- struct dentry *dentry = d_real(path->dentry, NULL, file->f_flags);
+ struct dentry *dentry = d_real(path->dentry, NULL, file->f_flags, 0);
if (IS_ERR(dentry))
return PTR_ERR(dentry);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index c0c02cca776b..19e89ce39017 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -70,7 +70,7 @@ static int ovl_check_append_only(struct inode *inode, int flag)
static struct dentry *ovl_d_real(struct dentry *dentry,
const struct inode *inode,
- unsigned int open_flags)
+ unsigned int open_flags, unsigned int flags)
{
struct dentry *real;
int err;
@@ -102,7 +102,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
goto bug;
/* Handle recursion */
- real = d_real(real, inode, open_flags);
+ real = d_real(real, inode, open_flags, 0);
if (!inode || inode == d_inode(real))
return real;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index aae1cdb76851..fd0721e520f4 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -147,7 +147,7 @@ struct dentry_operations {
struct vfsmount *(*d_automount)(struct path *);
int (*d_manage)(const struct path *, bool);
struct dentry *(*d_real)(struct dentry *, const struct inode *,
- unsigned int);
+ unsigned int, unsigned int);
} ____cacheline_aligned;
/*
@@ -566,7 +566,8 @@ static inline struct dentry *d_backing_dentry(struct dentry *upper)
* d_real - Return the real dentry
* @dentry: the dentry to query
* @inode: inode to select the dentry from multiple layers (can be NULL)
- * @flags: open flags to control copy-up behavior
+ * @open_flags: open flags to control copy-up behavior
+ * @flags: flags to control what is returned by this function
*
* If dentry is on a union/overlay, then return the underlying, real dentry.
* Otherwise return the dentry itself.
@@ -575,10 +576,10 @@ static inline struct dentry *d_backing_dentry(struct dentry *upper)
*/
static inline struct dentry *d_real(struct dentry *dentry,
const struct inode *inode,
- unsigned int flags)
+ unsigned int open_flags, unsigned int flags)
{
if (unlikely(dentry->d_flags & DCACHE_OP_REAL))
- return dentry->d_op->d_real(dentry, inode, flags);
+ return dentry->d_op->d_real(dentry, inode, open_flags, flags);
else
return dentry;
}
@@ -593,7 +594,7 @@ static inline struct dentry *d_real(struct dentry *dentry,
static inline struct inode *d_real_inode(const struct dentry *dentry)
{
/* This usage of d_real() results in const dentry */
- return d_backing_inode(d_real((struct dentry *) dentry, NULL, 0));
+ return d_backing_inode(d_real((struct dentry *) dentry, NULL, 0, 0));
}
struct name_snapshot {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6e1fd5d21248..ee1db83c39cb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1233,7 +1233,7 @@ static inline struct inode *file_inode(const struct file *f)
static inline struct dentry *file_dentry(const struct file *file)
{
- return d_real(file->f_path.dentry, file_inode(file), 0);
+ return d_real(file->f_path.dentry, file_inode(file), 0, 0);
}
static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
--
2.5.5
On Tue, Sep 5, 2017 at 10:46 AM, Miklos Szeredi <[email protected]> wrote:
> Add a separate flags argument (in addition to the open flags) to control
> the behavior of d_real().
>
> Signed-off-by: Miklos Szeredi <[email protected]>
> ---
...
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
>
> static struct dentry *ovl_d_real(struct dentry *dentry,
> const struct inode *inode,
> - unsigned int open_flags)
> + unsigned int open_flags, unsigned int flags)
> {
> struct dentry *real;
> int err;
> @@ -102,7 +102,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> goto bug;
>
> /* Handle recursion */
> - real = d_real(real, inode, open_flags);
> + real = d_real(real, inode, open_flags, 0);
>
Shouldn't recursion pass on flags?
The answer is probably per flag.
The 2 currently proposed flags don't end up in recursion anyway,
although it is arguable that D_REAL_ALL should end up in recursion
because according to comment it should behave the same as
d_real for regular files.
For the purpose for which D_REAL_ALL was proposed (atime update)
the recursion case doesn't really matter.
Maybe a flag D_REAL_NORECURSE and then for
update_ovl_inode_times() use D_REAL_ALL|D_REAL_NORECURSE
Alternatively, update_ovl_inode_times() could use D_REAL_UPPER
and then we explicitly say that we don't care about lower mtime/ctime
modifications.
Amir.
On Tue, Sep 5, 2017 at 10:55 AM, Amir Goldstein <[email protected]> wrote:
> On Tue, Sep 5, 2017 at 10:46 AM, Miklos Szeredi <[email protected]> wrote:
>> Add a separate flags argument (in addition to the open flags) to control
>> the behavior of d_real().
>>
>> Signed-off-by: Miklos Szeredi <[email protected]>
>> ---
> ...
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>>
>> static struct dentry *ovl_d_real(struct dentry *dentry,
>> const struct inode *inode,
>> - unsigned int open_flags)
>> + unsigned int open_flags, unsigned int flags)
>> {
>> struct dentry *real;
>> int err;
>> @@ -102,7 +102,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>> goto bug;
>>
>> /* Handle recursion */
>> - real = d_real(real, inode, open_flags);
>> + real = d_real(real, inode, open_flags, 0);
>>
>
> Shouldn't recursion pass on flags?
> The answer is probably per flag.
> The 2 currently proposed flags don't end up in recursion anyway,
> although it is arguable that D_REAL_ALL should end up in recursion
> because according to comment it should behave the same as
> d_real for regular files.
>
> For the purpose for which D_REAL_ALL was proposed (atime update)
> the recursion case doesn't really matter.
>
> Maybe a flag D_REAL_NORECURSE and then for
> update_ovl_inode_times() use D_REAL_ALL|D_REAL_NORECURSE
>
> Alternatively, update_ovl_inode_times() could use D_REAL_UPPER
> and then we explicitly say that we don't care about lower mtime/ctime
> modifications.
Well, D_REAL_ALL imples nonrecurse. Maybe that needs better
documentation (comment above in ovl_d_real()) but I don't otherwise
see a problem with the current state of affairs.
Thanks,
Miklos
On Tue, Sep 5, 2017 at 10:55 AM, Amir Goldstein <[email protected]> wrote:
[...]
> Alternatively, update_ovl_inode_times() could use D_REAL_UPPER
> and then we explicitly say that we don't care about lower mtime/ctime
> modifications.
Right, you convinced me that this is the right way to go. Updated patch below
(and pushed to overlayfs-next).
Thanks,
Miklos
---
From: Miklos Szeredi <[email protected]>
Subject: ovl: fix relatime for directories
Need to treat non-regular overlayfs files the same as regular files when
checking for an atime update.
Add a d_real() flag to make it return the upper dentry for all file types.
Reported-by: "zhangyi (F)" <[email protected]>
Signed-off-by: Miklos Szeredi <[email protected]>
---
fs/inode.c | 21 +++++++++++++++++----
fs/overlayfs/super.c | 3 +++
include/linux/dcache.h | 3 +++
3 files changed, 23 insertions(+), 4 deletions(-)
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1569,11 +1569,24 @@ EXPORT_SYMBOL(bmap);
static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
bool rcu)
{
- if (!rcu) {
- struct inode *realinode = d_real_inode(dentry);
+ struct dentry *upperdentry;
- if (unlikely(inode != realinode) &&
- (!timespec_equal(&inode->i_mtime, &realinode->i_mtime) ||
+ /*
+ * Nothing to do if in rcu or if non-overlayfs
+ */
+ if (rcu || likely(!(dentry->d_flags & DCACHE_OP_REAL)))
+ return;
+
+ upperdentry = d_real(dentry, NULL, 0, D_REAL_UPPER);
+
+ /*
+ * If file is on lower then we can't update atime, so no worries about
+ * stale mtime/ctime.
+ */
+ if (upperdentry) {
+ struct inode *realinode = d_inode(upperdentry);
+
+ if ((!timespec_equal(&inode->i_mtime, &realinode->i_mtime) ||
!timespec_equal(&inode->i_ctime, &realinode->i_ctime))) {
inode->i_mtime = realinode->i_mtime;
inode->i_ctime = realinode->i_ctime;
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -75,6 +75,9 @@ static struct dentry *ovl_d_real(struct
struct dentry *real;
int err;
+ if (flags & D_REAL_UPPER)
+ return ovl_dentry_upper(dentry);
+
if (!d_is_reg(dentry)) {
if (!inode || inode == d_inode(dentry))
return dentry;
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -562,6 +562,9 @@ static inline struct dentry *d_backing_d
return upper;
}
+/* d_real() flags */
+#define D_REAL_UPPER 0x2 /* return upper dentry or NULL if non-upper */
+
/**
* d_real - Return the real dentry
* @dentry: the dentry to query