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 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>
>> ---
>>  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
>>



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux