Re: [PATCH 2/2] ovl: get exclusive ownership on upper/work dirs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux