Re: [PATCH v7 1/4] fs: move sgid stripping operation from inode_init_owner into mode_strip_sgid

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

on 2022/4/26 16:39, Christian Brauner wrote:
> On Tue, Apr 26, 2022 at 07:39:07AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> on 2022/4/26 15:06, Christian Brauner wrote:
>>> On Tue, Apr 26, 2022 at 12:19:49PM +0800, Yang Xu wrote:
>>>> This has no functional change. Just create and export mode_strip_sgid
>>>> api for the subsequent patch. This function is used to strip S_ISGID mode
>>>> when init a new inode.
>>>>
>>>> Reviewed-by: Darrick J. Wong<djwong@kernel.org>
>>>> 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
>>> message to:
>>>
>>> fs: add mode_strip_sgid() helper
>>>
>>> Add a dedicated helper to handle the setgid bit when creating a new file
>>> in a setgid directory. This is a preparatory patch for moving setgid
>>> stripping into the vfs. The patch contains no functional changes.
>>>
>>> Currently the setgid stripping logic is open-coded directly in
>>> inode_init_owner() and the individual filesystems are responsible for
>>> handling setgid inheritance. Since this has proven to be brittle as
>>> evidenced by old issues we uncovered over the last months (see [1] to
>>> [3] below) we will try to move this logic into the vfs.
>>>
>>> Link: e014f37db1a2 ("xfs: use setattr_copy to set vfs inode attributes" [1]
>>> Link: 01ea173e103e ("xfs: fix up non-directory creation in SGID directories") [2]
>>> Link: fd84bfdddd16 ("ceph: fix up non-directory creation in SGID directories") [3]
>>
>> This seems better, thanks.
>>
>> ps: Sorry, forgive my poor ability for write this.
>
> This really isn't any comment on your ability to write this! I tried to
> make this clear but I obviously failed.
>
> It is really just that this has an associated non-zero regression risk
> and we need to make sure to highlight this and be very clear about the
> motivation for this change. So it's equal parts pedantry and trying to
> keep our own heads off the guillotine.

Understand. So do you have other comments? I plan to send a v8(based on 
5.18-rc4).