Re: [PATCH v2 03/17] ovl: decode pure upper file handles

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

 



On Fri, Jan 19, 2018 at 12:57 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Thu, Jan 18, 2018 at 10:35 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>> On Thu, Jan 18, 2018 at 10:10 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>>> On Thu, Jan 18, 2018 at 8:49 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>>>> On Thu, Jan 18, 2018 at 4:39 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>>>>> On Thu, Jan 18, 2018 at 3:34 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>>>>>> On Thu, Jan 18, 2018 at 4:09 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>>>>>>> [Added Al Viro]
>>>>>>>
>>>>>>> On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>>>>>>>> Decoding an upper file handle is done by decoding the upper dentry from
>>>>>>>> underlying upper fs, finding or allocating an overlay inode that is
>>>>>>>> hashed by the real upper inode and instantiating an overlay dentry with
>>>>>>>> that inode.
>>>>>>>>
>>>>>>>> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>>>>>>>> ---
>>>>>>>>  fs/overlayfs/export.c    | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  fs/overlayfs/namei.c     |  4 +--
>>>>>>>>  fs/overlayfs/overlayfs.h |  2 ++
>>>>>>>>  3 files changed, 95 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
>>>>>>>> index 58c4f5e8a67e..5c72784a0b4d 100644
>>>>>>>> --- a/fs/overlayfs/export.c
>>>>>>>> +++ b/fs/overlayfs/export.c
>>>>>>>> @@ -93,6 +93,97 @@ static int ovl_encode_inode_fh(struct inode *inode, u32 *fid, int *max_len,
>>>>>>>>         return type;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +/*
>>>>>>>> + * Find or instantiate an overlay dentry from real dentries.
>>>>>>>> + */
>>>>>>>> +static struct dentry *ovl_obtain_alias(struct super_block *sb,
>>>>>>>> +                                      struct dentry *upper,
>>>>>>>> +                                      struct ovl_path *lowerpath)
>>>>>>>> +{
>>>>>>>> +       struct inode *inode;
>>>>>>>> +       struct dentry *dentry;
>>>>>>>> +       struct ovl_entry *oe;
>>>>>>>> +
>>>>>>>> +       /* TODO: obtain non pure-upper */
>>>>>>>> +       if (lowerpath)
>>>>>>>> +               return ERR_PTR(-EIO);
>>>>>>>> +
>>>>>>>> +       inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0);
>>>>>>>> +       if (IS_ERR(inode)) {
>>>>>>>> +               dput(upper);
>>>>>>>> +               return ERR_CAST(inode);
>>>>>>>> +       }
>>>>>>>> +
>>>>>>>> +       dentry = d_obtain_alias(inode);
>>>>>>>> +       if (IS_ERR(dentry) || dentry->d_fsdata)
>>>>>>>
>>>>>>> Racing two instances of this code, each thinking it got a new alias
>>>>>>> and trying to fill it, results in a memory leak.
>>>>>>>
>>>>>>> Haven't checked in too much depth, but apparently other filesystems
>>>>>>> are not affected, so we need something special here.
>>>>>>>
>>>>>>> One solution: split d_instantiate_anon(dentry, inode) out of
>>>>>>> __d_obtain_alias() and supply that with the already initialized
>>>>>>> dentry.
>>>>>>>
>>>>>>
>>>>>> Can't we use &OVL_I(inode)->lock to avoid the race?
>>>>>
>>>>> We could.  But then d_splice_alias() will find our half baked dentry
>>>>> and return that from ovl_lookup().
>>>>
>>>> No it won't, because we do not obtain dir dentries this way.
>>>> We actually do in this patch [3/17], but since patch [4/17] we don't,
>>>> so I only need to fix this patch not to obtain dir dentry and to
>>>> protect concurrent decode of non-dir with &OVL_I(inode)->lock.
>>>>
>>>>> So we do need to have the dentry
>>>>> fully initialized by the time it's added into the inode's alias list.
>>>>>
>>>>
>>>> The only problems I see with adding a non-dir disconnected alias
>>>> that is not fully baked are:
>>>> 1. We can get it in ovl_encode_inode_fh() from d_find_any_alias()
>>>> 2. nfsd can get it in exportfs_decode_fh() from find_acceptable_alias()
>>>>     in a weird hypothetical case where the fully baked dentry we just
>>>>     returned from ovl_obtain_alias() in NOT acceptable by nfsd but
>>>>     the half baked dentry IS acceptable
>>>> 3. Another kernel user that uses d_find_any_alias() or one of the use
>>>>     case that only Al can think of...
>>>>
>>>> Cases 2 and 3, I don't know if they are for real.
>>>>
>>>> Case 1 is only a problem due to lack of export_operations method
>>>> 'dentry_to_fh'. exportfs_encode_fh() has the right dentry, but it does
>>>> not pass it to the filesystem for encoding, so I think it should be
>>>> solved by adding this method.
>>>
>>> I agree with your analysis.
>>>
>>> However, I don't see what's wrong with adding fully baked dentries to
>>> the inode.
>>
>> I agree that adding half baked dentries is not a good practice and
>> we should avoid it.
>>
>
> How is this for an option?
>
> ===========================================
>
> +/*
> + * Find or instantiate an overlay dentry from real dentries.
> + */
> +static struct dentry *ovl_obtain_alias(struct super_block *sb,
> +                                      struct dentry *upper,
> +                                      struct ovl_path *lowerpath)
> +{
> +       struct inode *inode;
> +       struct dentry *dentry;
> +       struct ovl_entry *oe;
> +       void *fsdata = &oe;
> +
> +       /* TODO: obtain non pure-upper */
> +       if (lowerpath)
> +               return ERR_PTR(-EIO);
> +
> +       inode = ovl_get_inode(sb, dget(upper), NULL, NULL, 0);
> +       if (IS_ERR(inode)) {
> +               dput(upper);
> +               return ERR_CAST(inode);
> +       }
> +

With optimistic find:

+       /* First try our luck to find a cached dentry */
+       dentry = d_find_any_alias(inode);
+       if (dentry) {
+               iput(inode);
+               return dentry;
+       }
+
+       /* Then allocate ovl_entry, but free it if we do find a cached dentry */

> +       oe = ovl_alloc_entry(0);
> +       if (!oe) {
> +               iput(inode);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +       oe->has_upper = true;
> +
> +       dentry = d_obtain_alias_fsdata(inode, fsdata);
> +       /* A new allocated dentry assigns *fsdata and sets it to NULL */
> +       if (oe)
> +               kfree(oe);
> +
> +       return dentry;
> +}
> +
>
>
> ...
>
>
> -static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected)
> +static struct dentry *__d_obtain_alias(struct inode *inode, int disconnected,
> +                                      void **fsdata)
>  {
>         struct dentry *tmp;
>         struct dentry *res;
> @@ -1962,6 +1963,12 @@ static struct dentry *__d_obtain_alias(struct
> inode *inode, int disconnected)
>         if (disconnected)
>                 add_flags |= DCACHE_DISCONNECTED;
>
> +       /* Take ownership of pre-allocated fs-specific data */
> +       if (fsdata) {
> +               tmp->d_fsdata = fsdata;

And without the bug:
 +               tmp->d_fsdata = *fsdata;


> +               *fsdata = NULL;
> +       }
> +
>         spin_lock(&tmp->d_lock);
>         __d_set_inode_and_type(tmp, inode, add_flags);
>         hlist_add_head(&tmp->d_u.d_alias, &inode->i_dentry);
> @@ -1998,7 +2005,13 @@ static struct dentry *__d_obtain_alias(struct
> inode *inode, int disconnected)
>   */
>  struct dentry *d_obtain_alias(struct inode *inode)
>  {
> -       return __d_obtain_alias(inode, 1);
> +       return __d_obtain_alias(inode, 1, NULL);
> +}
> +EXPORT_SYMBOL(d_obtain_alias);
> +
> +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata)
> +{
> +       return __d_obtain_alias(inode, 1, fsdata);
>  }
>  EXPORT_SYMBOL(d_obtain_alias);
>
> @@ -2019,7 +2032,7 @@ EXPORT_SYMBOL(d_obtain_alias);
>   */
>  struct dentry *d_obtain_root(struct inode *inode)
>  {
> -       return __d_obtain_alias(inode, 0);
> +       return __d_obtain_alias(inode, 0, NULL);
>  }
>  EXPORT_SYMBOL(d_obtain_root);



[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