| Message ID | 1495533033-22367-3-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 23, 2017 at 11:50 AM, Amir Goldstein <amir73il@gmail.com> wrote: > Bad things can happen if several concurrent overlay mounts try to > use the same upperdir path and/or workdir path. > > Try to get the 'inuse' advisory lock on upper and work dir. > Fail mount if another overlay mount instance or another user > holds the 'inuse' lock. > > Note that this provides no protection for concurrent overlay > mount that use overlapping (i.e. descendant) upper dirs or > work dirs. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/overlayfs/super.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 67 insertions(+), 3 deletions(-) > > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index 4882ffb..ac9212d 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -165,12 +165,42 @@ static const struct dentry_operations ovl_reval_dentry_operations = { > .d_weak_revalidate = ovl_dentry_weak_revalidate, > }; > > +/* Get exclusive ownership on upper/work dir among overlay mounts */ > +static bool ovl_dir_lock(struct dentry *dentry) > +{ > + struct inode *inode; > + > + if (!dentry) > + return false; > + > + inode = d_inode(dentry); > + if (!inode || inode_inuse(inode)) > + return false; > + > + return inode_inuse_trylock(inode); > +} > + > +static void ovl_dir_unlock(struct dentry *dentry) > +{ > + struct inode *inode; > + > + if (!dentry) > + return; > + > + inode = d_inode(dentry); > + if (inode && inode_inuse(inode)) > + inode_inuse_unlock(inode); > +} Seems a bit overcomplicated. Aren't we always dealing with positive dentries? In which case these can just be trivial wrappers around inode_inuse_{try|un}lock(), or can be gotten rid of completely. > + > static void ovl_put_super(struct super_block *sb) > { > struct ovl_fs *ufs = sb->s_fs_info; > unsigned i; > > + ovl_dir_unlock(ufs->workdir); > dput(ufs->workdir); > + if (ufs->upper_mnt) > + ovl_dir_unlock(ufs->upper_mnt->mnt_root); > mntput(ufs->upper_mnt); > for (i = 0; i < ufs->numlower; i++) > mntput(ufs->lower_mnt[i]); > @@ -407,6 +437,14 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt, > if (retried) > goto out_dput; > > + /* > + * We have parent i_mutex, so this test is race free > + * w.r.t. ovl_dir_lock() below by another overlay mount. > + */ > + err = -EBUSY; > + if (inode_inuse(work->d_inode)) > + goto out_dput; > + Why not lock it here? > retried = true; > ovl_workdir_cleanup(dir, mnt, work, 0); > dput(work); > @@ -446,6 +484,14 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt, > inode_unlock(work->d_inode); > if (err) > goto out_dput; > + > + /* > + * Protect our work dir from being deleted/renamed and from > + * being reused by another overlay mount. > + */ > + err = -EBUSY; > + if (!ovl_dir_lock(work)) > + goto out_dput; > } > out_unlock: > inode_unlock(dir); > @@ -849,6 +895,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > pr_err("overlayfs: failed to clone upperpath\n"); > goto out_put_lowerpath; > } > + /* > + * Protect our upper dir from being deleted/renamed and from > + * being reused by another overlay mount. > + */ > + err = -EBUSY; > + if (!ovl_dir_lock(upperpath.dentry)) { > + pr_err("overlayfs: upperdir in-use by another overlay mount?\n"); > + goto out_put_upper_mnt; > + } > + > /* Don't inherit atime flags */ > ufs->upper_mnt->mnt_flags &= ~(MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME); > > @@ -857,6 +913,10 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > ufs->workdir = ovl_workdir_create(ufs->upper_mnt, workpath.dentry); > err = PTR_ERR(ufs->workdir); > if (IS_ERR(ufs->workdir)) { > + if (err == -EBUSY) { > + pr_err("overlayfs: workdir in-use by another overlay mount?\n"); Why ask? Aren't we sure? > + goto out_unlock_upperdir; > + } > pr_warn("overlayfs: failed to create directory %s/%s (errno: %i); mounting read-only\n", > ufs->config.workdir, OVL_WORKDIR_NAME, -err); > sb->s_flags |= MS_RDONLY; > @@ -874,7 +934,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > > err = ovl_check_d_type_supported(&workpath); > if (err < 0) > - goto out_put_workdir; > + goto out_unlock_workdir; > > /* > * We allowed this configuration and don't want to > @@ -910,7 +970,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > err = -ENOMEM; > ufs->lower_mnt = kcalloc(numlower, sizeof(struct vfsmount *), GFP_KERNEL); > if (ufs->lower_mnt == NULL) > - goto out_put_workdir; > + goto out_unlock_workdir; > for (i = 0; i < numlower; i++) { > struct vfsmount *mnt = clone_private_mount(&stack[i]); > > @@ -1002,8 +1062,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) > for (i = 0; i < ufs->numlower; i++) > mntput(ufs->lower_mnt[i]); > kfree(ufs->lower_mnt); > -out_put_workdir: > +out_unlock_workdir: > + ovl_dir_unlock(ufs->workdir); > dput(ufs->workdir); > +out_unlock_upperdir: > + ovl_dir_unlock(upperpath.dentry); > +out_put_upper_mnt: > mntput(ufs->upper_mnt); > out_put_lowerpath: > for (i = 0; i < numlower; i++) > -- > 2.7.4 >
On Wed, May 31, 2017 at 1:18 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Tue, May 23, 2017 at 11:50 AM, Amir Goldstein <amir73il@gmail.com> wrote: >> Bad things can happen if several concurrent overlay mounts try to >> use the same upperdir path and/or workdir path. >> >> Try to get the 'inuse' advisory lock on upper and work dir. >> Fail mount if another overlay mount instance or another user >> holds the 'inuse' lock. >> >> Note that this provides no protection for concurrent overlay >> mount that use overlapping (i.e. descendant) upper dirs or >> work dirs. >> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> --- >> fs/overlayfs/super.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 67 insertions(+), 3 deletions(-) >> >> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c >> index 4882ffb..ac9212d 100644 >> --- a/fs/overlayfs/super.c >> +++ b/fs/overlayfs/super.c >> @@ -165,12 +165,42 @@ static const struct dentry_operations ovl_reval_dentry_operations = { >> .d_weak_revalidate = ovl_dentry_weak_revalidate, >> }; >> >> +/* Get exclusive ownership on upper/work dir among overlay mounts */ >> +static bool ovl_dir_lock(struct dentry *dentry) >> +{ >> + struct inode *inode; >> + >> + if (!dentry) >> + return false; >> + >> + inode = d_inode(dentry); >> + if (!inode || inode_inuse(inode)) >> + return false; >> + >> + return inode_inuse_trylock(inode); >> +} >> + >> +static void ovl_dir_unlock(struct dentry *dentry) >> +{ >> + struct inode *inode; >> + >> + if (!dentry) >> + return; >> + >> + inode = d_inode(dentry); >> + if (inode && inode_inuse(inode)) >> + inode_inuse_unlock(inode); >> +} > > Seems a bit overcomplicated. Aren't we always dealing with positive > dentries? In which case these can just be trivial wrappers around > inode_inuse_{try|un}lock(), or can be gotten rid of completely. > I find the wrappers convenient for cleanup code, but sure I can make them much thinner, dput() style. >> + >> static void ovl_put_super(struct super_block *sb) >> { >> struct ovl_fs *ufs = sb->s_fs_info; >> unsigned i; >> >> + ovl_dir_unlock(ufs->workdir); >> dput(ufs->workdir); >> + if (ufs->upper_mnt) >> + ovl_dir_unlock(ufs->upper_mnt->mnt_root); >> mntput(ufs->upper_mnt); >> for (i = 0; i < ufs->numlower; i++) >> mntput(ufs->lower_mnt[i]); >> @@ -407,6 +437,14 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt, >> if (retried) >> goto out_dput; >> >> + /* >> + * We have parent i_mutex, so this test is race free >> + * w.r.t. ovl_dir_lock() below by another overlay mount. >> + */ >> + err = -EBUSY; >> + if (inode_inuse(work->d_inode)) >> + goto out_dput; >> + > > Why not lock it here? > >> retried = true; >> ovl_workdir_cleanup(dir, mnt, work, 0); >> dput(work); >> @@ -446,6 +484,14 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt, >> inode_unlock(work->d_inode); >> if (err) >> goto out_dput; >> + >> + /* >> + * Protect our work dir from being deleted/renamed and from >> + * being reused by another overlay mount. >> + */ >> + err = -EBUSY; >> + if (!ovl_dir_lock(work)) >> + goto out_dput; >> } >> out_unlock: >> inode_unlock(dir); >> @@ -849,6 +895,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) >> pr_err("overlayfs: failed to clone upperpath\n"); >> goto out_put_lowerpath; >> } >> + /* >> + * Protect our upper dir from being deleted/renamed and from >> + * being reused by another overlay mount. >> + */ >> + err = -EBUSY; >> + if (!ovl_dir_lock(upperpath.dentry)) { >> + pr_err("overlayfs: upperdir in-use by another overlay mount?\n"); >> + goto out_put_upper_mnt; >> + } >> + >> /* Don't inherit atime flags */ >> ufs->upper_mnt->mnt_flags &= ~(MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME); >> >> @@ -857,6 +913,10 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) >> ufs->workdir = ovl_workdir_create(ufs->upper_mnt, workpath.dentry); >> err = PTR_ERR(ufs->workdir); >> if (IS_ERR(ufs->workdir)) { >> + if (err == -EBUSY) { >> + pr_err("overlayfs: workdir in-use by another overlay mount?\n"); > > Why ask? Aren't we sure? > >> + goto out_unlock_upperdir; >> + } >> pr_warn("overlayfs: failed to create directory %s/%s (errno: %i); mounting read-only\n", >> ufs->config.workdir, OVL_WORKDIR_NAME, -err); >> sb->s_flags |= MS_RDONLY; >> @@ -874,7 +934,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) >> >> err = ovl_check_d_type_supported(&workpath); >> if (err < 0) >> - goto out_put_workdir; >> + goto out_unlock_workdir; >> >> /* >> * We allowed this configuration and don't want to >> @@ -910,7 +970,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) >> err = -ENOMEM; >> ufs->lower_mnt = kcalloc(numlower, sizeof(struct vfsmount *), GFP_KERNEL); >> if (ufs->lower_mnt == NULL) >> - goto out_put_workdir; >> + goto out_unlock_workdir; >> for (i = 0; i < numlower; i++) { >> struct vfsmount *mnt = clone_private_mount(&stack[i]); >> >> @@ -1002,8 +1062,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) >> for (i = 0; i < ufs->numlower; i++) >> mntput(ufs->lower_mnt[i]); >> kfree(ufs->lower_mnt); >> -out_put_workdir: >> +out_unlock_workdir: >> + ovl_dir_unlock(ufs->workdir); >> dput(ufs->workdir); >> +out_unlock_upperdir: >> + ovl_dir_unlock(upperpath.dentry); >> +out_put_upper_mnt: >> mntput(ufs->upper_mnt); >> out_put_lowerpath: >> for (i = 0; i < numlower; i++) >> -- >> 2.7.4 >>
On Wed, May 31, 2017 at 3:47 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Wed, May 31, 2017 at 1:18 PM, Miklos Szeredi <miklos@szeredi.hu> wrote: >> On Tue, May 23, 2017 at 11:50 AM, Amir Goldstein <amir73il@gmail.com> wrote: >>> Bad things can happen if several concurrent overlay mounts try to >>> use the same upperdir path and/or workdir path. >>> >>> Try to get the 'inuse' advisory lock on upper and work dir. >>> Fail mount if another overlay mount instance or another user >>> holds the 'inuse' lock. >>> >>> Note that this provides no protection for concurrent overlay >>> mount that use overlapping (i.e. descendant) upper dirs or >>> work dirs. >>> >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com> >>> --- ... >>> @@ -407,6 +437,14 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt, >>> if (retried) >>> goto out_dput; >>> >>> + /* >>> + * We have parent i_mutex, so this test is race free >>> + * w.r.t. ovl_dir_lock() below by another overlay mount. >>> + */ >>> + err = -EBUSY; >>> + if (inode_inuse(work->d_inode)) >>> + goto out_dput; >>> + >> >> Why not lock it here? Because we are locking 'work' inode, not workdir and the 'work' inode is about to be zapped and replaced with a new work inode on retry. Are you suggesting to move the inuse lock to the workdir inode? doable. I guess I choose to lock workdir/work because of the may_delete protection it provides, but you questioned that part anyway. >> >>> retried = true; >>> ovl_workdir_cleanup(dir, mnt, work, 0); >>> dput(work); >>> @@ -446,6 +484,14 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt, >>> inode_unlock(work->d_inode); >>> if (err) >>> goto out_dput; >>> + >>> + /* >>> + * Protect our work dir from being deleted/renamed and from >>> + * being reused by another overlay mount. >>> + */ >>> + err = -EBUSY; >>> + if (!ovl_dir_lock(work)) >>> + goto out_dput; >>> } >>> out_unlock: >>> inode_unlock(dir); >>> @@ -849,6 +895,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) >>> pr_err("overlayfs: failed to clone upperpath\n"); >>> goto out_put_lowerpath; >>> } >>> + /* >>> + * Protect our upper dir from being deleted/renamed and from >>> + * being reused by another overlay mount. >>> + */ >>> + err = -EBUSY; >>> + if (!ovl_dir_lock(upperpath.dentry)) { >>> + pr_err("overlayfs: upperdir in-use by another overlay mount?\n"); >>> + goto out_put_upper_mnt; >>> + } >>> + >>> /* Don't inherit atime flags */ >>> ufs->upper_mnt->mnt_flags &= ~(MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME); >>> >>> @@ -857,6 +913,10 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) >>> ufs->workdir = ovl_workdir_create(ufs->upper_mnt, workpath.dentry); >>> err = PTR_ERR(ufs->workdir); >>> if (IS_ERR(ufs->workdir)) { >>> + if (err == -EBUSY) { >>> + pr_err("overlayfs: workdir in-use by another overlay mount?\n"); >> >> Why ask? Aren't we sure? >> So I think ovl_workdir_cleanup() can also return EBUSY if one of the dirs/files inside it are used as a mount point or a dir used as a rootdir. Also, since inode_inuse() is not overlay specific, cannot rule out the option of some other code setting inode_inuse on workdir, thus the "?". I don't mind dropping the "?" though - perhaps phrase the error more generically: pr_err("overlayfs: workdir is in-use by another mount\n"); The man page for mount(2) has a broad phrasing for EBUSY - "target is still busy (... etc.)".
On Wed, May 31, 2017 at 3:05 PM, Amir Goldstein <amir73il@gmail.com> wrote: > Are you suggesting to move the inuse lock to the workdir inode? doable. > I guess I choose to lock workdir/work because of the may_delete > protection it provides, > but you questioned that part anyway. Yes, I think we should protect workdir/upperdir with I_INUSE. > I don't mind dropping the "?" though - perhaps phrase the error more > generically: > pr_err("overlayfs: workdir is in-use by another mount\n"); Okay. Thanks, Miklos
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 4882ffb..ac9212d 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -165,12 +165,42 @@ static const struct dentry_operations ovl_reval_dentry_operations = { .d_weak_revalidate = ovl_dentry_weak_revalidate, }; +/* Get exclusive ownership on upper/work dir among overlay mounts */ +static bool ovl_dir_lock(struct dentry *dentry) +{ + struct inode *inode; + + if (!dentry) + return false; + + inode = d_inode(dentry); + if (!inode || inode_inuse(inode)) + return false; + + return inode_inuse_trylock(inode); +} + +static void ovl_dir_unlock(struct dentry *dentry) +{ + struct inode *inode; + + if (!dentry) + return; + + inode = d_inode(dentry); + if (inode && inode_inuse(inode)) + inode_inuse_unlock(inode); +} + static void ovl_put_super(struct super_block *sb) { struct ovl_fs *ufs = sb->s_fs_info; unsigned i; + ovl_dir_unlock(ufs->workdir); dput(ufs->workdir); + if (ufs->upper_mnt) + ovl_dir_unlock(ufs->upper_mnt->mnt_root); mntput(ufs->upper_mnt); for (i = 0; i < ufs->numlower; i++) mntput(ufs->lower_mnt[i]); @@ -407,6 +437,14 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt, if (retried) goto out_dput; + /* + * We have parent i_mutex, so this test is race free + * w.r.t. ovl_dir_lock() below by another overlay mount. + */ + err = -EBUSY; + if (inode_inuse(work->d_inode)) + goto out_dput; + retried = true; ovl_workdir_cleanup(dir, mnt, work, 0); dput(work); @@ -446,6 +484,14 @@ static struct dentry *ovl_workdir_create(struct vfsmount *mnt, inode_unlock(work->d_inode); if (err) goto out_dput; + + /* + * Protect our work dir from being deleted/renamed and from + * being reused by another overlay mount. + */ + err = -EBUSY; + if (!ovl_dir_lock(work)) + goto out_dput; } out_unlock: inode_unlock(dir); @@ -849,6 +895,16 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) pr_err("overlayfs: failed to clone upperpath\n"); goto out_put_lowerpath; } + /* + * Protect our upper dir from being deleted/renamed and from + * being reused by another overlay mount. + */ + err = -EBUSY; + if (!ovl_dir_lock(upperpath.dentry)) { + pr_err("overlayfs: upperdir in-use by another overlay mount?\n"); + goto out_put_upper_mnt; + } + /* Don't inherit atime flags */ ufs->upper_mnt->mnt_flags &= ~(MNT_NOATIME | MNT_NODIRATIME | MNT_RELATIME); @@ -857,6 +913,10 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) ufs->workdir = ovl_workdir_create(ufs->upper_mnt, workpath.dentry); err = PTR_ERR(ufs->workdir); if (IS_ERR(ufs->workdir)) { + if (err == -EBUSY) { + pr_err("overlayfs: workdir in-use by another overlay mount?\n"); + goto out_unlock_upperdir; + } pr_warn("overlayfs: failed to create directory %s/%s (errno: %i); mounting read-only\n", ufs->config.workdir, OVL_WORKDIR_NAME, -err); sb->s_flags |= MS_RDONLY; @@ -874,7 +934,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) err = ovl_check_d_type_supported(&workpath); if (err < 0) - goto out_put_workdir; + goto out_unlock_workdir; /* * We allowed this configuration and don't want to @@ -910,7 +970,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) err = -ENOMEM; ufs->lower_mnt = kcalloc(numlower, sizeof(struct vfsmount *), GFP_KERNEL); if (ufs->lower_mnt == NULL) - goto out_put_workdir; + goto out_unlock_workdir; for (i = 0; i < numlower; i++) { struct vfsmount *mnt = clone_private_mount(&stack[i]); @@ -1002,8 +1062,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent) for (i = 0; i < ufs->numlower; i++) mntput(ufs->lower_mnt[i]); kfree(ufs->lower_mnt); -out_put_workdir: +out_unlock_workdir: + ovl_dir_unlock(ufs->workdir); dput(ufs->workdir); +out_unlock_upperdir: + ovl_dir_unlock(upperpath.dentry); +out_put_upper_mnt: mntput(ufs->upper_mnt); out_put_lowerpath: for (i = 0; i < numlower; i++)
Bad things can happen if several concurrent overlay mounts try to use the same upperdir path and/or workdir path. Try to get the 'inuse' advisory lock on upper and work dir. Fail mount if another overlay mount instance or another user holds the 'inuse' lock. Note that this provides no protection for concurrent overlay mount that use overlapping (i.e. descendant) upper dirs or work dirs. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/overlayfs/super.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 67 insertions(+), 3 deletions(-)