On Wed, May 31, 2017 at 3:47 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Wed, May 31, 2017 at 1:18 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: >> On Tue, May 23, 2017 at 11:50 AM, Amir Goldstein <amir73il@xxxxxxxxx> 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@xxxxxxxxx> >>> --- ... >>> @@ -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.)". -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html