Re: [PATCH] ovl: ignore lower entries when checking purity of non-directory entries

[Miklos Szeredi]

On Mon, Feb 1, 2016 at 3: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.

Thanks for the analysis and patch.  Added to the queue.

Thanks,
Miklos