Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 18 Apr 2022 04:08:51 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Christian Brauner <brauner@kernel.org>
Cc:     Yang Xu <xuyang2018.jy@fujitsu.com>, david@fromorbit.com,
        djwong@kernel.org, linux-fsdevel@vger.kernel.org,
        ceph-devel@vger.kernel.org, linux-nfs@vger.kernel.org,
        linux-xfs@vger.kernel.org, viro@zeniv.linux.org.uk,
        jlayton@kernel.org
Subject: Re: [PATCH v3 1/7] fs/inode: move sgid strip operation from
 inode_init_owner into inode_sgid_strip
Message-ID: <YlzWQyF5e14/UVDs@casper.infradead.org>
References: <1650020543-24908-1-git-send-email-xuyang2018.jy@fujitsu.com>
 <20220415140924.oirar6dklelujnxs@wittgenstein>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220415140924.oirar6dklelujnxs@wittgenstein>
Precedence: bulk

On Fri, Apr 15, 2022 at 04:09:24PM +0200, Christian Brauner wrote:
> > +			inode_sgid_strip(mnt_userns, dir, &mode);
> >  	} else
> >  		inode_fsgid_set(inode, mnt_userns);
> >  	inode->i_mode = mode;
> > @@ -2405,3 +2403,21 @@ struct timespec64 current_time(struct inode *inode)
> >  	return timestamp_truncate(now, inode);
> >  }
> >  EXPORT_SYMBOL(current_time);
> > +
> > +void inode_sgid_strip(struct user_namespace *mnt_userns,
> > +		      const struct inode *dir, umode_t *mode)
> > +{
> > +	if (!dir || !(dir->i_mode & S_ISGID))
> > +		return;
> > +	if ((*mode & (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP))
> > +		return;
> > +	if (S_ISDIR(*mode))
> > +		return;
> 
> I'd place that check first as this whole function is really only
> relevant for non-directories.
> 
> Otherwise I can live with *mode being a pointer although I still find
> this unpleasant API wise but the bikeshed does it's job without having
> my color. :)

No, I think your instincts are correct.  This should be

umode_t inode_sgid_strip(struct user_namespace *mnt_userns,
		const struct inode *dir, umode_t mode)
{
	if (S_ISDIR(mode) || !dir || !(dir->i_mode & S_ISGID))
		return mode;
	if (mode & (S_ISGID | S_IXGRP) != (S_ISGID | S_IXGRP))
		return mode;
...

and the same for prepare_mode().

And really, I think this should be called inode_strip_sgid().  Right?