[PATCH] ovl: fix dir cache leak for ovl_cache_get

[PATCH] ovl: fix dir cache leak for ovl_cache_get

[Rock Lee]

In ovl_cache_get, there is a chance that ovl_dentry_version_get and
cache->version are not equal. Before set the cache to NULL, free it
first.

Signed-off-by: Rock Lee <rockdotlee@gmail.com>
---
 fs/overlayfs/readdir.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index f241b4e..b3af3d3 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -331,6 +331,8 @@ static struct ovl_dir_cache *ovl_cache_get(struct dentry *dentry)
 		cache->refcount++;
 		return cache;
 	}
+
+	kfree(cache);
 	ovl_set_dir_cache(dentry, NULL);
 
 	cache = kzalloc(sizeof(struct ovl_dir_cache), GFP_KERNEL);
-- 
1.9.1

Re: [PATCH] ovl: fix dir cache leak for ovl_cache_get

[Amir Goldstein]

On Thu, May 4, 2017 at 10:07 AM, Rock Lee <rockdotlee@gmail.com> wrote:
> In ovl_cache_get, there is a chance that ovl_dentry_version_get and
> cache->version are not equal. Before set the cache to NULL, free it
> first.
>

This is not a leak. dir cache is refcounted and freed on
ovl_dir_{release|reset}() -> ovl_cache_put()

> Signed-off-by: Rock Lee <rockdotlee@gmail.com>
> ---
>  fs/overlayfs/readdir.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index f241b4e..b3af3d3 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -331,6 +331,8 @@ static struct ovl_dir_cache *ovl_cache_get(struct dentry *dentry)
>                 cache->refcount++;
>                 return cache;
>         }
> +
> +       kfree(cache);
>         ovl_set_dir_cache(dentry, NULL);
>
>         cache = kzalloc(sizeof(struct ovl_dir_cache), GFP_KERNEL);
> --
> 1.9.1
>

Re: [PATCH] ovl: fix dir cache leak for ovl_cache_get

[Rock Lee]

Hi, Amir:

> This is not a leak. dir cache is refcounted and freed on
> ovl_dir_{release|reset}() -> ovl_cache_put()

Here is the snippet:

if (cache && ovl_dentry_version_get(dentry) == cache->version) {
cache->refcount++;
return cache;
}

ovl_set_dir_cache(dentry, NULL);

I am a little confused here. From the logic of the code, there must
have a circumstance --- cache is not NULL and
ovl_dentry_version_get(dentry) != cache->version. If
ovl_set_dir_cache(dentry, NULL), we can't find the old dir cache  form
dentry and struct file(later, file->private_data->cache will point at
the same dir cache with dentry). How could we free it in
ovl_dir_{release|reset} ?

BTW, could you please an example when ovl_dentry_version_get(dentry)
!= cache->version in ovl_cache_get ?

Thanks for your time.
-- 
Cheers,
Rock

Re: [PATCH] ovl: fix dir cache leak for ovl_cache_get

[Amir Goldstein]

On Fri, May 5, 2017 at 9:34 AM, Rock Lee <rockdotlee@gmail.com> wrote:
> Hi, Amir:
>
>> This is not a leak. dir cache is refcounted and freed on
>> ovl_dir_{release|reset}() -> ovl_cache_put()
>
> Here is the snippet:
>
> if (cache && ovl_dentry_version_get(dentry) == cache->version) {
> cache->refcount++;
> return cache;
> }
>
> ovl_set_dir_cache(dentry, NULL);
>
> I am a little confused here. From the logic of the code, there must
> have a circumstance --- cache is not NULL and
> ovl_dentry_version_get(dentry) != cache->version. If
> ovl_set_dir_cache(dentry, NULL), we can't find the old dir cache  form
> dentry and struct file(later, file->private_data->cache will point at
> the same dir cache with dentry). How could we free it in
> ovl_dir_{release|reset} ?
>

It has a reference from another struct file, not this one
and cache will be freed when the last file that references it
is closed:

static int ovl_dir_release(struct inode *inode, struct file *file)
{
        struct ovl_dir_file *od = file->private_data;

        if (od->cache) {
                inode_lock(inode);
                ovl_cache_put(od, file->f_path.dentry);
                inode_unlock(inode);
        }

> BTW, could you please an example when ovl_dentry_version_get(dentry)
> != cache->version in ovl_cache_get ?
>

In theory (didn't try this code):

d1 = opendir("foo");
readdir(d1); /* populate d1's od->cache and read first entry */
pos = telldir(d1);
creat("foo/bar", 0600); /* ovl_dentry_version_inc() */
d2 = opendir("foo");
seekdir(d2, pos);
readdir(d2); /* populate d2's od->cache and read second entry */

d2's od->cache is NULL, ctx->pos = 1, and ovl_get_cache() will
find that ovl_dir_cache() is out dated and will create a new cache,
which should contain the new file "bar".

Amir.

Re: [PATCH] ovl: fix dir cache leak for ovl_cache_get

[Rock Lee]

On Sun, May 7, 2017 at 7:34 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, May 5, 2017 at 9:34 AM, Rock Lee <rockdotlee@gmail.com> wrote:
>> Hi, Amir:
>>
>>> This is not a leak. dir cache is refcounted and freed on
>>> ovl_dir_{release|reset}() -> ovl_cache_put()
>>
>> Here is the snippet:
>>
>> if (cache && ovl_dentry_version_get(dentry) == cache->version) {
>> cache->refcount++;
>> return cache;
>> }
>>
>> ovl_set_dir_cache(dentry, NULL);
>>
>> I am a little confused here. From the logic of the code, there must
>> have a circumstance --- cache is not NULL and
>> ovl_dentry_version_get(dentry) != cache->version. If
>> ovl_set_dir_cache(dentry, NULL), we can't find the old dir cache  form
>> dentry and struct file(later, file->private_data->cache will point at
>> the same dir cache with dentry). How could we free it in
>> ovl_dir_{release|reset} ?
>>
>
> It has a reference from another struct file, not this one
> and cache will be freed when the last file that references it
> is closed:
>
> static int ovl_dir_release(struct inode *inode, struct file *file)
> {
>         struct ovl_dir_file *od = file->private_data;
>
>         if (od->cache) {
>                 inode_lock(inode);
>                 ovl_cache_put(od, file->f_path.dentry);
>                 inode_unlock(inode);
>         }
>
>> BTW, could you please an example when ovl_dentry_version_get(dentry)
>> != cache->version in ovl_cache_get ?
>>
>
> In theory (didn't try this code):
>
> d1 = opendir("foo");
> readdir(d1); /* populate d1's od->cache and read first entry */
> pos = telldir(d1);
> creat("foo/bar", 0600); /* ovl_dentry_version_inc() */
> d2 = opendir("foo");
> seekdir(d2, pos);
> readdir(d2); /* populate d2's od->cache and read second entry */
>
> d2's od->cache is NULL, ctx->pos = 1, and ovl_get_cache() will
> find that ovl_dir_cache() is out dated and will create a new cache,
> which should contain the new file "bar".

Thanks for your explaination, I leaned a lot. I have tested your code,
and it met your expectation ;-).

One more question, only ctx->pos == 0 in ovl_iterate would cause
ovl_dir_reset and rebuild dir cache which means in some scenarios, dir
cache may be not up to date. Consider this code:

    DIR *dirp = opendir("foo");
    readdir(dirp);                /* make sure ctx->pos != 0 in the
blow readir() */
    creat("foo/bar" ,0600);  /* ovl_dentry_version_inc() */
    while ((entry = readdir(dirp)) != NULL) {
        printf("name %s\n", entry->d_name);
    }

In this case, "foo/bar" will not be shown in the output because
readdir will not rebuild dir cache. Could we just ovl_dir_reset()
everytime regardless of ctx->pos in ovl_iterate() ? Once files
created/removed/renamed in a directory, ovl_iterate will rebuild dir
cache and keep the dir cache up to date?

-- 
Cheers,
Rock

Re: [PATCH] ovl: fix dir cache leak for ovl_cache_get

[Amir Goldstein]

On Mon, May 8, 2017 at 12:54 PM, Rock Lee <rockdotlee@gmail.com> wrote:

>
> One more question, only ctx->pos == 0 in ovl_iterate would cause
> ovl_dir_reset and rebuild dir cache which means in some scenarios, dir
> cache may be not up to date. Consider this code:
>
>     DIR *dirp = opendir("foo");
>     readdir(dirp);                /* make sure ctx->pos != 0 in the
> blow readir() */
>     creat("foo/bar" ,0600);  /* ovl_dentry_version_inc() */
>     while ((entry = readdir(dirp)) != NULL) {
>         printf("name %s\n", entry->d_name);
>     }
>
> In this case, "foo/bar" will not be shown in the output because
> readdir will not rebuild dir cache. Could we just ovl_dir_reset()
> everytime regardless of ctx->pos in ovl_iterate() ? Once files
> created/removed/renamed in a directory, ovl_iterate will rebuild dir
> cache and keep the dir cache up to date?
>

We can't because:

1. After rebuilding the cache, you have no guaranty that ctx->pos is
pointing AFTER all the entries that you already read BEFORE rebuilding
the cache, so the result list can be a total disaster.

2. Even if we have a way to use persistent iterator position somehow,
rebuilding the cache on every iteration for a large dir is expensive.

Anyway, the situation you described - skipping an entry that was created
AFTER dir iteration has started can happen on any file system, so it's not
even a bug what you suggest to fix.

Amir.