ACK/CMNT: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: use translated ids when chaning lower fs attrs

[Seth Forshee]

On Thu, Apr 11, 2019 at 09:27:18AM -0500, Tyler Hicks wrote:
> [+Christian]
> 
> On 2019-04-11 09:04:24, Seth Forshee wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1824350
> > 
> > shiftfs_setattr() is preparing a new set of attributes with the
> > owner translated for the lower fs, but it then passes the
> > original attrs. As a result the owner is set to the untranslated
> > owner, which causes the shiftfs inodes to also have incorrect
> > ids. For example:
> > 
> >  # mkdir dir
> >  # touch file
> >  # ls -lh dir file
> >  drwxr-xr-x 2 root root 4.0K Apr 11 13:05 dir
> >  -rw-r--r-- 1 root root 0 Apr 11 13:05 file
> >  # chown 500:500 dir file
> >  # ls -lh dir file
> >  drwxr-xr-x 2 1000500 1000500 4.0K Apr 11 12:42 dir
> >  -rw-r--r-- 1 1000500 1000500 0 Apr 11 12:42 file
> > 
> > Fix this to pass the correct iattr struct to notify_change().
> > 
> > Reviewed-by: Christian Brauner <christian.brauner at ubuntu.com>
> > Signed-off-by: Seth Forshee <seth.forshee at canonical.com>
> 
> Acked-by: Tyler Hicks <tyhicks at canonical.com>
> 
> 
> I took another look at shiftfs_setattr(). Doesn't ia_file need to be
> translated to a lower file when (newattr.ia_valid & ATTR_FILE) is
> true? Otherwise, we're giving a file with shiftfs_{file,dir}_operations
> to the lower filesystem's setattr operation.

That's not something I'm familiar with, but looking at it now I think
you are right. If I'm understanding things correctly it looks like this
is only really an issue for network filesystems and possibly aufs, which
I think shiftfs likely doesn't support well as an underlay in any case.
But still it's something we should fix, though I think it can wait for a
post-release SRU.

Thanks,
Seth