Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v1] fs: Add kfuncs to handle idmapped mounts
To:     Alexey Gladkov <legion@kernel.org>
Cc:     bpf@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Christian Brauner <brauner@kernel.org>
References: <c35fbb4cb0a3a9b4653f9a032698469d94ca6e9c.1688123230.git.legion@kernel.org>
From:   Hou Tao <houtao@huaweicloud.com>
Message-ID: <babdf7a8-9663-6d71-821a-34da2aff80e2@huaweicloud.com>
Date:   Tue, 4 Jul 2023 19:42:53 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <c35fbb4cb0a3a9b4653f9a032698469d94ca6e9c.1688123230.git.legion@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

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@kernel.org>
> ---
>  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 ?
> +
> +/**
> + * bpf_inode_into_vfs_ids - map an inode's i_uid and i_gid down according to an idmapping
> + * @idmap: idmap of the mount the inode was found from
> + * @inode: inode to map
> + *
> + * The inode's i_uid and i_gid mapped down according to @idmap. If the inode's
> + * i_uid or i_gid has no mapping INVALID_VFSUID or INVALID_VFSGID is returned in
> + * the corresponding position.
> + *
> + * Return: A 64-bit integer containing the current GID and UID, and created as
> + * such: *gid* **<< 32 \|** *uid*.
> + */
> +__bpf_kfunc uint64_t bpf_inode_into_vfs_ids(struct mnt_idmap *idmap,
> +		const struct inode *inode)
> +{
> +	vfsuid_t vfsuid = i_uid_into_vfsuid(idmap, inode);
> +	vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> +
> +	return (u64) __vfsgid_val(vfsgid) << 32 |
> +		     __vfsuid_val(vfsuid);
> +}
> +
> +__diag_pop();
> +
> +BTF_SET8_START(idmap_btf_ids)
> +BTF_ID_FLAGS(func, bpf_is_idmapped_mnt)
> +BTF_ID_FLAGS(func, bpf_file_mnt_idmap)
> +BTF_ID_FLAGS(func, bpf_inode_into_vfs_ids)
> +BTF_SET8_END(idmap_btf_ids)
> +
> +static const struct btf_kfunc_id_set idmap_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set   = &idmap_btf_ids,
> +};
> +
> +static int __init bpf_idmap_kfunc_init(void)
> +{
> +	return register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC, &idmap_kfunc_set);
> +}
> +
Is BPF_PROG_TYPE_TRACING sufficient for your use case ? It seems
BPF_PROG_TYPE_UNSPEC will make these kfuncs be available for all bpf
program types.
> +late_initcall(bpf_idmap_kfunc_init);