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@xxxxxxxxxxxxx> > Signed-off-by: Yang Xu <xuyang2018.jy@xxxxxxxxxxx> > --- 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?
