On Tue, Oct 11, 2022 at 11:11:37AM +0300, Amir Goldstein wrote:
> On Fri, Oct 7, 2022 at 5:06 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > In setattr_{copy,prepare}() we need to perform the same permission
> > checks to determine whether we need to drop the setgid bit or not.
> > Instead of open-coding it twice add a simple helper the encapsulates the
> > logic. We will reuse this helpers to make dropping the setgid bit during
> > write operations more consistent in a follow up patch.
> >
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
>
> Looks good.
> Some suggestions below - not a must.
>
> Thanks,
> Amir.
>
> > ---
> >
> > Notes:
> > /* v2 */
> > patch added
> >
> > fs/attr.c | 29 ++++++++++++++++++++++++-----
> > 1 file changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/attr.c b/fs/attr.c
> > index 1552a5f23d6b..b1cff6f5b715 100644
> > --- a/fs/attr.c
> > +++ b/fs/attr.c
> > @@ -18,6 +18,27 @@
> > #include <linux/evm.h>
> > #include <linux/ima.h>
> >
> > +/**
> > + * setattr_drop_sgid - check generic setgid permissions
>
> Helper name sounds like a directive, where it should sound like a question.
> e.g. setattr_should_remove_sgid()
sounds good
>
> > + * @mnt_userns: User namespace of the mount the inode was created from
> > + * @inode: inode to check
> > + * @vfsgid: the new/current vfsgid of @inode
> > + *
> > + * This function determines whether the setgid bit needs to be removed because
> > + * the caller lacks privileges over the inode.
> > + *
> > + * Return: true if the setgid bit needs to be removed, false if not.
>
> You may want to consider matching the return value to that of
> should_remove_sgid(), that is 0 or ATTR_KILL_SGID to make all the family of
> those helpers behave similarly.
fine by me
