NACK: Re: [PATCH 2/2][SRU][EOAN] UBUNTU: SAUCE: overlayfs: allow with shiftfs as underlay

[Christian Brauner]

On Tue, Oct 01, 2019 at 10:55:42PM +0200, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1846272
> 
> In commit [1] we enabled overlayfs on top of shiftfs. This approach was
> buggy since it let to a regression for some standard overlayfs workloads
> (cf. [2]).
> In our original approach in [1] Seth and I concluded that running
> overlayfs on top of shiftfs was not possible because of the way
> overlayfs is currently opening files. The fact that it did not pass down
> the dentry of shiftfs but rather it's own caused shiftfs to be confused
> since it stashes away necessary information in d_fsdata.
> Our solution was to modify open_with_fake_path() to also take a dentry
> as an argument, then change overlayfs to pass in the shiftfs dentry
> which then would override the dentry in the passed in struct path in
> open_with_fake_path().
> However, this led to a regression for some standard overlayfs workloads
> (cf. [2]).
> After various discussions involving Seth and myself in Paris we
> concluded the reason for the regression was that we effectively created
> a struct path that was comprised of the vfsmount of the overlayfs dentry
> and the dentry of shiftfs. This is obviously broken.
> The fix is to a) not modify open_with_fake_path() and b) change
> overlayfs to do what shiftfs is doing, namely correctly setup the struct
> path such that vfsmount and dentry match and are both from shiftfs.
> Note, that overlayfs already does this for the .open method for
> directories. It just did not do it for the .open method for regular
> files leading to this issue. The reason why this hasn't been a problem
> for overlayfs so far is that it didn't allow running on top of
> filesystems that make use of d_fsdata _implicitly_ by disallowing any
> filesystem that is itself an overlay, or has revalidate methods for it's
> dentries as those usually have d_fsdata set up. Any other filesystem
> falling in this category would have suffered from the same problem.
> 
> Seth managed to trigger the regression with the following script:
>  #!/bin/bash
> 
>  utils=(bash cat)
> 
>  mkdir -p lower/proc upper work root
>  for util in ${utils[@]}; do
>  	path="$(which $util)"
>  	dir="$(dirname $path)"
>  	mkdir -p "lower/$dir"
>  	cp -v "$path" "lower/$path"
>  	libs="$(ldd $path | egrep -o '(/usr)?/lib.*\.[0-9]')"
>  	for lib in $libs; do
>  		dir="$(dirname $lib)"
>  		mkdir -p "lower/$dir"
>  		cp -v "$lib" "lower/$lib"
>  	done
>  done
> 
>  mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work nodev root
>  mount -t proc nodev root/proc
>  chroot root bash -c "cat /proc/self/maps"
>  umount root/proc
>  umount root
> 
> With the patch here applied the regression is not reproducible.
> 
> /* References */
> [1]: 37430e430a14 ("UBUNTU: SAUCE: shiftfs: enable overlayfs on shiftfs")
> [2]: https://bugs.launchpad.net/bugs/1838677
> 
> Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>

Will resend as a separate patch so that each patch corresponds to one
bug report.

Thanks!
Christian