ovl: fix dir cache leak for ovl_cache_get

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 <[email protected]>

---
 fs/overlayfs/readdir.c | 2 ++
 1 file changed, 2 insertions(+)

fs/overlayfs/readdir.c +2 -1

index f241b4e..b3af3d3 100644
@@ -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

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

Hi, Amir:

Quoted 2 lines

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 ?

Quoted 14 lines · Rock Lee

Hi, Amir:

Quoted 2 lines

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

Quoted 8 lines

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

Quoted 3 lines

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

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?