On Thu, 28 Apr 2022 at 12:57, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Thu, Apr 28, 2022 at 1:30 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > On Thu, Apr 28, 2022 at 12:10:24PM +0200, Miklos Szeredi wrote:
> > > On Thu, 7 Apr 2022 at 13:23, Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > Tested-by: Giuseppe Scrivano <gscrivan@xxxxxxxxxx>
> > > > Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> > > > ---
> > > > /* v2 */
> > > > unchanged
> > > >
> > > > /* v3 */
> > > > unchanged
> > > >
> > > > /* v4 */
> > > > - Vivek Goyal <vgoyal@xxxxxxxxxx>:
> > > > - 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?
>
> Let me explain because it's my hack ;)
>
> We need to find an ovl dentry corresponding with the dirent
> readdir result.
>
> The correct mount to use for lookup_one() is path->mnt
> (the mount of the ovl_readdir path) same as is done a few lines later
> for calling vfs_getattr().
That's a different one. Apparently the correct mount is already used
in that case.
Thanks,
Miklos
