Re: gfp flags for security_inode_alloc()?

[Casey Schaufler <casey@schaufler-ca.com>]

On 3/27/2012 9:22 PM, Tetsuo Handa wrote:
> security_inode_alloc() is called from inode_init_always().
> inode_init_always() is called from xfs_inode_alloc().
>
> fs/xfs/xfs_iget.c:
>  58 STATIC struct xfs_inode *
>  59 xfs_inode_alloc(
>  60         struct xfs_mount        *mp,
>  61         xfs_ino_t               ino)
>  62 {
>  63         struct xfs_inode        *ip;
>  64 
>  65         /*
>  66          * if this didn't occur in transactions, we could use
>  67          * KM_MAYFAIL and return NULL here on ENOMEM. Set the
>  68          * code up to do this anyway.
>  69          */
>  70         ip = kmem_zone_alloc(xfs_inode_zone, KM_SLEEP);
>  71         if (!ip)
>  72                 return NULL;
>  73         if (inode_init_always(mp->m_super, VFS_I(ip))) {
>
> kmem_flags_convert(KM_SLEEP) returns GFP_KERNEL or GFP_NOFS depending on
> whether (current->flags & PF_FSTRANS) == 0 or not.
>
> fs/xfs/kmem.c:
> 104 void *
> 105 kmem_zone_alloc(kmem_zone_t *zone, unsigned int __nocast flags)
> 106 {
> 107         int     retries = 0;
> 108         gfp_t   lflags = kmem_flags_convert(flags);
> 109         void    *ptr;
> 110 
> 111         do {
> 112                 ptr = kmem_cache_alloc(zone, lflags);
>
> fs/xfs/kmem.h:
>  40 static inline gfp_t
>  41 kmem_flags_convert(unsigned int __nocast flags)
>  42 {
>  43         gfp_t   lflags;
>  44 
>  45         BUG_ON(flags & ~(KM_SLEEP|KM_NOSLEEP|KM_NOFS|KM_MAYFAIL));
>  46 
>  47         if (flags & KM_NOSLEEP) {
>  48                 lflags = GFP_ATOMIC | __GFP_NOWARN;
>  49         } else {
>  50                 lflags = GFP_KERNEL | __GFP_NOWARN;
>  51                 if ((current->flags & PF_FSTRANS) || (flags & KM_NOFS))
>  52                         lflags &= ~__GFP_FS;
>  53         }
>  54         return lflags;
>  55 }
>
> Therefore, should SMACK use GFP_NOFS like SELinux does?

I am perfectly willing to make the change if it is currently incorrect.
I will happily accept any expert advice this or any other Smack misuse
of GFP flags.

Thank you.

>
> security/smack/smack_lsm.c:
> 520 static int smack_inode_alloc_security(struct inode *inode)
> 521 {
> 522         inode->i_security = new_inode_smack(smk_of_current());
> 523         if (inode->i_security == NULL)
> 524                 return -ENOMEM;
> 525         return 0;
> 526 }
>
> security/smack/smack_lsm.c:
>  75 struct inode_smack *new_inode_smack(char *smack)
>  76 {
>  77         struct inode_smack *isp;
>  78 
>  79         isp = kzalloc(sizeof(struct inode_smack), GFP_KERNEL);
>  80         if (isp == NULL)
>  81                 return NULL;
>
> security/selinux/hooks.c:
> 199 static int inode_alloc_security(struct inode *inode)
> 200 {
> 201         struct inode_security_struct *isec;
> 202         u32 sid = current_sid();
> 203 
> 204         isec = kmem_cache_zalloc(sel_inode_cache, GFP_NOFS);
> 205         if (!isec)
> 206                 return -ENOMEM;
>
> It would be nice if we could encapsulate downgrading from GFP_KERNEL to
> GFP_NOFS into core of memory allocation functions that really need to check
> whether GFP_FS is set or not, for
>
> (1) free page likely be found without checking whether GFP_FS is set or not
>
> (2) if GFP_KERNEL or GFP_NOFS is used, there must be a valid "struct
>     task_struct" which can remember the state whether I must use GFP_NOFS or
>     not (like XFS does). Maybe something like preemption level counter.
>
> (3) (I don't know whether below case can happen or not, but...:)
>     The upper filesystem layer starts something that causes current thread to
>     avoid use of GFP_FS, and the lower filesystem needs to do something that
>     entails memory allocation (e.g. read/write via network layer) caused by
>     request from upper filesystem layer. I'm worrying that the network layer
>     would fail to find that we are under the conditions where use of GFP_KERNEL
>     can cause problems to caller.
>
> If the SMACK question I mentioned above is true, there could be many locations
> where GFP_KERNEL is errornously used.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>