diff mbox series

[v5,4/4] ceph: Remove S_ISGID clear code in ceph_finish_async_create

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

Commit Message

Yang Xu April 21, 2022, 7:54 a.m. UTC
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(-)

Comments

Christian Brauner April 21, 2022, 8:18 a.m. UTC | #1
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>
Yang Xu April 21, 2022, 8:28 a.m. UTC | #2
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>
Xiubo Li April 21, 2022, 8:33 a.m. UTC | #3
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>
Christian Brauner April 21, 2022, 8:36 a.m. UTC | #4
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 mbox series

Patch

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()));
 	}