diff mbox series

[v7,4/4] ceph: Remove S_ISGID stripping code in ceph_finish_async_create

Message ID 1650946792-9545-4-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v7,1/4] fs: move sgid stripping operation from inode_init_owner into mode_strip_sgid | expand

Commit Message

Yang Xu (Fujitsu) April 26, 2022, 4:19 a.m. UTC 
Previous patches moved sgid stripping exclusively into the vfs. So
manual sgid stripping by the filesystem isn't needed anymore.

Reviewed-by: Xiubo Li <xiubli@redhat.com>
Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/ceph/file.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Christian Brauner April 26, 2022, 7:14 a.m. UTC | #1 
On Tue, Apr 26, 2022 at 12:19:52PM +0800, Yang Xu wrote:
> Previous patches moved sgid stripping exclusively into the vfs. So
> manual sgid stripping by the filesystem isn't needed anymore.
> 
> Reviewed-by: Xiubo Li <xiubli@redhat.com>
> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

Since this is a very sensitive patch series I think we need to be
annoyingly pedantic about the commit messages. This is really only
necessary because of the nature of these changes so you'll forgive me
for being really annoying about this. Here's what I'd change the commit
messages to:

ceph: rely on vfs for setgid stripping

Now that we finished moving setgid stripping for regular files in setgid
directories into the vfs, individual filesystem don't need to manually
strip the setgid bit anymore. Drop the now unneeded code from ceph.


>  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;

(Frankly, this ideally shouldn't be necessary as well, i.e. it'd be
great if that part would've been done by the vfs already too but it's
not as security sensitive as setgid stripping for regular files.)
Yang Xu (Fujitsu) April 26, 2022, 8:43 a.m. UTC | #2 
on 2022/4/26 15:14, Christian Brauner wrote:
> On Tue, Apr 26, 2022 at 12:19:52PM +0800, Yang Xu wrote:
>> Previous patches moved sgid stripping exclusively into the vfs. So
>> manual sgid stripping by the filesystem isn't needed anymore.
>>
>> Reviewed-by: Xiubo Li<xiubli@redhat.com>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> Since this is a very sensitive patch series I think we need to be
> annoyingly pedantic about the commit messages. This is really only
> necessary because of the nature of these changes so you'll forgive me
> for being really annoying about this. Here's what I'd change the commit
> messages to:
>
> ceph: rely on vfs for setgid stripping
>
> Now that we finished moving setgid stripping for regular files in setgid
> directories into the vfs, individual filesystem don't need to manually
> strip the setgid bit anymore. Drop the now unneeded code from ceph.

This seems better, thanks.
>
>
>>   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;
>
> (Frankly, this ideally shouldn't be necessary as well, i.e. it'd be
> great if that part would've been done by the vfs already too but it's
> not as security sensitive as setgid stripping for regular files.)

Maybe we can just add mode_add_sgid api into vfs_prepare_mode in the 
future or only just add mode_add_sgid into do_mkdirat?

static inline umode_t vfs_prepare_mode(struct user_namespace *mnt_userns,
				   const struct inode *dir, umode_t mode)
{
	mode = mode_strip_sgid(mnt_userns, dir, mode);

	if (!IS_POSIXACL(dir))
		mode &= ~current_umask();

        mode = mode_add_sgid(dir, mode)

        return mode;
}

fs/inode.c
umodet mode_add_sgid(const struct inode *dir, umode_t mode)
{
	if (dir && dir->i_mode & S_ISGID && S_ISDIR(mode))
		 mode |= S_ISGID;

	return mode;		
}


Then we can remove  "mode |= S_ISGID" code in inode_init_owner after we 
check all places called inode_init_owner.

But now, let's finished S_ISGID stripping patchset firstly.
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()));
 	}