Re: [PATCH v1 2/3] vfs: strip file's S_ISGID mode on vfs instead of on filesystem

[Christian Brauner <brauner@kernel.org>]

On Mon, Mar 28, 2022 at 05:56:28PM +0800, Yang Xu wrote:
> Currently, vfs only passes mode argument to filesystem, then use inode_init_owner()
> to strip S_ISGID. Some filesystem(ie ext4/btrfs) will call inode_init_owner
> firstly, then posxi acl setup, but xfs uses the contrary order. It will affect
> S_ISGID clear especially umask with S_IXGRP.
> 
> Vfs has all the info it needs - it doesn't need the filesystems to do everything
> correctly with the mode and ensuring that they order things like posix acl setup
> functions correctly with inode_init_owner() to strip the SGID bit.
> 
> Just strip the SGID bit at the VFS, and then the filesystems can't get it wrong.
> 
> Also, the inode_sgid_strip() api should be used before IS_POSIXACL() because
> this api may change mode by using umask but S_ISGID clear isn't related to
> SB_POSIXACL flag.
> 
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

I think adding that helper and using it in the vfs already is a good
idea. But I wonder whether leaving this in inode_init_owner() might be
desirable as well. I don't know how likely it is but if any filesystem
is somehow internally creating a new inode without using vfs_*() helpers
and botches the job then inode_init_owner() would still correctly strip
the setgid bit currently for them.

If we think it's a rather low risk then we can simply move the
strippping completely out of inode_init_owner(). If we think that that's
too risky it might be worth adding a new inode_owner() helper that is
called from inode_init_owner() and that filesystem can be switched to
that we know are safe in that regard?