diff mbox series

[v6,1/4] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

Message ID 1650856181-21350-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v6,1/4] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip | expand

Commit Message

Yang Xu (Fujitsu) April 25, 2022, 3:09 a.m. UTC 
This has no functional change. Just create and export inode_sgid_strip
api for the subsequent patch. This function is used to strip inode's
S_ISGID mode when init a new inode.

Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 fs/inode.c         | 37 +++++++++++++++++++++++++++++++++----
 include/linux/fs.h |  2 ++
 2 files changed, 35 insertions(+), 4 deletions(-)

Comments

Matthew Wilcox April 25, 2022, 2:45 a.m. UTC | #1 
On Mon, Apr 25, 2022 at 11:09:38AM +0800, Yang Xu wrote:
> This has no functional change. Just create and export inode_sgid_strip
> api for the subsequent patch. This function is used to strip inode's
> S_ISGID mode when init a new inode.

Why would you call this inode_sgid_strip() instead of
inode_strip_sgid()?
Yang Xu (Fujitsu) April 25, 2022, 3:08 a.m. UTC | #2 
on 2022/4/25 10:45, Matthew Wilcox wrote:
> On Mon, Apr 25, 2022 at 11:09:38AM +0800, Yang Xu wrote:
>> This has no functional change. Just create and export inode_sgid_strip
>> api for the subsequent patch. This function is used to strip inode's
>> S_ISGID mode when init a new inode.
>
> Why would you call this inode_sgid_strip() instead of
> inode_strip_sgid()?

Because I treated "inode sgid(inode's sgid)" as a whole.

inode_strip_sgid sounds also ok, but now seems strip_inode_sgid seem 
more clear because we strip inode sgid depend on not only inode's 
condition but also depend on parent directory's condition.

What do you think about this?

ps: I can aceept the above several way, so if you insist, I can change 
it to inode_strip_sgid.
Christian Brauner April 25, 2022, 11:29 a.m. UTC | #3 
On Mon, Apr 25, 2022 at 03:08:36AM +0000, xuyang2018.jy@fujitsu.com wrote:
> on 2022/4/25 10:45, Matthew Wilcox wrote:
> > On Mon, Apr 25, 2022 at 11:09:38AM +0800, Yang Xu wrote:
> >> This has no functional change. Just create and export inode_sgid_strip
> >> api for the subsequent patch. This function is used to strip inode's
> >> S_ISGID mode when init a new inode.
> >
> > Why would you call this inode_sgid_strip() instead of
> > inode_strip_sgid()?
> 
> Because I treated "inode sgid(inode's sgid)" as a whole.
> 
> inode_strip_sgid sounds also ok, but now seems strip_inode_sgid seem 
> more clear because we strip inode sgid depend on not only inode's 
> condition but also depend on parent directory's condition.
> 
> What do you think about this?
> 
> ps: I can aceept the above several way, so if you insist, I can change 
> it to inode_strip_sgid.

I agree with Willy. I think inode_strip_sgid() is better. It'll be in
good company as <object>_<verb>_<what?> is pretty common:

inode_update_atime()
inode_init_once()
inode_init_owner()
inode_init_early()
inode_add_lru()
inode_needs_sync()
inode_set_flags()

Maybe mode_remove_sgid() is even better because it makes it clear that
the change happens to @mode and not @dir. But I'm fine with
inode_strip_sgid() or inode_remove_sgid() too.
Darrick J. Wong April 25, 2022, 4:53 p.m. UTC | #4 
On Mon, Apr 25, 2022 at 11:09:38AM +0800, Yang Xu wrote:
> This has no functional change. Just create and export inode_sgid_strip
> api for the subsequent patch. This function is used to strip inode's
> S_ISGID mode when init a new inode.
> 
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>

I've a slight preference for inode_strip_sgid() as well, but otherwise
this looks like a reasonable refactoring.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/inode.c         | 37 +++++++++++++++++++++++++++++++++----
>  include/linux/fs.h |  2 ++
>  2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d9b422504d1..78e7ef567e04 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>  		/* Directories are special, and always inherit S_ISGID */
>  		if (S_ISDIR(mode))
>  			mode |= S_ISGID;
> -		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> -			mode &= ~S_ISGID;
> +		else
> +			mode = inode_sgid_strip(mnt_userns, dir, mode);
>  	} else
>  		inode_fsgid_set(inode, mnt_userns);
>  	inode->i_mode = mode;
> @@ -2405,3 +2403,34 @@ struct timespec64 current_time(struct inode *inode)
>  	return timestamp_truncate(now, inode);
>  }
>  EXPORT_SYMBOL(current_time);
> +
> +/**
> + * inode_sgid_strip - handle the sgid bit for non-directories
> + * @mnt_userns: User namespace of the mount the inode was created from
> + * @dir: parent directory inode
> + * @mode: mode of the file to be created in @dir
> + *
> + * If the @mode of the new file has both the S_ISGID and S_IXGRP bit
> + * raised and @dir has the S_ISGID bit raised ensure that the caller is
> + * either in the group of the parent directory or they have CAP_FSETID
> + * in their user namespace and are privileged over the parent directory.
> + * In all other cases, strip the S_ISGID bit from @mode.
> + *
> + * Return: the new mode to use for the file
> + */
> +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;
> +	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
> +		return mode;
> +	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> +		return mode;
> +
> +	mode &= ~S_ISGID;
> +	return mode;
> +}
> +EXPORT_SYMBOL(inode_sgid_strip);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bbde95387a23..532de76c9b91 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1897,6 +1897,8 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
>  void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>  		      const struct inode *dir, umode_t mode);
>  extern bool may_open_dev(const struct path *path);
> +umode_t inode_sgid_strip(struct user_namespace *mnt_userns,
> +			 const struct inode *dir, umode_t mode);
>  
>  /*
>   * This is the "filldir" function type, used by readdir() to let
> -- 
> 2.27.0
>
Matthew Wilcox April 25, 2022, 5:33 p.m. UTC | #5 
On Mon, Apr 25, 2022 at 01:29:47PM +0200, Christian Brauner wrote:
> On Mon, Apr 25, 2022 at 03:08:36AM +0000, xuyang2018.jy@fujitsu.com wrote:
> > on 2022/4/25 10:45, Matthew Wilcox wrote:
> > > On Mon, Apr 25, 2022 at 11:09:38AM +0800, Yang Xu wrote:
> > >> This has no functional change. Just create and export inode_sgid_strip
> > >> api for the subsequent patch. This function is used to strip inode's
> > >> S_ISGID mode when init a new inode.
> > >
> > > Why would you call this inode_sgid_strip() instead of
> > > inode_strip_sgid()?
> > 
> > Because I treated "inode sgid(inode's sgid)" as a whole.
> > 
> > inode_strip_sgid sounds also ok, but now seems strip_inode_sgid seem 
> > more clear because we strip inode sgid depend on not only inode's 
> > condition but also depend on parent directory's condition.
> > 
> > What do you think about this?
> > 
> > ps: I can aceept the above several way, so if you insist, I can change 
> > it to inode_strip_sgid.
> 
> I agree with Willy. I think inode_strip_sgid() is better. It'll be in
> good company as <object>_<verb>_<what?> is pretty common:
> 
> inode_update_atime()
> inode_init_once()
> inode_init_owner()
> inode_init_early()
> inode_add_lru()
> inode_needs_sync()
> inode_set_flags()
> 
> Maybe mode_remove_sgid() is even better because it makes it clear that
> the change happens to @mode and not @dir. But I'm fine with
> inode_strip_sgid() or inode_remove_sgid() too.

Oh!  Yes, mode_strip_sgid() is better.  We're operating on the mode,
not the inode.
Yang Xu (Fujitsu) April 26, 2022, 1:22 a.m. UTC | #6 
on 2022/4/26 1:33, Matthew Wilcox wrote:
> On Mon, Apr 25, 2022 at 01:29:47PM +0200, Christian Brauner wrote:
>> On Mon, Apr 25, 2022 at 03:08:36AM +0000, xuyang2018.jy@fujitsu.com wrote:
>>> on 2022/4/25 10:45, Matthew Wilcox wrote:
>>>> On Mon, Apr 25, 2022 at 11:09:38AM +0800, Yang Xu wrote:
>>>>> This has no functional change. Just create and export inode_sgid_strip
>>>>> api for the subsequent patch. This function is used to strip inode's
>>>>> S_ISGID mode when init a new inode.
>>>>
>>>> Why would you call this inode_sgid_strip() instead of
>>>> inode_strip_sgid()?
>>>
>>> Because I treated "inode sgid(inode's sgid)" as a whole.
>>>
>>> inode_strip_sgid sounds also ok, but now seems strip_inode_sgid seem
>>> more clear because we strip inode sgid depend on not only inode's
>>> condition but also depend on parent directory's condition.
>>>
>>> What do you think about this?
>>>
>>> ps: I can aceept the above several way, so if you insist, I can change
>>> it to inode_strip_sgid.
>>
>> I agree with Willy. I think inode_strip_sgid() is better. It'll be in
>> good company as<object>_<verb>_<what?>  is pretty common:
>>
>> inode_update_atime()
>> inode_init_once()
>> inode_init_owner()
>> inode_init_early()
>> inode_add_lru()
>> inode_needs_sync()
>> inode_set_flags()
>>
>> Maybe mode_remove_sgid() is even better because it makes it clear that
>> the change happens to @mode and not @dir. But I'm fine with
>> inode_strip_sgid() or inode_remove_sgid() too.
>
> Oh!  Yes, mode_strip_sgid() is better.  We're operating on the mode,
> not the inode.

OK, I will use mode_strip_sgid().
diff mbox series

Patch

diff --git a/fs/inode.c b/fs/inode.c
index 9d9b422504d1..78e7ef567e04 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2246,10 +2246,8 @@  void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		/* Directories are special, and always inherit S_ISGID */
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
-		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
-			 !in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
-			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
-			mode &= ~S_ISGID;
+		else
+			mode = inode_sgid_strip(mnt_userns, dir, mode);
 	} else
 		inode_fsgid_set(inode, mnt_userns);
 	inode->i_mode = mode;
@@ -2405,3 +2403,34 @@  struct timespec64 current_time(struct inode *inode)
 	return timestamp_truncate(now, inode);
 }
 EXPORT_SYMBOL(current_time);
+
+/**
+ * inode_sgid_strip - handle the sgid bit for non-directories
+ * @mnt_userns: User namespace of the mount the inode was created from
+ * @dir: parent directory inode
+ * @mode: mode of the file to be created in @dir
+ *
+ * If the @mode of the new file has both the S_ISGID and S_IXGRP bit
+ * raised and @dir has the S_ISGID bit raised ensure that the caller is
+ * either in the group of the parent directory or they have CAP_FSETID
+ * in their user namespace and are privileged over the parent directory.
+ * In all other cases, strip the S_ISGID bit from @mode.
+ *
+ * Return: the new mode to use for the file
+ */
+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;
+	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
+		return mode;
+	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
+		return mode;
+
+	mode &= ~S_ISGID;
+	return mode;
+}
+EXPORT_SYMBOL(inode_sgid_strip);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbde95387a23..532de76c9b91 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1897,6 +1897,8 @@  extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
 void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
 		      const struct inode *dir, umode_t mode);
 extern bool may_open_dev(const struct path *path);
+umode_t inode_sgid_strip(struct user_namespace *mnt_userns,
+			 const struct inode *dir, umode_t mode);
 
 /*
  * This is the "filldir" function type, used by readdir() to let