Re: ovl: hash non-dir by lower inode for fsnotify

[Mark Salyzyn]

On 08/01/2018 11:05 PM, Greg KH wrote:
> On Wed, Aug 01, 2018 at 02:29:01PM -0700, Mark Salyzyn wrote:
> > 764baba80168ad3adafb521d2ab483ccbc49e344 ovl: hash non-dir by lower inode
> > for fsnotify is not part of 4.14 stable and yet it was marked for 4.13
> > stable merge when committed.
> >
> > Please evaluate.
> Why not try applying it yourself to 4.14.y and note that it does not
> apply at all and then provide a working backport so that we can skip at
> least one email cycle here?  :)
>
> thanks,
>
> greg k-h

Because I am embarrassed by the backport (!) perhaps? :-)

+linux-kernel list and authors/approvers for clearance.

I took some liberty with sb = dentry_d_sb and then sprinkled it in, 
upstream passes sb to the function and the conflicts assumed so.

--------------------------> snip <-------------------------

From 764baba80168ad3adafb521d2ab483ccbc49e344 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Sun, 4 Feb 2018 15:35:09 +0200
Subject: ovl: hash non-dir by lower inode for fsnotify

(cherry pick from commit 764baba80168ad3adafb521d2ab483ccbc49e344)

Commit 31747eda41ef ("ovl: hash directory inodes for fsnotify")
fixed an issue of inotify watch on directory that stops getting
events after dropping dentry caches.

A similar issue exists for non-dir non-upper files, for example:

$ mkdir -p lower upper work merged
$ touch lower/foo
$ mount -t overlay -o
lowerdir=lower,workdir=work,upperdir=upper none merged
$ inotifywait merged/foo &
$ echo 2 > /proc/sys/vm/drop_caches
$ cat merged/foo

inotifywait doesn't get the OPEN event, because ovl_lookup() called
from 'cat' allocates a new overlay inode and does not reuse the
watched inode.

Fix this by hashing non-dir overlay inodes by lower real inode in
the following cases that were not hashed before this change:
Â- A non-upper overlay mount
Â- A lower non-hardlink when index=off

A helper ovl_hash_bylower() was added to put all the logic and
documentation about which real inode an overlay inode is hashed by
into one place.

The issue dates back to initial version of overlayfs, but this
patch depends on ovl_inode code that was introduced in kernel v4.13.

Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx> #v4.13
Signed-off-by: Mark Salyzyn <salyzyn@xxxxxxxxxxx> #v4.14
---
Âfs/overlayfs/inode.c | 62 +++++++++++++++++++++++++++++++-------------
Â1 file changed, 44 insertions(+), 18 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 28a320464609a..7cfef4152e9a4 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -14,6 +14,7 @@
Â#include <linux/posix_acl.h>
Â#include <linux/ratelimit.h>
Â#include "overlayfs.h"
+#include "ovl_entry.h"

Âint ovl_setattr(struct dentry *dentry, struct iattr *attr)
Â{
@@ -608,39 +609,63 @@ static bool ovl_verify_inode(struct inode *inode, 
struct dentry *lowerdentry,
ÂÂÂÂ return true;
Â}

+/*
+ * Does overlay inode need to be hashed by lower inode?
+ */
+static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
+ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂÂ struct dentry *lower, struct dentry *index)
+{
+ÂÂÂ struct ovl_fs *ofs = sb->s_fs_info;
+
+ÂÂÂ /* No, if pure upper */
+ÂÂÂ if (!lower)
+ÂÂÂ ÂÂÂ return false;
+
+ÂÂÂ /* Yes, if already indexed */
+ÂÂÂ if (index)
+ÂÂÂ ÂÂÂ return true;
+
+ÂÂÂ /* Yes, if won't be copied up */
+ÂÂÂ if (!ofs->upper_mnt)
+ÂÂÂ ÂÂÂ return true;
+
+ÂÂÂ /* No, if lower hardlink is or will be broken on copy up */
+ÂÂÂ if ((upper || !ovl_indexdir(sb)) &&
+ÂÂÂ ÂÂÂ !d_is_dir(lower) && d_inode(lower)->i_nlink > 1)
+ÂÂÂ ÂÂÂ return false;
+
+ÂÂÂ /* No, if non-indexed upper with NFS export */
+ÂÂÂ if (sb->s_export_op && upper)
+ÂÂÂ ÂÂÂ return false;
+
+ÂÂÂ /* Otherwise, hash by lower inode for fsnotify */
+ÂÂÂ return true;
+}
+
Âstruct inode *ovl_get_inode(struct dentry *dentry, struct dentry 
*upperdentry,
ÂÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ struct dentry *index)
Â{
+ÂÂÂ struct super_block *sb = dentry->d_sb;
ÂÂÂÂ struct dentry *lowerdentry = ovl_dentry_lower(dentry);
ÂÂÂÂ struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
ÂÂÂÂ struct inode *inode;
-ÂÂÂ /* 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 bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry, index);
ÂÂÂÂ bool is_dir;

-ÂÂÂ if (WARN_ON(upperdentry && indexed && !lowerdentry))
-ÂÂÂ ÂÂÂ return ERR_PTR(-EIO);
-
ÂÂÂÂ if (!realinode)
ÂÂÂÂ ÂÂÂ realinode = d_inode(lowerdentry);

ÂÂÂÂ /*
-ÂÂÂ Â* Copy up origin (lower) may exist for non-indexed non-dir upper, but
-ÂÂÂ Â* we must not use lower as hash key in that case.
-ÂÂÂ Â* Hash non-dir that is or could be indexed by origin inode.
-ÂÂÂ Â* Hash dir that is or could be merged by origin inode.
-ÂÂÂ Â* Hash pure upper and non-indexed non-dir by upper inode.
+ÂÂÂ Â* Copy up origin (lower) may exist for non-indexed upper, but we must
+ÂÂÂ Â* not use lower as hash key if this is a broken hardlink.
ÂÂÂÂ Â*/
ÂÂÂÂ is_dir = S_ISDIR(realinode->i_mode);
-ÂÂÂ if (is_dir)
-ÂÂÂ ÂÂÂ origin = lowerdentry;
-
-ÂÂÂ if (upperdentry || origin) {
-ÂÂÂ ÂÂÂ struct inode *key = d_inode(origin ?: upperdentry);
+ÂÂÂ if (upperdentry || bylower) {
+ÂÂÂ ÂÂÂ struct inode *key = d_inode(bylower ? lowerdentry :
+ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂÂÂ upperdentry);
ÂÂÂÂ ÂÂÂ unsigned int nlink = is_dir ? 1 : realinode->i_nlink;

-ÂÂÂ ÂÂÂ inode = iget5_locked(dentry->d_sb, (unsigned long) key,
+ÂÂÂ ÂÂÂ inode = iget5_locked(sb, (unsigned long) key,
ÂÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂÂ ovl_inode_test, ovl_inode_set, key);
ÂÂÂÂ ÂÂÂ if (!inode)
ÂÂÂÂ ÂÂÂ ÂÂÂ goto out_nomem;
@@ -664,7 +689,8 @@ struct inode *ovl_get_inode(struct dentry *dentry, 
struct dentry *upperdentry,
ÂÂÂÂ ÂÂÂ ÂÂÂ nlink = ovl_get_nlink(lowerdentry, upperdentry, nlink);
ÂÂÂÂ ÂÂÂ set_nlink(inode, nlink);
ÂÂÂÂ } else {
-ÂÂÂ ÂÂÂ inode = new_inode(dentry->d_sb);
+ÂÂÂ ÂÂÂ /* Lower hardlink that will be broken on copy up */
+ÂÂÂ ÂÂÂ inode = new_inode(sb);
ÂÂÂÂ ÂÂÂ if (!inode)
ÂÂÂÂ ÂÂÂ ÂÂÂ goto out_nomem;
ÂÂÂÂ }
--
2.18.0.597.ga71716f1ad-goog