From babf4770be0adc69e6d2de150f4040f175e24beb Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Wed, 10 Oct 2018 19:10:06 +0300 Subject: [PATCH 01/13] ovl: fix error handling in ovl_verify_set_fh() We hit a BUG on kfree of an ERR_PTR()... Reported-by: syzbot+ff03fe05c717b82502d0@syzkaller.appspotmail.com Fixes: 8b88a2e64036 ("ovl: verify upper root dir matches lower root dir") Cc: # v4.13 Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/namei.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c index 9c0ca6a7becf..efd372312ef1 100644 --- a/fs/overlayfs/namei.c +++ b/fs/overlayfs/namei.c @@ -422,8 +422,10 @@ int ovl_verify_set_fh(struct dentry *dentry, const char *name, fh = ovl_encode_real_fh(real, is_upper); err = PTR_ERR(fh); - if (IS_ERR(fh)) + if (IS_ERR(fh)) { + fh = NULL; goto fail; + } err = ovl_verify_fh(dentry, name, fh); if (set && err == -ENODATA) From 1f244dc5213960f76e7463bbb5719c331045bbc9 Mon Sep 17 00:00:00 2001 From: Miklos Szeredi Date: Fri, 26 Oct 2018 23:34:39 +0200 Subject: [PATCH 02/13] ovl: clean up error handling in ovl_get_tmpfile() If security_inode_copy_up() fails, it should not set new_creds, so no need for the cleanup (which would've Oops-ed anyway, due to old_creds being NULL). Signed-off-by: Miklos Szeredi --- fs/overlayfs/copy_up.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 1cc797a08a5b..989782a0c0c9 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -477,9 +477,8 @@ static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c) }; err = security_inode_copy_up(c->dentry, &new_creds); - temp = ERR_PTR(err); if (err < 0) - goto out; + return ERR_PTR(err); if (new_creds) old_creds = override_creds(new_creds); @@ -488,7 +487,7 @@ static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c) temp = ovl_do_tmpfile(c->workdir, c->stat.mode); else temp = ovl_create_temp(c->workdir, &cattr); -out: + if (new_creds) { revert_creds(old_creds); put_cred(new_creds); From 8f97d1e99149a7f1aa19e47a51b09764382a482e Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Thu, 11 Oct 2018 17:38:14 +0300 Subject: [PATCH 03/13] vfs: fix FIGETBSZ ioctl on an overlayfs file Some anon_bdev filesystems (e.g. overlayfs, ceph) don't have s_blocksize set. Returning zero from FIGETBSZ ioctl results in a Floating point exception from the e2fsprogs utility filefrag, which divides the size of the file with the value returned by FIGETBSZ. Fix the interface by returning -EINVAL for these filesystems. Fixes: d1d04ef8572b ("ovl: stack file ops") Cc: # v4.19 Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/ioctl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/ioctl.c b/fs/ioctl.c index 2005529af560..0400297c8d72 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -669,6 +669,9 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd, return ioctl_fiemap(filp, arg); case FIGETBSZ: + /* anon_bdev filesystems may not have a block size */ + if (!inode->i_sb->s_blocksize) + return -EINVAL; return put_user(inode->i_sb->s_blocksize, argp); case FICLONE: From 6cd078702f2f33cb6b19a682de3e9184112f1a46 Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Thu, 18 Oct 2018 09:45:49 +0300 Subject: [PATCH 04/13] ovl: fix recursive oi->lock in ovl_link() linking a non-copied-up file into a non-copied-up parent results in a nested call to mutex_lock_interruptible(&oi->lock). Fix this by copying up target parent before ovl_nlink_start(), same as done in ovl_rename(). ~/unionmount-testsuite$ ./run --ov -s ~/unionmount-testsuite$ ln /mnt/a/foo100 /mnt/a/dir100/ WARNING: possible recursive locking detected -------------------------------------------- ln/1545 is trying to acquire lock: 00000000bcce7c4c (&ovl_i_lock_key[depth]){+.+.}, at: ovl_copy_up_start+0x28/0x7d but task is already holding lock: 0000000026d73d5b (&ovl_i_lock_key[depth]){+.+.}, at: ovl_nlink_start+0x3c/0xc1 [SzM: this seems to be a false positive, but doing the copy-up first is harmless and removes the lockdep splat] Reported-by: syzbot+3ef5c0d1a5cb0b21e6be@syzkaller.appspotmail.com Fixes: 5f8415d6b87e ("ovl: persistent overlay inode nlink for...") Cc: # v4.13 Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/dir.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index 276914ae3c60..e1a55ecb7aba 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -663,6 +663,10 @@ static int ovl_link(struct dentry *old, struct inode *newdir, if (err) goto out_drop_write; + err = ovl_copy_up(new->d_parent); + if (err) + goto out_drop_write; + if (ovl_is_metacopy_dentry(old)) { err = ovl_set_redirect(old, false); if (err) From 007ea44892e6fa963a0876a979e34890325c64eb Mon Sep 17 00:00:00 2001 From: Miklos Szeredi Date: Fri, 26 Oct 2018 23:34:39 +0200 Subject: [PATCH 05/13] ovl: relax permission checking on underlying layers Make permission checking more consistent: - special files don't need any access check on underling fs - exec permission check doesn't need to be performed on underlying fs Reported-by: "J. Bruce Fields" Signed-off-by: Miklos Szeredi --- fs/overlayfs/inode.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c index 3b7ed5d2279c..6bcc9dedc342 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -286,13 +286,22 @@ int ovl_permission(struct inode *inode, int mask) if (err) return err; - old_cred = ovl_override_creds(inode->i_sb); - if (!upperinode && - !special_file(realinode->i_mode) && mask & MAY_WRITE) { + /* No need to do any access on underlying for special files */ + if (special_file(realinode->i_mode)) + return 0; + + /* No need to access underlying for execute */ + mask &= ~MAY_EXEC; + if ((mask & (MAY_READ | MAY_WRITE)) == 0) + return 0; + + /* Lower files get copied up, so turn write access into read */ + if (!upperinode && mask & MAY_WRITE) { mask &= ~(MAY_WRITE | MAY_APPEND); - /* Make sure mounter can read file for copy up later */ mask |= MAY_READ; } + + old_cred = ovl_override_creds(inode->i_sb); err = inode_permission(realinode, mask); revert_creds(old_cred); From b10cdcdc2012b2c199077a0a648e8a7067e573bf Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Mon, 8 Oct 2018 07:25:23 +0300 Subject: [PATCH 06/13] ovl: untangle copy up call chain In an attempt to dedup ~100 LOC, we ended up creating a tangled call chain, whose branches merge and diverge in several points according to the immutable c->tmpfile copy up mode. This call chain was hard to analyse for locking correctness because the locking requirements for the c->tmpfile flow were very different from the locking requirements for the !c->tmpfile flow (i.e. directory vs. regulare file copy up). Split the copy up helpers of the c->tmpfile flow from those of the !c->tmpfile (i.e. workdir) flow and remove the c->tmpfile mode from copy up context. Suggested-by: Al Viro Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/copy_up.c | 255 ++++++++++++++++++++++++++--------------- 1 file changed, 164 insertions(+), 91 deletions(-) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 989782a0c0c9..618cffc14f91 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -395,7 +395,6 @@ struct ovl_copy_up_ctx { struct dentry *destdir; struct qstr destname; struct dentry *workdir; - bool tmpfile; bool origin; bool indexed; bool metacopy; @@ -440,62 +439,6 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c) return err; } -static int ovl_install_temp(struct ovl_copy_up_ctx *c, struct dentry *temp, - struct dentry **newdentry) -{ - int err; - struct dentry *upper; - struct inode *udir = d_inode(c->destdir); - - upper = lookup_one_len(c->destname.name, c->destdir, c->destname.len); - if (IS_ERR(upper)) - return PTR_ERR(upper); - - if (c->tmpfile) - err = ovl_do_link(temp, udir, upper); - else - err = ovl_do_rename(d_inode(c->workdir), temp, udir, upper, 0); - - if (!err) - *newdentry = dget(c->tmpfile ? upper : temp); - dput(upper); - - return err; -} - -static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c) -{ - int err; - struct dentry *temp; - const struct cred *old_creds = NULL; - struct cred *new_creds = NULL; - struct ovl_cattr cattr = { - /* Can't properly set mode on creation because of the umask */ - .mode = c->stat.mode & S_IFMT, - .rdev = c->stat.rdev, - .link = c->link - }; - - err = security_inode_copy_up(c->dentry, &new_creds); - if (err < 0) - return ERR_PTR(err); - - if (new_creds) - old_creds = override_creds(new_creds); - - if (c->tmpfile) - temp = ovl_do_tmpfile(c->workdir, c->stat.mode); - else - temp = ovl_create_temp(c->workdir, &cattr); - - if (new_creds) { - revert_creds(old_creds); - put_cred(new_creds); - } - - return temp; -} - static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) { int err; @@ -547,14 +490,170 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) return err; } -static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c) +static struct dentry *ovl_get_workdir_temp(struct ovl_copy_up_ctx *c) +{ + int err; + struct dentry *temp; + const struct cred *old_creds = NULL; + struct cred *new_creds = NULL; + struct ovl_cattr cattr = { + /* Can't properly set mode on creation because of the umask */ + .mode = c->stat.mode & S_IFMT, + .rdev = c->stat.rdev, + .link = c->link + }; + + err = security_inode_copy_up(c->dentry, &new_creds); + if (err < 0) + return ERR_PTR(err); + + if (new_creds) + old_creds = override_creds(new_creds); + + temp = ovl_create_temp(c->workdir, &cattr); + + if (new_creds) { + revert_creds(old_creds); + put_cred(new_creds); + } + + return temp; +} + +/* + * Move temp file from workdir into place on upper dir. + * Used when copying up directories and when upper fs doesn't support O_TMPFILE. + * + * Caller must hold ovl_lock_rename_workdir(). + */ +static int ovl_rename_temp(struct ovl_copy_up_ctx *c, struct dentry *temp, + struct dentry **newdentry) +{ + int err; + struct dentry *upper; + struct inode *udir = d_inode(c->destdir); + + upper = lookup_one_len(c->destname.name, c->destdir, c->destname.len); + if (IS_ERR(upper)) + return PTR_ERR(upper); + + err = ovl_do_rename(d_inode(c->workdir), temp, udir, upper, 0); + if (!err) + *newdentry = dget(temp); + dput(upper); + + return err; +} + +/* + * Copyup using workdir to prepare temp file. Used when copying up directories, + * special files or when upper fs doesn't support O_TMPFILE. + */ +static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) { - struct inode *udir = c->destdir->d_inode; struct inode *inode; struct dentry *newdentry = NULL; struct dentry *temp; int err; + err = ovl_lock_rename_workdir(c->workdir, c->destdir); + if (err) + return err; + + temp = ovl_get_workdir_temp(c); + err = PTR_ERR(temp); + if (IS_ERR(temp)) + goto unlock; + + err = ovl_copy_up_inode(c, temp); + if (err) + goto cleanup; + + if (S_ISDIR(c->stat.mode) && c->indexed) { + err = ovl_create_index(c->dentry, c->lowerpath.dentry, temp); + if (err) + goto cleanup; + } + + err = ovl_rename_temp(c, temp, &newdentry); + if (err) + goto cleanup; + + if (!c->metacopy) + ovl_set_upperdata(d_inode(c->dentry)); + inode = d_inode(c->dentry); + ovl_inode_update(inode, newdentry); + if (S_ISDIR(inode->i_mode)) + ovl_set_flag(OVL_WHITEOUTS, inode); +out_dput: + dput(temp); +unlock: + unlock_rename(c->workdir, c->destdir); + + return err; + +cleanup: + ovl_cleanup(d_inode(c->workdir), temp); + goto out_dput; +} + +static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c) +{ + int err; + struct dentry *temp; + const struct cred *old_creds = NULL; + struct cred *new_creds = NULL; + + err = security_inode_copy_up(c->dentry, &new_creds); + if (err < 0) + return ERR_PTR(err); + + if (new_creds) + old_creds = override_creds(new_creds); + + temp = ovl_do_tmpfile(c->workdir, c->stat.mode); + + if (new_creds) { + revert_creds(old_creds); + put_cred(new_creds); + } + + return temp; +} + +/* Link O_TMPFILE into place on upper dir */ +static int ovl_link_tmpfile(struct ovl_copy_up_ctx *c, struct dentry *temp, + struct dentry **newdentry) +{ + int err; + struct dentry *upper; + struct inode *udir = d_inode(c->destdir); + + inode_lock_nested(udir, I_MUTEX_PARENT); + + upper = lookup_one_len(c->destname.name, c->destdir, c->destname.len); + err = PTR_ERR(upper); + if (IS_ERR(upper)) + goto unlock; + + err = ovl_do_link(temp, udir, upper); + if (!err) + *newdentry = dget(upper); + dput(upper); + +unlock: + inode_unlock(udir); + + return err; +} + +/* Copyup using O_TMPFILE which does not require cross dir locking */ +static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c) +{ + struct dentry *newdentry = NULL; + struct dentry *temp; + int err; + temp = ovl_get_tmpfile(c); if (IS_ERR(temp)) return PTR_ERR(temp); @@ -563,35 +662,17 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c) if (err) goto out; - if (S_ISDIR(c->stat.mode) && c->indexed) { - err = ovl_create_index(c->dentry, c->lowerpath.dentry, temp); - if (err) - goto out; - } - - if (c->tmpfile) { - inode_lock_nested(udir, I_MUTEX_PARENT); - err = ovl_install_temp(c, temp, &newdentry); - inode_unlock(udir); - } else { - err = ovl_install_temp(c, temp, &newdentry); - } + err = ovl_link_tmpfile(c, temp, &newdentry); if (err) goto out; if (!c->metacopy) ovl_set_upperdata(d_inode(c->dentry)); - inode = d_inode(c->dentry); - ovl_inode_update(inode, newdentry); - if (S_ISDIR(inode->i_mode)) - ovl_set_flag(OVL_WHITEOUTS, inode); + ovl_inode_update(d_inode(c->dentry), newdentry); out: - if (err && !c->tmpfile) - ovl_cleanup(d_inode(c->workdir), temp); dput(temp); return err; - } /* @@ -645,18 +726,10 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c) } /* Should we copyup with O_TMPFILE or with workdir? */ - if (S_ISREG(c->stat.mode) && ofs->tmpfile) { - c->tmpfile = true; - err = ovl_copy_up_locked(c); - } else { - err = ovl_lock_rename_workdir(c->workdir, c->destdir); - if (!err) { - err = ovl_copy_up_locked(c); - unlock_rename(c->workdir, c->destdir); - } - } - - + if (S_ISREG(c->stat.mode) && ofs->tmpfile) + err = ovl_copy_up_tmpfile(c); + else + err = ovl_copy_up_workdir(c); if (err) goto out; From 6b52243f633eb5521e427bf78c85ccf646795f46 Mon Sep 17 00:00:00 2001 From: Miklos Szeredi Date: Fri, 26 Oct 2018 23:34:39 +0200 Subject: [PATCH 07/13] ovl: fold copy-up helpers into callers Now that the workdir and tmpfile copy up modes have been untagled, the functions become simple enough that the helpers can be folded into the callers. Add new helpers where there is any duplication remaining: preparing creds for creating the object. Signed-off-by: Miklos Szeredi --- fs/overlayfs/copy_up.c | 181 ++++++++++++++++------------------------- 1 file changed, 70 insertions(+), 111 deletions(-) diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c index 618cffc14f91..d6a3346e2672 100644 --- a/fs/overlayfs/copy_up.c +++ b/fs/overlayfs/copy_up.c @@ -490,59 +490,32 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp) return err; } -static struct dentry *ovl_get_workdir_temp(struct ovl_copy_up_ctx *c) +struct ovl_cu_creds { + const struct cred *old; + struct cred *new; +}; + +static int ovl_prep_cu_creds(struct dentry *dentry, struct ovl_cu_creds *cc) { int err; - struct dentry *temp; - const struct cred *old_creds = NULL; - struct cred *new_creds = NULL; - struct ovl_cattr cattr = { - /* Can't properly set mode on creation because of the umask */ - .mode = c->stat.mode & S_IFMT, - .rdev = c->stat.rdev, - .link = c->link - }; - err = security_inode_copy_up(c->dentry, &new_creds); + cc->old = cc->new = NULL; + err = security_inode_copy_up(dentry, &cc->new); if (err < 0) - return ERR_PTR(err); + return err; - if (new_creds) - old_creds = override_creds(new_creds); + if (cc->new) + cc->old = override_creds(cc->new); - temp = ovl_create_temp(c->workdir, &cattr); - - if (new_creds) { - revert_creds(old_creds); - put_cred(new_creds); - } - - return temp; + return 0; } -/* - * Move temp file from workdir into place on upper dir. - * Used when copying up directories and when upper fs doesn't support O_TMPFILE. - * - * Caller must hold ovl_lock_rename_workdir(). - */ -static int ovl_rename_temp(struct ovl_copy_up_ctx *c, struct dentry *temp, - struct dentry **newdentry) +static void ovl_revert_cu_creds(struct ovl_cu_creds *cc) { - int err; - struct dentry *upper; - struct inode *udir = d_inode(c->destdir); - - upper = lookup_one_len(c->destname.name, c->destdir, c->destname.len); - if (IS_ERR(upper)) - return PTR_ERR(upper); - - err = ovl_do_rename(d_inode(c->workdir), temp, udir, upper, 0); - if (!err) - *newdentry = dget(temp); - dput(upper); - - return err; + if (cc->new) { + revert_creds(cc->old); + put_cred(cc->new); + } } /* @@ -552,15 +525,28 @@ static int ovl_rename_temp(struct ovl_copy_up_ctx *c, struct dentry *temp, static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) { struct inode *inode; - struct dentry *newdentry = NULL; - struct dentry *temp; + struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir); + struct dentry *temp, *upper; + struct ovl_cu_creds cc; int err; + struct ovl_cattr cattr = { + /* Can't properly set mode on creation because of the umask */ + .mode = c->stat.mode & S_IFMT, + .rdev = c->stat.rdev, + .link = c->link + }; err = ovl_lock_rename_workdir(c->workdir, c->destdir); if (err) return err; - temp = ovl_get_workdir_temp(c); + err = ovl_prep_cu_creds(c->dentry, &cc); + if (err) + goto unlock; + + temp = ovl_create_temp(c->workdir, &cattr); + ovl_revert_cu_creds(&cc); + err = PTR_ERR(temp); if (IS_ERR(temp)) goto unlock; @@ -575,102 +561,75 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c) goto cleanup; } - err = ovl_rename_temp(c, temp, &newdentry); + upper = lookup_one_len(c->destname.name, c->destdir, c->destname.len); + err = PTR_ERR(upper); + if (IS_ERR(upper)) + goto cleanup; + + err = ovl_do_rename(wdir, temp, udir, upper, 0); + dput(upper); if (err) goto cleanup; if (!c->metacopy) ovl_set_upperdata(d_inode(c->dentry)); inode = d_inode(c->dentry); - ovl_inode_update(inode, newdentry); + ovl_inode_update(inode, temp); if (S_ISDIR(inode->i_mode)) ovl_set_flag(OVL_WHITEOUTS, inode); -out_dput: - dput(temp); unlock: unlock_rename(c->workdir, c->destdir); return err; cleanup: - ovl_cleanup(d_inode(c->workdir), temp); - goto out_dput; -} - -static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c) -{ - int err; - struct dentry *temp; - const struct cred *old_creds = NULL; - struct cred *new_creds = NULL; - - err = security_inode_copy_up(c->dentry, &new_creds); - if (err < 0) - return ERR_PTR(err); - - if (new_creds) - old_creds = override_creds(new_creds); - - temp = ovl_do_tmpfile(c->workdir, c->stat.mode); - - if (new_creds) { - revert_creds(old_creds); - put_cred(new_creds); - } - - return temp; -} - -/* Link O_TMPFILE into place on upper dir */ -static int ovl_link_tmpfile(struct ovl_copy_up_ctx *c, struct dentry *temp, - struct dentry **newdentry) -{ - int err; - struct dentry *upper; - struct inode *udir = d_inode(c->destdir); - - inode_lock_nested(udir, I_MUTEX_PARENT); - - upper = lookup_one_len(c->destname.name, c->destdir, c->destname.len); - err = PTR_ERR(upper); - if (IS_ERR(upper)) - goto unlock; - - err = ovl_do_link(temp, udir, upper); - if (!err) - *newdentry = dget(upper); - dput(upper); - -unlock: - inode_unlock(udir); - - return err; + ovl_cleanup(wdir, temp); + dput(temp); + goto unlock; } /* Copyup using O_TMPFILE which does not require cross dir locking */ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c) { - struct dentry *newdentry = NULL; - struct dentry *temp; + struct inode *udir = d_inode(c->destdir); + struct dentry *temp, *upper; + struct ovl_cu_creds cc; int err; - temp = ovl_get_tmpfile(c); + err = ovl_prep_cu_creds(c->dentry, &cc); + if (err) + return err; + + temp = ovl_do_tmpfile(c->workdir, c->stat.mode); + ovl_revert_cu_creds(&cc); + if (IS_ERR(temp)) return PTR_ERR(temp); err = ovl_copy_up_inode(c, temp); if (err) - goto out; + goto out_dput; + + inode_lock_nested(udir, I_MUTEX_PARENT); + + upper = lookup_one_len(c->destname.name, c->destdir, c->destname.len); + err = PTR_ERR(upper); + if (!IS_ERR(upper)) { + err = ovl_do_link(temp, udir, upper); + dput(upper); + } + inode_unlock(udir); - err = ovl_link_tmpfile(c, temp, &newdentry); if (err) - goto out; + goto out_dput; if (!c->metacopy) ovl_set_upperdata(d_inode(c->dentry)); - ovl_inode_update(d_inode(c->dentry), newdentry); + ovl_inode_update(d_inode(c->dentry), temp); -out: + return 0; + +out_dput: dput(temp); return err; } From 9df085f3c9a2d4658a9fe323d70c200aa00ede93 Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Mon, 3 Sep 2018 09:12:09 +0300 Subject: [PATCH 08/13] ovl: relax requirement for non null uuid of lower fs We use uuid to associate an overlay lower file handle with a lower layer, so we can accept lower fs with null uuid as long as all lower layers with null uuid are on the same fs. This change allows enabling index and nfs_export features for the setup of single lower fs of type squashfs - squashfs supports file handles, but has a null uuid. This change also allows enabling index and nfs_export features for nested overlayfs, where the lower overlay has nfs_export enabled. Enabling the index feature with single lower squashfs fixes the unionmount-testsuite test: ./run --ov --squashfs --verify As a by-product, if, like the lower squashfs, upper fs also uses the generic export_encode_fh() implementation to export 32bit inode file handles (e.g. ext4), then the xino_auto config/module/mount option will enable unique overlay inode numbers. Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/super.c | 34 +++++++++++++++++++++++++++++++--- fs/overlayfs/util.c | 3 +-- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 30adc9d408a0..22ffb23ea44d 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -1175,10 +1175,30 @@ out: return err; } -/* Get a unique fsid for the layer */ -static int ovl_get_fsid(struct ovl_fs *ofs, struct super_block *sb) +static bool ovl_lower_uuid_ok(struct ovl_fs *ofs, const uuid_t *uuid) { unsigned int i; + + if (!ofs->config.nfs_export && !(ofs->config.index && ofs->upper_mnt)) + return true; + + for (i = 0; i < ofs->numlowerfs; i++) { + /* + * We use uuid to associate an overlay lower file handle with a + * lower layer, so we can accept lower fs with null uuid as long + * as all lower layers with null uuid are on the same fs. + */ + if (uuid_equal(&ofs->lower_fs[i].sb->s_uuid, uuid)) + return false; + } + return true; +} + +/* Get a unique fsid for the layer */ +static int ovl_get_fsid(struct ovl_fs *ofs, const struct path *path) +{ + struct super_block *sb = path->mnt->mnt_sb; + unsigned int i; dev_t dev; int err; @@ -1191,6 +1211,14 @@ static int ovl_get_fsid(struct ovl_fs *ofs, struct super_block *sb) return i + 1; } + if (!ovl_lower_uuid_ok(ofs, &sb->s_uuid)) { + ofs->config.index = false; + ofs->config.nfs_export = false; + pr_warn("overlayfs: %s uuid detected in lower fs '%pd2', falling back to index=off,nfs_export=off.\n", + uuid_is_null(&sb->s_uuid) ? "null" : "conflicting", + path->dentry); + } + err = get_anon_bdev(&dev); if (err) { pr_err("overlayfs: failed to get anonymous bdev for lowerpath\n"); @@ -1225,7 +1253,7 @@ static int ovl_get_lower_layers(struct ovl_fs *ofs, struct path *stack, struct vfsmount *mnt; int fsid; - err = fsid = ovl_get_fsid(ofs, stack[i].mnt->mnt_sb); + err = fsid = ovl_get_fsid(ofs, &stack[i]); if (err < 0) goto out; diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index ace4fe4c39a9..8cd37baf5d0a 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -65,8 +65,7 @@ struct super_block *ovl_same_sb(struct super_block *sb) */ int ovl_can_decode_fh(struct super_block *sb) { - if (!sb->s_export_op || !sb->s_export_op->fh_to_dentry || - uuid_is_null(&sb->s_uuid)) + if (!sb->s_export_op || !sb->s_export_op->fh_to_dentry) return 0; return sb->s_export_op->encode_fh ? -1 : FILEID_INO32_GEN; From 0e32992f7faccd50d1b00032de83e87a35fed247 Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Thu, 18 Oct 2018 18:37:13 +0300 Subject: [PATCH 09/13] ovl: remove the 'locked' argument of ovl_nlink_{start,end} It just makes the interface strange without adding any significant value. The only case where locked is false and return value is 0 is in ovl_rename() when new is negative, so handle that case explicitly in ovl_rename(). Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/dir.c | 21 +++++++++++---------- fs/overlayfs/overlayfs.h | 4 ++-- fs/overlayfs/util.c | 28 ++++++++++++---------------- 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index e1a55ecb7aba..e03f39550ccf 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -652,7 +652,6 @@ static int ovl_link(struct dentry *old, struct inode *newdir, struct dentry *new) { int err; - bool locked = false; struct inode *inode; err = ovl_want_write(old); @@ -673,7 +672,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, goto out_drop_write; } - err = ovl_nlink_start(old, &locked); + err = ovl_nlink_start(old); if (err) goto out_drop_write; @@ -686,7 +685,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, if (err) iput(inode); - ovl_nlink_end(old, locked); + ovl_nlink_end(old); out_drop_write: ovl_drop_write(old); out: @@ -811,7 +810,6 @@ static bool ovl_pure_upper(struct dentry *dentry) static int ovl_do_remove(struct dentry *dentry, bool is_dir) { int err; - bool locked = false; const struct cred *old_cred; struct dentry *upperdentry; bool lower_positive = ovl_lower_positive(dentry); @@ -832,7 +830,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir) if (err) goto out_drop_write; - err = ovl_nlink_start(dentry, &locked); + err = ovl_nlink_start(dentry); if (err) goto out_drop_write; @@ -848,7 +846,7 @@ static int ovl_do_remove(struct dentry *dentry, bool is_dir) else drop_nlink(dentry->d_inode); } - ovl_nlink_end(dentry, locked); + ovl_nlink_end(dentry); /* * Copy ctime @@ -1012,7 +1010,6 @@ static int ovl_rename(struct inode *olddir, struct dentry *old, unsigned int flags) { int err; - bool locked = false; struct dentry *old_upperdir; struct dentry *new_upperdir; struct dentry *olddentry; @@ -1021,6 +1018,7 @@ static int ovl_rename(struct inode *olddir, struct dentry *old, bool old_opaque; bool new_opaque; bool cleanup_whiteout = false; + bool update_nlink = false; bool overwrite = !(flags & RENAME_EXCHANGE); bool is_dir = d_is_dir(old); bool new_is_dir = d_is_dir(new); @@ -1078,10 +1076,12 @@ static int ovl_rename(struct inode *olddir, struct dentry *old, err = ovl_copy_up(new); if (err) goto out_drop_write; - } else { - err = ovl_nlink_start(new, &locked); + } else if (d_inode(new)) { + err = ovl_nlink_start(new); if (err) goto out_drop_write; + + update_nlink = true; } old_cred = ovl_override_creds(old->d_sb); @@ -1210,7 +1210,8 @@ out_unlock: unlock_rename(new_upperdir, old_upperdir); out_revert_creds: revert_creds(old_cred); - ovl_nlink_end(new, locked); + if (update_nlink) + ovl_nlink_end(new); out_drop_write: ovl_drop_write(old); out: diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index a3c0d9584312..b48ef22ef96b 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -271,8 +271,8 @@ bool ovl_test_flag(unsigned long flag, struct inode *inode); bool ovl_inuse_trylock(struct dentry *dentry); void ovl_inuse_unlock(struct dentry *dentry); bool ovl_need_index(struct dentry *dentry); -int ovl_nlink_start(struct dentry *dentry, bool *locked); -void ovl_nlink_end(struct dentry *dentry, bool locked); +int ovl_nlink_start(struct dentry *dentry); +void ovl_nlink_end(struct dentry *dentry); int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir); int ovl_check_metacopy_xattr(struct dentry *dentry); bool ovl_is_metacopy_dentry(struct dentry *dentry); diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index 8cd37baf5d0a..d3a786006729 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -738,14 +738,14 @@ fail: * Operations that change overlay inode and upper inode nlink need to be * synchronized with copy up for persistent nlink accounting. */ -int ovl_nlink_start(struct dentry *dentry, bool *locked) +int ovl_nlink_start(struct dentry *dentry) { struct ovl_inode *oi = OVL_I(d_inode(dentry)); const struct cred *old_cred; int err; - if (!d_inode(dentry)) - return 0; + if (WARN_ON(!d_inode(dentry))) + return -ENOENT; /* * With inodes index is enabled, we store the union overlay nlink @@ -787,26 +787,22 @@ int ovl_nlink_start(struct dentry *dentry, bool *locked) out: if (err) mutex_unlock(&oi->lock); - else - *locked = true; return err; } -void ovl_nlink_end(struct dentry *dentry, bool locked) +void ovl_nlink_end(struct dentry *dentry) { - if (locked) { - if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) && - d_inode(dentry)->i_nlink == 0) { - const struct cred *old_cred; + if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) && + d_inode(dentry)->i_nlink == 0) { + const struct cred *old_cred; - old_cred = ovl_override_creds(dentry->d_sb); - ovl_cleanup_index(dentry); - revert_creds(old_cred); - } - - mutex_unlock(&OVL_I(d_inode(dentry))->lock); + old_cred = ovl_override_creds(dentry->d_sb); + ovl_cleanup_index(dentry); + revert_creds(old_cred); } + + mutex_unlock(&OVL_I(d_inode(dentry))->lock); } int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir) From 1e92e3072c1456abedf51c9dedd245d2ba0daa4f Mon Sep 17 00:00:00 2001 From: Amir Goldstein Date: Thu, 18 Oct 2018 18:37:14 +0300 Subject: [PATCH 10/13] ovl: abstract ovl_inode lock with a helper The abstraction improves code readabilty (to some). Signed-off-by: Amir Goldstein Signed-off-by: Miklos Szeredi --- fs/overlayfs/overlayfs.h | 10 ++++++++++ fs/overlayfs/util.c | 25 +++++++++++++------------ 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index b48ef22ef96b..5e45cb3630a0 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -290,6 +290,16 @@ static inline unsigned int ovl_xino_bits(struct super_block *sb) return ofs->xino_bits; } +static inline int ovl_inode_lock(struct inode *inode) +{ + return mutex_lock_interruptible(&OVL_I(inode)->lock); +} + +static inline void ovl_inode_unlock(struct inode *inode) +{ + mutex_unlock(&OVL_I(inode)->lock); +} + /* namei.c */ int ovl_check_fh_len(struct ovl_fh *fh, int fh_len); diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c index d3a786006729..7c01327b1852 100644 --- a/fs/overlayfs/util.c +++ b/fs/overlayfs/util.c @@ -521,13 +521,13 @@ bool ovl_already_copied_up(struct dentry *dentry, int flags) int ovl_copy_up_start(struct dentry *dentry, int flags) { - struct ovl_inode *oi = OVL_I(d_inode(dentry)); + struct inode *inode = d_inode(dentry); int err; - err = mutex_lock_interruptible(&oi->lock); + err = ovl_inode_lock(inode); if (!err && ovl_already_copied_up_locked(dentry, flags)) { err = 1; /* Already copied up */ - mutex_unlock(&oi->lock); + ovl_inode_unlock(inode); } return err; @@ -535,7 +535,7 @@ int ovl_copy_up_start(struct dentry *dentry, int flags) void ovl_copy_up_end(struct dentry *dentry) { - mutex_unlock(&OVL_I(d_inode(dentry))->lock); + ovl_inode_unlock(d_inode(dentry)); } bool ovl_check_origin_xattr(struct dentry *dentry) @@ -740,11 +740,11 @@ fail: */ int ovl_nlink_start(struct dentry *dentry) { - struct ovl_inode *oi = OVL_I(d_inode(dentry)); + struct inode *inode = d_inode(dentry); const struct cred *old_cred; int err; - if (WARN_ON(!d_inode(dentry))) + if (WARN_ON(!inode)) return -ENOENT; /* @@ -767,11 +767,11 @@ int ovl_nlink_start(struct dentry *dentry) return err; } - err = mutex_lock_interruptible(&oi->lock); + err = ovl_inode_lock(inode); if (err) return err; - if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, d_inode(dentry))) + if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode)) goto out; old_cred = ovl_override_creds(dentry->d_sb); @@ -786,15 +786,16 @@ int ovl_nlink_start(struct dentry *dentry) out: if (err) - mutex_unlock(&oi->lock); + ovl_inode_unlock(inode); return err; } void ovl_nlink_end(struct dentry *dentry) { - if (ovl_test_flag(OVL_INDEX, d_inode(dentry)) && - d_inode(dentry)->i_nlink == 0) { + struct inode *inode = d_inode(dentry); + + if (ovl_test_flag(OVL_INDEX, inode) && inode->i_nlink == 0) { const struct cred *old_cred; old_cred = ovl_override_creds(dentry->d_sb); @@ -802,7 +803,7 @@ void ovl_nlink_end(struct dentry *dentry) revert_creds(old_cred); } - mutex_unlock(&OVL_I(d_inode(dentry))->lock); + ovl_inode_unlock(inode); } int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *upperdir) From 14fa085640a7eb55431eec8a0273bbf0c463ce46 Mon Sep 17 00:00:00 2001 From: Chengguang Xu Date: Fri, 22 Jun 2018 09:49:36 +0800 Subject: [PATCH 11/13] ovl: using posix_acl_xattr_size() to get size instead of posix_acl_to_xattr() There is no functional change but it seems better to get size by calling posix_acl_xattr_size() instead of calling posix_acl_to_xattr() with NULL buffer argument. Additionally, remove unnecessary assignments. Signed-off-by: Chengguang Xu Signed-off-by: Miklos Szeredi --- fs/overlayfs/dir.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index e03f39550ccf..ce1857fb5434 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -414,13 +414,12 @@ static int ovl_set_upper_acl(struct dentry *upperdentry, const char *name, if (!IS_ENABLED(CONFIG_FS_POSIX_ACL) || !acl) return 0; - size = posix_acl_to_xattr(NULL, acl, NULL, 0); + size = posix_acl_xattr_size(acl->a_count); buffer = kmalloc(size, GFP_KERNEL); if (!buffer) return -ENOMEM; - size = posix_acl_to_xattr(&init_user_ns, acl, buffer, size); - err = size; + err = posix_acl_to_xattr(&init_user_ns, acl, buffer, size); if (err < 0) goto out_free; From 5e1275808630ea3b2c97c776f40e475017535f72 Mon Sep 17 00:00:00 2001 From: Miklos Szeredi Date: Wed, 31 Oct 2018 12:15:23 +0100 Subject: [PATCH 12/13] ovl: check whiteout in ovl_create_over_whiteout() Kaixuxia repors that it's possible to crash overlayfs by removing the whiteout on the upper layer before creating a directory over it. This is a reproducer: mkdir lower upper work merge touch lower/file mount -t overlay overlay -olowerdir=lower,upperdir=upper,workdir=work merge rm merge/file ls -al merge/file rm upper/file ls -al merge/ mkdir merge/file Before commencing with a vfs_rename(..., RENAME_EXCHANGE) verify that the lookup of "upper" is positive and is a whiteout, and return ESTALE otherwise. Reported by: kaixuxia Signed-off-by: Miklos Szeredi Fixes: e9be9d5e76e3 ("overlay filesystem") Cc: # v3.18 --- fs/overlayfs/dir.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index ce1857fb5434..c6289147c787 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -462,6 +462,10 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, if (IS_ERR(upper)) goto out_unlock; + err = -ESTALE; + if (d_is_negative(upper) || !IS_WHITEOUT(d_inode(upper))) + goto out_dput; + newdentry = ovl_create_temp(workdir, cattr); err = PTR_ERR(newdentry); if (IS_ERR(newdentry)) From d47748e5ae5af6572e520cc9767bbe70c22ea498 Mon Sep 17 00:00:00 2001 From: Miklos Szeredi Date: Thu, 1 Nov 2018 21:31:39 +0100 Subject: [PATCH 13/13] ovl: automatically enable redirect_dir on metacopy=on Current behavior is to automatically disable metacopy if redirect_dir is not enabled and proceed with the mount. If "metacopy=on" mount option was given, then this behavior can confuse the user: no mount failure, yet metacopy is disabled. This patch makes metacopy=on imply redirect_dir=on. The converse is also true: turning off full redirect with redirect_dir= {off|follow|nofollow} will disable metacopy. If both metacopy=on and redirect_dir={off|follow|nofollow} is specified, then mount will fail, since there's no way to correctly resolve the conflict. Reported-by: Daniel Walsh Fixes: d5791044d2e5 ("ovl: Provide a mount option metacopy=on/off...") Cc: # v4.19 Signed-off-by: Miklos Szeredi --- Documentation/filesystems/overlayfs.txt | 6 +++++ fs/overlayfs/super.c | 36 ++++++++++++++++++++----- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt index 51c136c821bf..eef7d9d259e8 100644 --- a/Documentation/filesystems/overlayfs.txt +++ b/Documentation/filesystems/overlayfs.txt @@ -286,6 +286,12 @@ pointed by REDIRECT. This should not be possible on local system as setting "trusted." xattrs will require CAP_SYS_ADMIN. But it should be possible for untrusted layers like from a pen drive. +Note: redirect_dir={off|nofollow|follow(*)} conflicts with metacopy=on, and +results in an error. + +(*) redirect_dir=follow only conflicts with metacopy=on if upperdir=... is +given. + Sharing and copying layers -------------------------- diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 22ffb23ea44d..0116735cc321 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -472,6 +472,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) { char *p; int err; + bool metacopy_opt = false, redirect_opt = false; config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL); if (!config->redirect_mode) @@ -516,6 +517,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) config->redirect_mode = match_strdup(&args[0]); if (!config->redirect_mode) return -ENOMEM; + redirect_opt = true; break; case OPT_INDEX_ON: @@ -548,6 +550,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) case OPT_METACOPY_ON: config->metacopy = true; + metacopy_opt = true; break; case OPT_METACOPY_OFF: @@ -572,13 +575,32 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) if (err) return err; - /* metacopy feature with upper requires redirect_dir=on */ - if (config->upperdir && config->metacopy && !config->redirect_dir) { - pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=on\", falling back to metacopy=off.\n"); - config->metacopy = false; - } else if (config->metacopy && !config->redirect_follow) { - pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n"); - config->metacopy = false; + /* + * This is to make the logic below simpler. It doesn't make any other + * difference, since config->redirect_dir is only used for upper. + */ + if (!config->upperdir && config->redirect_follow) + config->redirect_dir = true; + + /* Resolve metacopy -> redirect_dir dependency */ + if (config->metacopy && !config->redirect_dir) { + if (metacopy_opt && redirect_opt) { + pr_err("overlayfs: conflicting options: metacopy=on,redirect_dir=%s\n", + config->redirect_mode); + return -EINVAL; + } + if (redirect_opt) { + /* + * There was an explicit redirect_dir=... that resulted + * in this conflict. + */ + pr_info("overlayfs: disabling metacopy due to redirect_dir=%s\n", + config->redirect_mode); + config->metacopy = false; + } else { + /* Automatically enable redirect otherwise. */ + config->redirect_follow = config->redirect_dir = true; + } } return 0;