diff mbox series

[v2,1/3] vfs: Add inode_sgid_strip() api

Message ID 1649923039-2273-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/3] vfs: Add inode_sgid_strip() api | expand

Commit Message

Yang Xu (Fujitsu) April 14, 2022, 7:57 a.m. UTC 
inode_sgid_strip() function is used to strip S_ISGID mode
when creat/open/mknod file.

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

Comments

Christian Brauner April 14, 2022, 12:02 p.m. UTC | #1 
On Thu, Apr 14, 2022 at 03:57:17PM +0800, Yang Xu wrote:
> inode_sgid_strip() function is used to strip S_ISGID mode
> when creat/open/mknod file.
> 
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/inode.c         | 18 ++++++++++++++++++
>  include/linux/fs.h |  3 ++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d9b422504d1..d63264998855 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2405,3 +2405,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, 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;
> +	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
> +		return;
> +	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> +		return;
> +
> +	*mode &= ~S_ISGID;
> +}
> +EXPORT_SYMBOL(inode_sgid_strip);


I still think this should return umode_t with the setgid bit stripped
instead of modifying the mode directly. I may have misunderstood Dave,
but I thought he preferred to return umode_t too?

umode_t inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir, umode_t mode)
{
	if (S_ISDIR(mode))
		return mode;

	if (!dir || !(dir->i_mode & S_ISGID))
		return;

	if ((mode & (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP))
		return;

	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
		return;

	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
		return;

	return mode & ~S_ISGID;
}
Darrick J. Wong April 14, 2022, 3:57 p.m. UTC | #2 
On Thu, Apr 14, 2022 at 03:57:17PM +0800, Yang Xu wrote:
> inode_sgid_strip() function is used to strip S_ISGID mode
> when creat/open/mknod file.
> 
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/inode.c         | 18 ++++++++++++++++++
>  include/linux/fs.h |  3 ++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d9b422504d1..d63264998855 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2405,3 +2405,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, 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;
> +	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
> +		return;
> +	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> +		return;
> +
> +	*mode &= ~S_ISGID;
> +}

Thanks for cleaning up the multiple if statements from last time.

I still would like to see patch 1 replace the code in inode_init_owner
so that we can compare before and after in the same patch.  Patch 2 can
then be solely about moving the callsite around the VFS.

--D

> +EXPORT_SYMBOL(inode_sgid_strip);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bbde95387a23..94d94219fe7c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1897,7 +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);
> -
> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
> +		      umode_t *mode);
>  /*
>   * This is the "filldir" function type, used by readdir() to let
>   * the kernel specify what kind of dirent layout it wants to have.
> -- 
> 2.27.0
>
Yang Xu (Fujitsu) April 15, 2022, 1:18 a.m. UTC | #3 
on 2022/4/14 23:57, Darrick J. Wong wrote:
> On Thu, Apr 14, 2022 at 03:57:17PM +0800, Yang Xu wrote:
>> inode_sgid_strip() function is used to strip S_ISGID mode
>> when creat/open/mknod file.
>>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/inode.c         | 18 ++++++++++++++++++
>>   include/linux/fs.h |  3 ++-
>>   2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9d9b422504d1..d63264998855 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2405,3 +2405,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, 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;
>> +	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
>> +		return;
>> +	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>> +		return;
>> +
>> +	*mode&= ~S_ISGID;
>> +}
>
> Thanks for cleaning up the multiple if statements from last time.
>
> I still would like to see patch 1 replace the code in inode_init_owner
> so that we can compare before and after in the same patch.  Patch 2 can
> then be solely about moving the callsite around the VFS.
>
Ok, then patch 1 can named as"fs/inode: move inode sgid strip operation 
from inode_init_owner into inode_sgid_strip".  What do you think about it?


Best Regards
Yang Xu
> --D
>
>> +EXPORT_SYMBOL(inode_sgid_strip);
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index bbde95387a23..94d94219fe7c 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1897,7 +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);
>> -
>> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
>> +		      umode_t *mode);
>>   /*
>>    * This is the "filldir" function type, used by readdir() to let
>>    * the kernel specify what kind of dirent layout it wants to have.
>> --
>> 2.27.0
>>
Yang Xu (Fujitsu) April 15, 2022, 1:39 a.m. UTC | #4 
on 2022/4/14 20:02, Christian Brauner wrote:
> On Thu, Apr 14, 2022 at 03:57:17PM +0800, Yang Xu wrote:
>> inode_sgid_strip() function is used to strip S_ISGID mode
>> when creat/open/mknod file.
>>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/inode.c         | 18 ++++++++++++++++++
>>   include/linux/fs.h |  3 ++-
>>   2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9d9b422504d1..d63264998855 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2405,3 +2405,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, 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;
>> +	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
>> +		return;
>> +	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>> +		return;
>> +
>> +	*mode&= ~S_ISGID;
>> +}
>> +EXPORT_SYMBOL(inode_sgid_strip);
>
>
> I still think this should return umode_t with the setgid bit stripped
> instead of modifying the mode directly. I may have misunderstood Dave,
> but I thought he preferred to return umode_t too?
Dave's comment as below:
"
Agreed, that's a much nicer API for this function - it makes it
clear that it can modifying the mode that is passed in.
"

So I think Dave should like modify mode directly instead of returning a 
umode_t value.

@Dave  So which way do you mean?

Best Regards
Yang Xu
>
> umode_t inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir, umode_t mode)
> {
> 	if (S_ISDIR(mode))
> 		return mode;
>
> 	if (!dir || !(dir->i_mode&  S_ISGID))
> 		return;
>
> 	if ((mode&  (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP))
> 		return;
>
> 	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
> 		return;
>
> 	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> 		return;
>
> 	return mode&  ~S_ISGID;
> }
Darrick J. Wong April 15, 2022, 1:40 a.m. UTC | #5 
On Fri, Apr 15, 2022 at 01:18:57AM +0000, xuyang2018.jy@fujitsu.com wrote:
> on 2022/4/14 23:57, Darrick J. Wong wrote:
> > On Thu, Apr 14, 2022 at 03:57:17PM +0800, Yang Xu wrote:
> >> inode_sgid_strip() function is used to strip S_ISGID mode
> >> when creat/open/mknod file.
> >>
> >> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> >> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> >> ---
> >>   fs/inode.c         | 18 ++++++++++++++++++
> >>   include/linux/fs.h |  3 ++-
> >>   2 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/inode.c b/fs/inode.c
> >> index 9d9b422504d1..d63264998855 100644
> >> --- a/fs/inode.c
> >> +++ b/fs/inode.c
> >> @@ -2405,3 +2405,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, 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;
> >> +	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
> >> +		return;
> >> +	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> >> +		return;
> >> +
> >> +	*mode&= ~S_ISGID;
> >> +}
> >
> > Thanks for cleaning up the multiple if statements from last time.
> >
> > I still would like to see patch 1 replace the code in inode_init_owner
> > so that we can compare before and after in the same patch.  Patch 2 can
> > then be solely about moving the callsite around the VFS.
> >
> Ok, then patch 1 can named as"fs/inode: move inode sgid strip operation 
> from inode_init_owner into inode_sgid_strip".  What do you think about it?

Sounds good to me.

--D

> 
> Best Regards
> Yang Xu
> > --D
> >
> >> +EXPORT_SYMBOL(inode_sgid_strip);
> >> diff --git a/include/linux/fs.h b/include/linux/fs.h
> >> index bbde95387a23..94d94219fe7c 100644
> >> --- a/include/linux/fs.h
> >> +++ b/include/linux/fs.h
> >> @@ -1897,7 +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);
> >> -
> >> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
> >> +		      umode_t *mode);
> >>   /*
> >>    * This is the "filldir" function type, used by readdir() to let
> >>    * the kernel specify what kind of dirent layout it wants to have.
> >> --
> >> 2.27.0
> >>
diff mbox series

Patch

diff --git a/fs/inode.c b/fs/inode.c
index 9d9b422504d1..d63264998855 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2405,3 +2405,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, 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;
+	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
+		return;
+	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
+		return;
+
+	*mode &= ~S_ISGID;
+}
+EXPORT_SYMBOL(inode_sgid_strip);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbde95387a23..94d94219fe7c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1897,7 +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);
-
+void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
+		      umode_t *mode);
 /*
  * This is the "filldir" function type, used by readdir() to let
  * the kernel specify what kind of dirent layout it wants to have.