[RFC PATCH] ovl: properly release old cache entries in ovl_cache_get()

[RFC PATCH] ovl: properly release old cache entries in ovl_cache_get()

[Paul Moore]

If an old readdir cache entry is found during lookup we need to
ensure that we drop a reference to the old cache entry before
we remove it from the cache.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 fs/overlayfs/readdir.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 78f62cc1797b..404e6849ff75 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -235,10 +235,8 @@ void ovl_dir_cache_free(struct inode *inode)
 	}
 }
 
-static void ovl_cache_put(struct ovl_dir_file *od, struct dentry *dentry)
+static void ovl_cache_put(struct ovl_dir_cache *cache, struct dentry *dentry)
 {
-	struct ovl_dir_cache *cache = od->cache;
-
 	WARN_ON(cache->refcount <= 0);
 	cache->refcount--;
 	if (!cache->refcount) {
@@ -327,7 +325,7 @@ static void ovl_dir_reset(struct file *file)
 	bool is_real;
 
 	if (cache && ovl_dentry_version_get(dentry) != cache->version) {
-		ovl_cache_put(od, dentry);
+		ovl_cache_put(cache, dentry);
 		od->cache = NULL;
 		od->cursor = NULL;
 	}
@@ -396,12 +394,15 @@ static struct ovl_dir_cache *ovl_cache_get(struct dentry *dentry)
 	struct ovl_dir_cache *cache;
 
 	cache = ovl_dir_cache(d_inode(dentry));
-	if (cache && ovl_dentry_version_get(dentry) == cache->version) {
-		WARN_ON(!cache->refcount);
-		cache->refcount++;
-		return cache;
+	if (cache) {
+		if (ovl_dentry_version_get(dentry) == cache->version) {
+			WARN_ON(!cache->refcount);
+			cache->refcount++;
+			return cache;
+		}
+		ovl_set_dir_cache(d_inode(dentry), NULL);
+		ovl_cache_put(cache, dentry);
 	}
-	ovl_set_dir_cache(d_inode(dentry), NULL);
 
 	cache = kzalloc(sizeof(struct ovl_dir_cache), GFP_KERNEL);
 	if (!cache)
@@ -913,7 +914,7 @@ static int ovl_dir_release(struct inode *inode, struct file *file)
 
 	if (od->cache) {
 		inode_lock(inode);
-		ovl_cache_put(od, file->f_path.dentry);
+		ovl_cache_put(od->cache, file->f_path.dentry);
 		inode_unlock(inode);
 	}
 	fput(od->realfile);

Re: [RFC PATCH] ovl: properly release old cache entries in ovl_cache_get()

[Paul Moore]

On Fri, Jul 1, 2022 at 10:11 AM Paul Moore <paul@paul-moore.com> wrote:
>
> If an old readdir cache entry is found during lookup we need to
> ensure that we drop a reference to the old cache entry before
> we remove it from the cache.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  fs/overlayfs/readdir.c |   21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)

I ran across this a few months ago while working on something related
in overlayfs' readdir cache, unfortunately that work has been shelved
for now, but it seems like this bugfix might still have merit,
although I'll leave that decision up to the overlayfs experts; it's
very possible I've missed an important detail and this isn't actually
a bug.

I've done some basic manual testing (kernel boots,
mounting/traversal/accesses are all okay), but nothing exhaustive.

-- 
paul-moore.com

Re: [RFC PATCH] ovl: properly release old cache entries in ovl_cache_get()

[Paul Moore]

On Fri, Jul 1, 2022 at 10:15 AM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Jul 1, 2022 at 10:11 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > If an old readdir cache entry is found during lookup we need to
> > ensure that we drop a reference to the old cache entry before
> > we remove it from the cache.
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> >  fs/overlayfs/readdir.c |   21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
>
> I ran across this a few months ago while working on something related
> in overlayfs' readdir cache, unfortunately that work has been shelved
> for now, but it seems like this bugfix might still have merit,
> although I'll leave that decision up to the overlayfs experts; it's
> very possible I've missed an important detail and this isn't actually
> a bug.
>
> I've done some basic manual testing (kernel boots,
> mounting/traversal/accesses are all okay), but nothing exhaustive.

Based on the lack of a response, should I assume this is not a bug and
this patch is not needed?

-- 
paul-moore.com

Re: [RFC PATCH] ovl: properly release old cache entries in ovl_cache_get()

[Miklos Szeredi]

On Mon, 11 Jul 2022 at 22:11, Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Jul 1, 2022 at 10:15 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Jul 1, 2022 at 10:11 AM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > If an old readdir cache entry is found during lookup we need to
> > > ensure that we drop a reference to the old cache entry before
> > > we remove it from the cache.
> > >
> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > > ---
> > >  fs/overlayfs/readdir.c |   21 +++++++++++----------
> > >  1 file changed, 11 insertions(+), 10 deletions(-)
> >
> > I ran across this a few months ago while working on something related
> > in overlayfs' readdir cache, unfortunately that work has been shelved
> > for now, but it seems like this bugfix might still have merit,
> > although I'll leave that decision up to the overlayfs experts; it's
> > very possible I've missed an important detail and this isn't actually
> > a bug.
> >
> > I've done some basic manual testing (kernel boots,
> > mounting/traversal/accesses are all okay), but nothing exhaustive.
>
> Based on the lack of a response, should I assume this is not a bug and
> this patch is not needed?

Hi Paul,

Sorry for the late response.

Yes, the code is okay, though could be better documented.   The logic
is that only open directories contain counted references to the cache,
not the directory inode.  The uncounted reference from the inode is
used to allow sharing the cache in case there are mulitple directory
readers. Thus the ref from the inode can be dropped without
decrementing the count, and this reference is reset to NULL when the
count hits zero.  Locking is provided by i_rwsem.

Thanks,
Miklos

Re: [RFC PATCH] ovl: properly release old cache entries in ovl_cache_get()

[Paul Moore]

On Tue, Jul 12, 2022 at 9:05 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Mon, 11 Jul 2022 at 22:11, Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Jul 1, 2022 at 10:15 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Jul 1, 2022 at 10:11 AM Paul Moore <paul@paul-moore.com> wrote:
> > > >
> > > > If an old readdir cache entry is found during lookup we need to
> > > > ensure that we drop a reference to the old cache entry before
> > > > we remove it from the cache.
> > > >
> > > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > > > ---
> > > >  fs/overlayfs/readdir.c |   21 +++++++++++----------
> > > >  1 file changed, 11 insertions(+), 10 deletions(-)
> > >
> > > I ran across this a few months ago while working on something related
> > > in overlayfs' readdir cache, unfortunately that work has been shelved
> > > for now, but it seems like this bugfix might still have merit,
> > > although I'll leave that decision up to the overlayfs experts; it's
> > > very possible I've missed an important detail and this isn't actually
> > > a bug.
> > >
> > > I've done some basic manual testing (kernel boots,
> > > mounting/traversal/accesses are all okay), but nothing exhaustive.
> >
> > Based on the lack of a response, should I assume this is not a bug and
> > this patch is not needed?
>
> Hi Paul,
>
> Sorry for the late response.
>
> Yes, the code is okay, though could be better documented.   The logic
> is that only open directories contain counted references to the cache,
> not the directory inode.  The uncounted reference from the inode is
> used to allow sharing the cache in case there are mulitple directory
> readers. Thus the ref from the inode can be dropped without
> decrementing the count, and this reference is reset to NULL when the
> count hits zero.  Locking is provided by i_rwsem.

Great, I'm glad to hear the existing code is working as intended.
Thanks for the explanation too!

-- 
paul-moore.com