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

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

 



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);
+       }
+
+       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;
+               *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