APPLIED[U]: [SRU][K/L/Unstable][PATCH 1/1] UBUNTU: SAUCE: overlayfs: handle idmapped mounts in ovl_do_(set|remove)xattr

[Aleksandr Mikhalitsyn]

On Fri, Mar 3, 2023 at 8:08 AM Andrea Righi <andrea.righi at canonical.com> wrote:
>
> On Thu, Mar 02, 2023 at 10:23:55PM +0100, Alexander Mikhalitsyn wrote:
> > BugLink: http://bugs.launchpad.net/bugs/2009065
> >
> > We have to use ovl_upper_mnt_userns(ofs) helper to get proper user namespace
> > for idmapped layer. Otherwise we'll get -EPERM.
> >
> > Right now, overlayfs on top of idmapped layer always mounted as read-only.
> > This is serious blocker for LXD/LXC unprivileged containers users who run
> > Docker containers inside.
> >
> > Reproducer:
> > $ cd /idmapped/mount/path
> > $ mkdir {work,upper,lower,ovl}
> > $ mount -t overlay overlay -o lowerdir=lower,upperdir=upper,workdir=work ovl
> > $ touch ovl/test
> > touch: cannot touch 'ovl/test': Read-only file system
> >
> > Error from dmesg:
> > overlayfs: failed to create directory work/work (errno: 1); mounting read-only
> >
> > Reproducible on all Ubuntu kernels with the base >= 5.19
> >
> > Fixes: eea996a46f ("UBUNTU: SAUCE: overlayfs: Skip permission checking for trusted.overlayfs.* xattrs")
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn at canonical.com>
>
> It all makes sense to me, we also have a clear reproducer of the
> problem, so: applied to lunar/linux-unstable (no need to apply this in
> lunar/linux, because it'll be replaced by linux-unstable in the next
> respin). Also:

Great, thanks! What about kinetic/master-next and jammy/hwe-5.19,
those branches inherit this change somehow?

>
> Acked-by: Andrea Righi <andrea.righi at canonical.com>
>
> BTW, I'm wondering if we have similar cases with our custom overlayfs
> changes where we just use init_user_ns, instead of the proper namespace.

Yep, that's really complex stuff. If you check the actual
torvalds/linux tree fs/overlayfs
you find like 6 places where init_user_ns is used (and user properly),
sometimes it is used implicitly through "nop_mnt_idmap"
[ https://github.com/torvalds/linux/commit/256c8aed2b420a7c57ed6469fbb0f8310f5aeec9
]
So we need to check each case very carefully.

> However, if there are similar cases we should be able to catch them
> running the typical LXD/LXC test cases I guess.

Yep, probably we need to add overlayfs to LXC tests somehow to catch
issues like this.

>
> Do you think we need to add other specific overlayfs test cases to our
> regression test suite?

Right now I have no extra tests suggestions.

Kind regards,
Alex

>
> Thanks,
> -Andrea
>
> > ---
> >  fs/overlayfs/overlayfs.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index e523d600da4e..3a85be75d64a 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -255,7 +255,7 @@ static inline int ovl_do_setxattr(struct ovl_fs *ofs, struct dentry *dentry,
> >       int err;
> >
> >       inode_lock(inode);
> > -     err = __vfs_setxattr_noperm(&init_user_ns, dentry, name, value, size, flags);
> > +     err = __vfs_setxattr_noperm(ovl_upper_mnt_userns(ofs), dentry, name, value, size, flags);
> >       inode_unlock(inode);
> >
> >       pr_debug("setxattr(%pd2, \"%s\", \"%*pE\", %zu, %d) = %i\n",
> > @@ -277,7 +277,7 @@ static inline int ovl_do_removexattr(struct ovl_fs *ofs, struct dentry *dentry,
> >       int err;
> >
> >       inode_lock(inode);
> > -     err = __vfs_removexattr_noperm(&init_user_ns, dentry, name);
> > +     err = __vfs_removexattr_noperm(ovl_upper_mnt_userns(ofs), dentry, name);
> >       inode_unlock(inode);
> >
> >       pr_debug("removexattr(%pd2, \"%s\") = %i\n", dentry, name, err);
> > --
> > 2.34.1
> >
> >
> > --
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team