Re: [PATCH] ovl: hash directory inodes for fsnotify

[Miklos Szeredi <miklos@szeredi.hu>]

On Sun, Jan 14, 2018 at 06:35:40PM +0200, Amir Goldstein wrote:
> fsnotify pins a watched directory inode in cache, but if directory dentry
> is released, new lookup will allocate a new dentry and a new inode.
> Directory events will be notified on the new inode, while fsnotify listener
> is watching the old pinned inode.
> 
> Hash all directory inodes to reuse the pinned inode on lookup. Pure upper
> dirs are hashes by real upper inode, merge and lower dirs are hashed by
> real lower inode.
> 
> The reference to lower inode was being held by the lower dentry object
> in the overlay dentry (oe->lowerstack[0]). Releasing the overlay dentry
> may drop lower inode refcount to zero. Add a refcount on behalf of the
> overlay inode to prevent that.
> 
> As a by-product, hashing directory inodes also detects multiple
> redirected dirs to the same lower dir and uncovered redirected dir
> target on and returns -ESTALE on lookup.
> 
> The reported issue dates back to initial version of overlayfs, but this
> patch depends on ovl_inode code that was introduced in kernel v4.13.

One nit: we are changing i_nlink for the overlay inode.  Not sure it matters,
but better keep it consistent with st_nlink.  So added the following incremental
patch on top of yours.

Thanks,
Miklos


Index: linux/fs/overlayfs/inode.c
===================================================================
--- linux.orig/fs/overlayfs/inode.c	2018-01-15 09:29:25.447748554 +0100
+++ linux/fs/overlayfs/inode.c	2018-01-15 09:29:13.506835860 +0100
@@ -645,6 +645,7 @@ struct inode *ovl_get_inode(struct dentr
 	/* Already indexed or could be indexed on copy up? */
 	bool indexed = (index || (ovl_indexdir(dentry->d_sb) && !upperdentry));
 	struct dentry *origin = indexed ? lowerdentry : NULL;
+	bool is_dir;
 
 	if (WARN_ON(upperdentry && indexed && !lowerdentry))
 		return ERR_PTR(-EIO);
@@ -659,12 +660,13 @@ struct inode *ovl_get_inode(struct dentr
 	 * Hash dir that is or could be merged by origin inode.
 	 * Hash pure upper and non-indexed non-dir by upper inode.
 	 */
-	if (S_ISDIR(realinode->i_mode))
+	is_dir = S_ISDIR(realinode->i_mode);
+	if (is_dir)
 		origin = lowerdentry;
 
 	if (upperdentry || origin) {
 		struct inode *key = d_inode(origin ?: upperdentry);
-		unsigned int nlink;
+		unsigned int nlink = 1;
 
 		inode = iget5_locked(dentry->d_sb, (unsigned long) key,
 				     ovl_inode_test, ovl_inode_set, key);
@@ -685,8 +687,9 @@ struct inode *ovl_get_inode(struct dentr
 			goto out;
 		}
 
-		nlink = ovl_get_nlink(lowerdentry, upperdentry,
-				      realinode->i_nlink);
+		if (!is_dir)
+			nlink = ovl_get_nlink(lowerdentry, upperdentry,
+					      realinode->i_nlink);
 		set_nlink(inode, nlink);
 	} else {
 		inode = new_inode(dentry->d_sb);
@@ -700,7 +703,7 @@ struct inode *ovl_get_inode(struct dentr
 		ovl_set_flag(OVL_IMPURE, inode);
 
 	/* Check for non-merge dir that may have whiteouts */
-	if (S_ISDIR(realinode->i_mode)) {
+	if (is_dir) {
 		struct ovl_entry *oe = dentry->d_fsdata;
 
 		if (((upperdentry && lowerdentry) || oe->numlower > 1) ||