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?
