| 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 |
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>
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>
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?
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 --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.
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(-)