Re: [PATCH v1] fs: Add kfuncs to handle idmapped mounts

[Alexey Gladkov]

On Wed, Jul 05, 2023 at 04:18:07PM +0200, Christian Brauner wrote:
> On Wed, Jul 05, 2023 at 03:43:09PM +0200, Alexey Gladkov wrote:
> > On Tue, Jul 04, 2023 at 05:28:13PM +0200, Christian Brauner wrote:
> > > On Tue, Jul 04, 2023 at 05:11:12PM +0200, Alexey Gladkov wrote:
> > > > On Tue, Jul 04, 2023 at 07:42:53PM +0800, Hou Tao wrote:
> > > > > Hi,
> > > > > 
> > > > > On 6/30/2023 7:08 PM, Alexey Gladkov wrote:
> > > > > > Since the introduction of idmapped mounts, file handling has become
> > > > > > somewhat more complicated. If the inode has been found through an
> > > > > > idmapped mount the idmap of the vfsmount must be used to get proper
> > > > > > i_uid / i_gid. This is important, for example, to correctly take into
> > > > > > account idmapped files when caching, LSM or for an audit.
> > > > > 
> > > > > Could you please add a bpf selftest for these newly added kfuncs ?
> > > > > >
> > > > > > Signed-off-by: Alexey Gladkov <legion@xxxxxxxxxx>
> > > > > > ---
> > > > > >  fs/mnt_idmapping.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 69 insertions(+)
> > > > > >
> > > > > > diff --git a/fs/mnt_idmapping.c b/fs/mnt_idmapping.c
> > > > > > index 4905665c47d0..ba98ce26b883 100644
> > > > > > --- a/fs/mnt_idmapping.c
> > > > > > +++ b/fs/mnt_idmapping.c
> > > > > > @@ -6,6 +6,7 @@
> > > > > >  #include <linux/mnt_idmapping.h>
> > > > > >  #include <linux/slab.h>
> > > > > >  #include <linux/user_namespace.h>
> > > > > > +#include <linux/bpf.h>
> > > > > >  
> > > > > >  #include "internal.h"
> > > > > >  
> > > > > > @@ -271,3 +272,71 @@ void mnt_idmap_put(struct mnt_idmap *idmap)
> > > > > >  		kfree(idmap);
> > > > > >  	}
> > > > > >  }
> > > > > > +
> > > > > > +__diag_push();
> > > > > > +__diag_ignore_all("-Wmissing-prototypes",
> > > > > > +		  "Global functions as their definitions will be in vmlinux BTF");
> > > > > > +
> > > > > > +/**
> > > > > > + * bpf_is_idmapped_mnt - check whether a mount is idmapped
> > > > > > + * @mnt: the mount to check
> > > > > > + *
> > > > > > + * Return: true if mount is mapped, false if not.
> > > > > > + */
> > > > > > +__bpf_kfunc bool bpf_is_idmapped_mnt(struct vfsmount *mnt)
> > > > > > +{
> > > > > > +	return is_idmapped_mnt(mnt);
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * bpf_file_mnt_idmap - get file idmapping
> > > > > > + * @file: the file from which to get mapping
> > > > > > + *
> > > > > > + * Return: The idmap for the @file.
> > > > > > + */
> > > > > > +__bpf_kfunc struct mnt_idmap *bpf_file_mnt_idmap(struct file *file)
> > > > > > +{
> > > > > > +	return file_mnt_idmap(file);
> > > > > > +}
> > > > > 
> > > > > A dummy question here: the implementation of file_mnt_idmap() is
> > > > > file->f_path.mnt->mnt_idmap, so if the passed file is a BTF pointer, is
> > > > > there any reason why we could not do such dereference directly in bpf
> > > > > program ?
> > > > 
> > > > I wanted to provide a minimal API for bpf programs. I thought that this
> > > > interface is stable enough, but after reading Christian's answer, it looks
> > > > like I was wrong.
> > > 
> > > It isn't even about stability per se. It's unlikely that if we change
> > > internal details that types or arguments to these helpers change. That's
> > > why we did the work of abstracting this all away in the first place and
> > > making this an opaque type.
> > > 
> > > The wider point is that according to the docs, kfuncs claim to have
> > > equivalent status to EXPORT_SYMBOL_*() with the added complexity of
> > > maybe having to take out of tree bpf programs into account.
> > > 
> > > Right now, we can look at the in-kernel users of is_idmapped_mnt(),
> > > convert them and then kill this thing off if we wanted to. As soon as
> > > this is a kfunc such an endeavour becomes a measure of "f**** around and
> > > find out". That's an entirely avoidable conflict if we don't even expose
> > > it in the first place.
> > > 
> > 
> > I was hoping to make it possible to use is_idmapped_mnt or its equivalent
> > to at least be able to distinguish a file with an idmapped mount from a
> > regular one.
> 
> Afaict, you can do this today pretty easily. For example,
> 
> #!/usr/bin/env bpftrace
> 
> #include <linux/mount.h>
> #include <linux/path.h>
> #include <linux/dcache.h>
> 
> kfunc:do_move_mount
> {
>         printf("Target path           %s\n", str(args->new_path->dentry->d_name.name));
>         printf("Target mount idmapped %d\n", args->new_path->mnt->mnt_idmap != kaddr("nop_mnt_idmap"));
> }
> 
> sample output:
> 
> Target path           console
> Target mount idmapped 0
> Target path           rootfs
> Target mount idmapped 1
> 

Well, it's a possible solution, but in this case we don't limit the bpf
programs in hacking. But on the other hand it will be only their problem.

Since this is the current strategy, this is suitable for me.

-- 
Rgrds, legion