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) <[email protected]>
> Signed-off-by: Yang Xu <[email protected]>
> ---
> 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.
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)<[email protected]>
>> Signed-off-by: Yang Xu<[email protected]>
>> ---
>> 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.
On Wed, Apr 20, 2022 at 01:27:39AM +0000, [email protected] 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)<[email protected]>
> >> Signed-off-by: Yang Xu<[email protected]>
> >> ---
> >> 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.
--
Dave Chinner
[email protected]
on 2022/4/21 5:52, Dave Chinner wrote:
> On Wed, Apr 20, 2022 at 01:27:39AM +0000, [email protected] 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)<[email protected]>
>>>> Signed-off-by: Yang Xu<[email protected]>
>>>> ---
>>>> 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.