diff mbox series

[v4,1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

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

Commit Message

Yang Xu (Fujitsu) April 19, 2022, 11:47 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.

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

Comments

Christian Brauner April 19, 2022, 2:05 p.m. UTC | #1 
On Tue, Apr 19, 2022 at 07:47:07PM +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.
> 
> Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/inode.c         | 22 ++++++++++++++++++----
>  include/linux/fs.h |  3 ++-
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d9b422504d1..3215e61a0021 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,19 @@ 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)
> +{

I think with Willy agreeing in an earlier version with me and you
needing to resend anyway I'd say have this return umode_t instead of
passing a pointer.
Yang Xu (Fujitsu) April 20, 2022, 1:27 a.m. UTC | #2 
on 2022/4/19 22:05, Christian Brauner wrote:
> On Tue, Apr 19, 2022 at 07:47:07PM +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.
>>
>> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/inode.c         | 22 ++++++++++++++++++----
>>   include/linux/fs.h |  3 ++-
>>   2 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9d9b422504d1..3215e61a0021 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,19 @@ 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)
>> +{
>
> I think with Willy agreeing in an earlier version with me and you
> needing to resend anyway I'd say have this return umode_t instead of
> passing a pointer.

IMO, I am fine with your and Willy way. But I need a reason otherwise
I can't convince myself why not use mode pointer directly.

I have asked you and Willy before why return umode_t value is better, 
why not modify mode pointer directly? Since we have use mode as 
argument, why not modify mode pointer directly in function?

Also the function name(inode_sgid_strip and prepare_mode) has  expressed 
their function clearly.
Dave Chinner April 20, 2022, 9:52 p.m. UTC | #3 
On Wed, Apr 20, 2022 at 01:27:39AM +0000, xuyang2018.jy@fujitsu.com wrote:
> on 2022/4/19 22:05, Christian Brauner wrote:
> > On Tue, Apr 19, 2022 at 07:47:07PM +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.
> >>
> >> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> >> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> >> ---
> >>   fs/inode.c         | 22 ++++++++++++++++++----
> >>   include/linux/fs.h |  3 ++-
> >>   2 files changed, 20 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/inode.c b/fs/inode.c
> >> index 9d9b422504d1..3215e61a0021 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,19 @@ 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)
> >> +{
> >
> > I think with Willy agreeing in an earlier version with me and you
> > needing to resend anyway I'd say have this return umode_t instead of
> > passing a pointer.
> 
> IMO, I am fine with your and Willy way. But I need a reason otherwise
> I can't convince myself why not use mode pointer directly.

You should listen to experienced developers like Willy and Christian
when they say "follow existing coding conventions".  Indeed, Darrick
has also mentioned he'd prefer it to return the new mode, and I'd
also prefer that it returns the new mode.

> I have asked you and Willy before why return umode_t value is better, 
> why not modify mode pointer directly? Since we have use mode as 
> argument, why not modify mode pointer directly in function?

If the function had mulitple return status (e.g. an error or a mode)
the convention is to pass the mode output variable by reference and
return the error status. But there is only one return value from
this function - the mode - and hence it should be returned in the
return value, not passed by reference.

Passing by reference unnecessarily makes the code more complex and
less mainatainable.  Code that returns a single value is easy to
understand, is more flexible in the way callers can use it and it's
simpler to maintain.

Cheers,

Dave.
Yang Xu (Fujitsu) April 21, 2022, 1:11 a.m. UTC | #4 
on 2022/4/21 5:52, Dave Chinner wrote:
> On Wed, Apr 20, 2022 at 01:27:39AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> on 2022/4/19 22:05, Christian Brauner wrote:
>>> On Tue, Apr 19, 2022 at 07:47:07PM +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.
>>>>
>>>> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>>> ---
>>>>    fs/inode.c         | 22 ++++++++++++++++++----
>>>>    include/linux/fs.h |  3 ++-
>>>>    2 files changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/inode.c b/fs/inode.c
>>>> index 9d9b422504d1..3215e61a0021 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,19 @@ 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)
>>>> +{
>>>
>>> I think with Willy agreeing in an earlier version with me and you
>>> needing to resend anyway I'd say have this return umode_t instead of
>>> passing a pointer.
>>
>> IMO, I am fine with your and Willy way. But I need a reason otherwise
>> I can't convince myself why not use mode pointer directly.
>
> You should listen to experienced developers like Willy and Christian
> when they say "follow existing coding conventions".  Indeed, Darrick
> has also mentioned he'd prefer it to return the new mode, and I'd
> also prefer that it returns the new mode.
OK. I just don't know  the "follow existing coding conventions" reason. 
Now, I know and understand.
>
>> I have asked you and Willy before why return umode_t value is better,
>> why not modify mode pointer directly? Since we have use mode as
>> argument, why not modify mode pointer directly in function?
>
> If the function had mulitple return status (e.g. an error or a mode)
> the convention is to pass the mode output variable by reference and
> return the error status. But there is only one return value from
> this function - the mode - and hence it should be returned in the
> return value, not passed by reference.
>
> Passing by reference unnecessarily makes the code more complex and
> less mainatainable.  Code that returns a single value is easy to
> understand, is more flexible in the way callers can use it and it's
> simpler to maintain.
OK,  it sounds reasonable and will use return value.

ps: Of course, I will remember this in my mind. Thanks for your replay.

Best Regards
Yang Xu

>
> Cheers,
>
> Dave.
diff mbox series

Patch

diff --git a/fs/inode.c b/fs/inode.c
index 9d9b422504d1..3215e61a0021 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,19 @@  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 (S_ISDIR(*mode) || !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;
+
+	*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.