Re: [PATCH v5 13/19] ovl: handle idmappings for layer lookup

[Christian Brauner <brauner@kernel.org>]

On Thu, Apr 28, 2022 at 12:10:24PM +0200, Miklos Szeredi wrote:
> On Thu, 7 Apr 2022 at 13:23, Christian Brauner <brauner@kernel.org> wrote:
> >
> > Make the two places where lookup helpers can be called either on lower
> > or upper layers take the mount's idmapping into account. To this end we
> > pass down the mount in struct ovl_lookup_data. It can later also be used
> > to construct struct path for various other helpers. This is needed to
> > support idmapped base layers with overlay.
> >
> > Cc: <linux-unionfs@vger.kernel.org>
> > Tested-by: Giuseppe Scrivano <gscrivan@redhat.com>
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > ---
> > /* v2 */
> > unchanged
> >
> > /* v3 */
> > unchanged
> >
> > /* v4 */
> > - Vivek Goyal <vgoyal@redhat.com>:
> >   - s/ovl_upper_idmap()/ovl_upper_mnt_userns()/g
> >
> > /* v5 */
> > unchanged
> > ---
> >  fs/overlayfs/export.c  |  3 ++-
> >  fs/overlayfs/namei.c   | 14 ++++++++------
> >  fs/overlayfs/readdir.c | 10 +++++-----
> >  3 files changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index ebde05c9cf62..5acf353d160b 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -391,7 +391,8 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> >          * pointer because we hold no lock on the real dentry.
> >          */
> >         take_dentry_name_snapshot(&name, real);
> > -       this = lookup_one_len(name.name.name, connected, name.name.len);
> > +       this = lookup_one(mnt_user_ns(layer->mnt), name.name.name,
> > +                         connected, name.name.len);
> 
> This one is tricky.  It's doing a lookup on overlay, so messing with
> the underlying mnt_userns is definitely wrong.
> 
> Is the mnt_userns needed for permission checking?   Possibly in that
> case the permission checking needs to be skipped altogether, since
> it's doing an fh -> dentry lookup which should succeed regardless of
> caller's caps.

If capabilities are checked then it's always caller's user namespace
that is used (Ofc, exceptions being filesystem-wide operations where
capabilities in the filesystem's userns are needed but that doesn't
apply here.).

Nothing has changed with the introduction of the mnt_userns in the
area of capability checking. IOW, the mnt_userns isn't used for
capability checks as that would be a major permission model change
overall (It might have applications in the future ofc.).

The mnt_userns matters for permission checking only in so far as it is
used to map the k{g,u}ids according to the mount and so is relevant in
only that sense in inode_permission().

If this is doing a lookup on an overlay and the relevant mount isn't
supposed to be idmapped then the simple thing to do is to pass
&init_user_ns.

Could you explain what "lookup on overlay" means here, i.e. what mount
is relevant?