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@xxxxxxxxxx>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@xxxxxxxxxx>
>> Signed-off-by: Yang Xu<xuyang2018.jy@xxxxxxxxxxx>
>> ---
>
> 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.
