On Tue, Apr 26, 2022 at 07:39:07AM +0000, xuyang2018.jy@xxxxxxxxxxx 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@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
> > 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.
