| Message ID | 20240229152405.105031-1-gscrivan@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hugetlbfs: support idmapped mounts | expand |
> On Feb 29, 2024, at 23:24, Giuseppe Scrivano <gscrivan@redhat.com> wrote: > > pass down the idmapped mount information to the different helper > functions. > > Differently, hugetlb_file_setup() will continue to not have any > mapping since it is only used from contexts where idmapped mounts are > not used. Sorry, could you explain more why you want this changes? What's the intention? Thanks.
Muchun Song <muchun.song@linux.dev> writes: >> On Feb 29, 2024, at 23:24, Giuseppe Scrivano <gscrivan@redhat.com> wrote: >> >> pass down the idmapped mount information to the different helper >> functions. >> >> Differently, hugetlb_file_setup() will continue to not have any >> mapping since it is only used from contexts where idmapped mounts are >> not used. > > Sorry, could you explain more why you want this changes? What's the > intention? we are adding user namespace support to Kubernetes to run each pod (a group of containers) without overlapping IDs. We need idmapped mounts for any mount shared among multiple pods. It was reported both for crun and containerd: - https://github.com/containers/crun/issues/1380 - https://github.com/containerd/containerd/issues/9585 Regards, Giuseppe
> On Mar 1, 2024, at 16:09, Giuseppe Scrivano <gscrivan@redhat.com> wrote: > > Muchun Song <muchun.song@linux.dev> writes: > >>> On Feb 29, 2024, at 23:24, Giuseppe Scrivano <gscrivan@redhat.com> wrote: >>> >>> pass down the idmapped mount information to the different helper >>> functions. >>> >>> Differently, hugetlb_file_setup() will continue to not have any >>> mapping since it is only used from contexts where idmapped mounts are >>> not used. >> >> Sorry, could you explain more why you want this changes? What's the >> intention? > > we are adding user namespace support to Kubernetes to run each > pod (a group of containers) without overlapping IDs. We need idmapped > mounts for any mount shared among multiple pods. > > It was reported both for crun and containerd: > > - https://github.com/containers/crun/issues/1380 > - https://github.com/containerd/containerd/issues/9585 It is helpful and really should go into commit log to explain why it is necessary (those information will useful for others). The changes are straightforward, but I am not familiar with Idmappings (I am not sure if there are more things to be considered). Thanks. > > Regards, > Giuseppe
On Fri, Mar 01, 2024 at 04:47:30PM +0800, Muchun Song wrote: > > > > On Mar 1, 2024, at 16:09, Giuseppe Scrivano <gscrivan@redhat.com> wrote: > > > > Muchun Song <muchun.song@linux.dev> writes: > > > >>> On Feb 29, 2024, at 23:24, Giuseppe Scrivano <gscrivan@redhat.com> wrote: > >>> > >>> pass down the idmapped mount information to the different helper > >>> functions. > >>> > >>> Differently, hugetlb_file_setup() will continue to not have any > >>> mapping since it is only used from contexts where idmapped mounts are > >>> not used. > >> > >> Sorry, could you explain more why you want this changes? What's the > >> intention? > > > > we are adding user namespace support to Kubernetes to run each > > pod (a group of containers) without overlapping IDs. We need idmapped > > mounts for any mount shared among multiple pods. > > > > It was reported both for crun and containerd: > > > > - https://github.com/containers/crun/issues/1380 > > - https://github.com/containerd/containerd/issues/9585 > > It is helpful and really should go into commit log to explain why it > is necessary (those information will useful for others). The changes > are straightforward, but I am not familiar with Idmappings (I am not > sure if there are more things to be considered). Fwiw, I've reviewed this before and it should be fine. I'll take another close look at it but last time I didn't see anything obvious that would be problematic so I'd be tempted to apply it unless there's specific objections.
On Thu, 29 Feb 2024 16:24:05 +0100, Giuseppe Scrivano wrote: > pass down the idmapped mount information to the different helper > functions. > > Differently, hugetlb_file_setup() will continue to not have any > mapping since it is only used from contexts where idmapped mounts are > not used. > > [...] Applied to the vfs.misc branch of the vfs/vfs.git tree. Patches in the vfs.misc branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.misc [1/1] hugetlbfs: support idmapped mounts https://git.kernel.org/vfs/vfs/c/91e78a1eb6b1
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c index d746866ae3b6..6502c7e776d1 100644 --- a/fs/hugetlbfs/inode.c +++ b/fs/hugetlbfs/inode.c @@ -933,7 +933,7 @@ static int hugetlbfs_setattr(struct mnt_idmap *idmap, unsigned int ia_valid = attr->ia_valid; struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode); - error = setattr_prepare(&nop_mnt_idmap, dentry, attr); + error = setattr_prepare(idmap, dentry, attr); if (error) return error; @@ -950,7 +950,7 @@ static int hugetlbfs_setattr(struct mnt_idmap *idmap, hugetlb_vmtruncate(inode, newsize); } - setattr_copy(&nop_mnt_idmap, inode, attr); + setattr_copy(idmap, inode, attr); mark_inode_dirty(inode); return 0; } @@ -985,6 +985,7 @@ static struct inode *hugetlbfs_get_root(struct super_block *sb, static struct lock_class_key hugetlbfs_i_mmap_rwsem_key; static struct inode *hugetlbfs_get_inode(struct super_block *sb, + struct mnt_idmap *idmap, struct inode *dir, umode_t mode, dev_t dev) { @@ -1006,7 +1007,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb, struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode); inode->i_ino = get_next_ino(); - inode_init_owner(&nop_mnt_idmap, inode, dir, mode); + inode_init_owner(idmap, inode, dir, mode); lockdep_set_class(&inode->i_mapping->i_mmap_rwsem, &hugetlbfs_i_mmap_rwsem_key); inode->i_mapping->a_ops = &hugetlbfs_aops; @@ -1050,7 +1051,7 @@ static int hugetlbfs_mknod(struct mnt_idmap *idmap, struct inode *dir, { struct inode *inode; - inode = hugetlbfs_get_inode(dir->i_sb, dir, mode, dev); + inode = hugetlbfs_get_inode(dir->i_sb, idmap, dir, mode, dev); if (!inode) return -ENOSPC; inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir)); @@ -1062,7 +1063,7 @@ static int hugetlbfs_mknod(struct mnt_idmap *idmap, struct inode *dir, static int hugetlbfs_mkdir(struct mnt_idmap *idmap, struct inode *dir, struct dentry *dentry, umode_t mode) { - int retval = hugetlbfs_mknod(&nop_mnt_idmap, dir, dentry, + int retval = hugetlbfs_mknod(idmap, dir, dentry, mode | S_IFDIR, 0); if (!retval) inc_nlink(dir); @@ -1073,7 +1074,7 @@ static int hugetlbfs_create(struct mnt_idmap *idmap, struct inode *dir, struct dentry *dentry, umode_t mode, bool excl) { - return hugetlbfs_mknod(&nop_mnt_idmap, dir, dentry, mode | S_IFREG, 0); + return hugetlbfs_mknod(idmap, dir, dentry, mode | S_IFREG, 0); } static int hugetlbfs_tmpfile(struct mnt_idmap *idmap, @@ -1082,7 +1083,7 @@ static int hugetlbfs_tmpfile(struct mnt_idmap *idmap, { struct inode *inode; - inode = hugetlbfs_get_inode(dir->i_sb, dir, mode | S_IFREG, 0); + inode = hugetlbfs_get_inode(dir->i_sb, idmap, dir, mode | S_IFREG, 0); if (!inode) return -ENOSPC; inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir)); @@ -1094,10 +1095,11 @@ static int hugetlbfs_symlink(struct mnt_idmap *idmap, struct inode *dir, struct dentry *dentry, const char *symname) { + const umode_t mode = S_IFLNK|S_IRWXUGO; struct inode *inode; int error = -ENOSPC; - inode = hugetlbfs_get_inode(dir->i_sb, dir, S_IFLNK|S_IRWXUGO, 0); + inode = hugetlbfs_get_inode(dir->i_sb, idmap, dir, mode, 0); if (inode) { int l = strlen(symname)+1; error = page_symlink(inode, symname, l); @@ -1566,6 +1568,7 @@ static struct file_system_type hugetlbfs_fs_type = { .init_fs_context = hugetlbfs_init_fs_context, .parameters = hugetlb_fs_parameters, .kill_sb = kill_litter_super, + .fs_flags = FS_ALLOW_IDMAP, }; static struct vfsmount *hugetlbfs_vfsmount[HUGE_MAX_HSTATE]; @@ -1619,7 +1622,9 @@ struct file *hugetlb_file_setup(const char *name, size_t size, } file = ERR_PTR(-ENOSPC); - inode = hugetlbfs_get_inode(mnt->mnt_sb, NULL, S_IFREG | S_IRWXUGO, 0); + /* hugetlbfs_vfsmount[] mounts do not use idmapped mounts. */ + inode = hugetlbfs_get_inode(mnt->mnt_sb, &nop_mnt_idmap, NULL, + S_IFREG | S_IRWXUGO, 0); if (!inode) goto out; if (creat_flags == HUGETLB_SHMFS_INODE)
pass down the idmapped mount information to the different helper functions. Differently, hugetlb_file_setup() will continue to not have any mapping since it is only used from contexts where idmapped mounts are not used. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- fs/hugetlbfs/inode.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)