Re: [PATCH v2 1/5] attr: add setattr_drop_sgid()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux