ACK/Cmnt: [PATCH v1][SRU][Disco] UBUNTU: SAUCE: overlayfs: allow with shiftfs as underlay

[Christian Brauner]

On Wed, Oct 02, 2019 at 10:48:51AM +0200, Stefan Bader wrote:
> On 02.10.19 09:44, 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>
> Acked-by: Stefan Bader <stefan.bader at canonical.com>
> > ---
> > /* v1 */
> > unchanged
> > 
> > /* v0 */
> > Link: https://lists.ubuntu.com/archives/kernel-team/2019-October/104183.html
> 
> Though the kernel is in C, most people start with 1 when thinking about patch
> submissions. ;)

I usually start to count from 0 but I had people express different
opinions. :)

Christian