On Mon, Feb 1, 2016 at 5:14 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > 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. Interesting. I haven't heard about this testsuite. Never read Documentation. =) I've send couple of patches for xfstests: this case and fixed bugs in setattr. This seems much better place for them. > > 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-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
