On Sun, Jan 31, 2016 at 04:17:53PM +0300, Konstantin Khlebnikov wrote: > After rename file dentry still holds reference to lower dentry from > previous location. This doesn't matter for data access because data > cames from upper dentry. But this stale lower dentry taints dentry > at new location and turns it into non-pure upper. Such file leaves > visible whiteout entry after remove in directory which shouldn't > have whiteouts at all. > > Overlayfs already tracks pureness of file location in oe->opaque. > This patch just uses that for detecting actual path type. > > Comment from Vivek Goyal's patch: > > Here are the details of the problem. Do following. > > $ mkdir upper lower work merged upper/dir/ > $ touch lower/test > $ sudo mount -t overlay overlay -olowerdir=lower,upperdir=upper,workdir=work > merged > $ mv merged/test merged/dir/ > $ rm merged/dir/test > $ ls -l merged/dir/ > /usr/bin/ls: cannot access merged/dir/test: No such file or directory > total 0 > c????????? ? ? ? ? ? test > > Basic problem seems to be that once a file has been unlinked, a > whiteout has been left behind which was not needed and hence it becomes > visible. > > whiteout is visible because parent dir is of not type MERGE, hence > od->is_real is set during ovl_dir_open(). And that means ovl_iterate() > passes on iterate handling directly to underlying fs. Underlying fs does > not know/filter whiteouts so it becomes visible to user. > > Why did we leave a whiteout to begin with when we should not have. > ovl_do_remove() checks for OVL_TYPE_PURE_UPPER() and does not leave > whiteout if file is pure upper. In this case file is not found to be > pure upper hence whiteout is left. > > So why file was not PURE_UPPER in this case? I think because dentry is > still carrying some leftover state which was valid before rename. For example, > od->numlower was set to 1 as it was a lower file. After rename, this state > is not valid anymore as there is no such file in lower. > > Signed-off-by: Konstantin Khlebnikov <koct9i@xxxxxxxxx> > Reported-by: Viktor Stanchev <me@xxxxxxxxxxxxxxxxxx> > Diagnosed-by: Vivek Goyal <vgoyal@xxxxxxxxxx> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=109611 Hi Konstantin, Thanks for the patch. This patch works for me also does not break unionmount-testsuite. This change also sounds reasonable to me. (given we can't free lower). So over rename we will be keeping references to dentries which we should not have to. I guess there does not seem to be a better option. Acked-by: Vivek Goyal <vgoyal@xxxxxxxxxx> Thanks Vivek > --- > fs/overlayfs/dir.c | 7 +++++++ > fs/overlayfs/super.c | 12 +++++++----- > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index ed95272d57a6..edf83f325bca 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -903,6 +903,13 @@ static int ovl_rename2(struct inode *olddir, struct dentry *old, > if (!overwrite && new_is_dir && !old_opaque && new_opaque) > ovl_remove_opaque(newdentry); > > + /* > + * Old dentry now lives in different location. Dentries in > + * lowerstack are stale. We cannot drop them here because > + * access to them is lockless. This could be only pure upper > + * or opaque directory - numlower is zero. Or upper non-dir > + * entry - its pureness is tracked by flag opaque. > + */ > if (old_opaque != new_opaque) { > ovl_dentry_set_opaque(old, new_opaque); > if (!overwrite) > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index 8d826bd56b26..ba28b007005e 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -76,12 +76,14 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry) > if (oe->__upperdentry) { > type = __OVL_PATH_UPPER; > > - if (oe->numlower) { > - if (S_ISDIR(dentry->d_inode->i_mode)) > - type |= __OVL_PATH_MERGE; > - } else if (!oe->opaque) { > + /* > + * Non-dir dentry can hold lower dentry from previous > + * location. Its purity depends only on opaque flag. > + */ > + if (oe->numlower && S_ISDIR(dentry->d_inode->i_mode)) > + type |= __OVL_PATH_MERGE; > + else if (!oe->opaque) > type |= __OVL_PATH_PURE; > - } > } else { > if (oe->numlower > 1) > type |= __OVL_PATH_MERGE; -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
