diff mbox series

[v3,1/7] fs/inode: move sgid strip operation from inode_init_owner into inode_sgid_strip

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

Commit Message

xuyang2018.jy@fujitsu.com April 15, 2022, 11:02 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 S_ISGID mode when init
a new inode.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
v2->v3:
1.Use const struct inode * instead of struct inode *
2.replace sgid strip with inode_sgid_strip in a single patch
 fs/inode.c         | 24 ++++++++++++++++++++----
 include/linux/fs.h |  3 ++-
 2 files changed, 22 insertions(+), 5 deletions(-)

Comments

Christian Brauner April 15, 2022, 2:09 p.m. UTC | #1
On Fri, Apr 15, 2022 at 07:02:17PM +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 S_ISGID mode when init
> a new inode.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
> v2->v3:
> 1.Use const struct inode * instead of struct inode *
> 2.replace sgid strip with inode_sgid_strip in a single patch
>  fs/inode.c         | 24 ++++++++++++++++++++----
>  include/linux/fs.h |  3 ++-
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d9b422504d1..1b569ad882ce 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
> +			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. :)

I'd like to do some good testing on this.

Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
xuyang2018.jy@fujitsu.com April 18, 2022, 2:08 a.m. UTC | #2
on 2022/4/15 22:09, Christian Brauner wrote:
> On Fri, Apr 15, 2022 at 07:02:17PM +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 S_ISGID mode when init
>> a new inode.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>> v2->v3:
>> 1.Use const struct inode * instead of struct inode *
>> 2.replace sgid strip with inode_sgid_strip in a single patch
>>   fs/inode.c         | 24 ++++++++++++++++++++----
>>   include/linux/fs.h |  3 ++-
>>   2 files changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9d9b422504d1..1b569ad882ce 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
>> +			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.
Sound reasonable.

Best Regards
Yang Xu
>
> 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. :)
>
> I'd like to do some good testing on this.
>
> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
Matthew Wilcox April 18, 2022, 3:08 a.m. UTC | #3
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?
xuyang2018.jy@fujitsu.com April 18, 2022, 8:39 a.m. UTC | #4
on 2022/4/18 11:08, Matthew Wilcox wrote:
> 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
I can't understand why returning umode_t is better. So Does kernel have 
some rules for adding new function I don't notice before? Just need a 
reason.

ps: I will decide whether use pointer or use return umode_t value before 
I send v4.

Best Regards
Yang Xu
>
> 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?
diff mbox series

Patch

diff --git a/fs/inode.c b/fs/inode.c
index 9d9b422504d1..1b569ad882ce 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
+			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;
+	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..4a617aaab6f6 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,
+		      const 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.