| Message ID | 1650527658-2218-4-git-send-email-xuyang2018.jy@fujitsu.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/4] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip | expand |
On Thu, Apr 21, 2022 at 03:54:18PM +0800, Yang Xu wrote: > Since vfs has stripped S_ISGID in the previous patch, the calltrace > as below: > > vfs: lookup_open > ... > if (open_flag & O_CREAT) { > if (open_flag & O_EXCL) > open_flag &= ~O_TRUNC; > mode = prepare_mode(mnt_userns, dir->d_inode, mode); > ... > dir_inode->i_op->atomic_open > > ceph: ceph_atomic_open > ... > if (flags & O_CREAT) > ceph_finish_async_create > > We have stripped sgid in prepare_mode, so remove this useless clear > code directly. I'd replace this with: "Previous patches moved sgid stripping exclusively into the vfs. So manual sgid stripping by the filesystem isn't needed anymore." > > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > --- Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
on 2022/4/21 16:18, Christian Brauner wrote: > On Thu, Apr 21, 2022 at 03:54:18PM +0800, Yang Xu wrote: >> Since vfs has stripped S_ISGID in the previous patch, the calltrace >> as below: >> >> vfs: lookup_open >> ... >> if (open_flag& O_CREAT) { >> if (open_flag& O_EXCL) >> open_flag&= ~O_TRUNC; >> mode = prepare_mode(mnt_userns, dir->d_inode, mode); >> ... >> dir_inode->i_op->atomic_open >> >> ceph: ceph_atomic_open >> ... >> if (flags& O_CREAT) >> ceph_finish_async_create >> >> We have stripped sgid in prepare_mode, so remove this useless clear >> code directly. > > I'd replace this with: > > "Previous patches moved sgid stripping exclusively into the vfs. So > manual sgid stripping by the filesystem isn't needed anymore." Looks more clear, so should I drop the above calltrace? > >> >> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com> >> --- > > Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
On 4/21/22 3:54 PM, Yang Xu wrote: > Since vfs has stripped S_ISGID in the previous patch, the calltrace > as below: > > vfs: lookup_open > ... > if (open_flag & O_CREAT) { > if (open_flag & O_EXCL) > open_flag &= ~O_TRUNC; > mode = prepare_mode(mnt_userns, dir->d_inode, mode); > ... > dir_inode->i_op->atomic_open > > ceph: ceph_atomic_open > ... > if (flags & O_CREAT) > ceph_finish_async_create > > We have stripped sgid in prepare_mode, so remove this useless clear > code directly. > > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > --- > fs/ceph/file.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 6c9e837aa1d3..8e3b99853333 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -651,10 +651,6 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry, > /* Directories always inherit the setgid bit. */ > if (S_ISDIR(mode)) > mode |= S_ISGID; > - else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) && > - !in_group_p(dir->i_gid) && > - !capable_wrt_inode_uidgid(&init_user_ns, dir, CAP_FSETID)) > - mode &= ~S_ISGID; > } else { > in.gid = cpu_to_le32(from_kgid(&init_user_ns, current_fsgid())); > } LGTM. Reviewed-by: Xiubo Li <xiubli@redhat.com>
On Thu, Apr 21, 2022 at 08:28:12AM +0000, xuyang2018.jy@fujitsu.com wrote: > on 2022/4/21 16:18, Christian Brauner wrote: > > On Thu, Apr 21, 2022 at 03:54:18PM +0800, Yang Xu wrote: > >> Since vfs has stripped S_ISGID in the previous patch, the calltrace > >> as below: > >> > >> vfs: lookup_open > >> ... > >> if (open_flag& O_CREAT) { > >> if (open_flag& O_EXCL) > >> open_flag&= ~O_TRUNC; > >> mode = prepare_mode(mnt_userns, dir->d_inode, mode); > >> ... > >> dir_inode->i_op->atomic_open > >> > >> ceph: ceph_atomic_open > >> ... > >> if (flags& O_CREAT) > >> ceph_finish_async_create > >> > >> We have stripped sgid in prepare_mode, so remove this useless clear > >> code directly. > > > > I'd replace this with: > > > > "Previous patches moved sgid stripping exclusively into the vfs. So > > manual sgid stripping by the filesystem isn't needed anymore." > Looks more clear, so should I drop the above calltrace? Imho, yes.
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 6c9e837aa1d3..8e3b99853333 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -651,10 +651,6 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry, /* Directories always inherit the setgid bit. */ if (S_ISDIR(mode)) mode |= S_ISGID; - else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) && - !in_group_p(dir->i_gid) && - !capable_wrt_inode_uidgid(&init_user_ns, dir, CAP_FSETID)) - mode &= ~S_ISGID; } else { in.gid = cpu_to_le32(from_kgid(&init_user_ns, current_fsgid())); }
Since vfs has stripped S_ISGID in the previous patch, the calltrace as below: vfs: lookup_open ... if (open_flag & O_CREAT) { if (open_flag & O_EXCL) open_flag &= ~O_TRUNC; mode = prepare_mode(mnt_userns, dir->d_inode, mode); ... dir_inode->i_op->atomic_open ceph: ceph_atomic_open ... if (flags & O_CREAT) ceph_finish_async_create We have stripped sgid in prepare_mode, so remove this useless clear code directly. Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- fs/ceph/file.c | 4 ---- 1 file changed, 4 deletions(-)