* gfp flags for security_inode_alloc()?
@ 2012-03-28 4:22 Tetsuo Handa
2012-03-28 15:50 ` Casey Schaufler
2012-03-29 5:42 ` Dave Chinner
0 siblings, 2 replies; 6+ messages in thread
From: Tetsuo Handa @ 2012-03-28 4:22 UTC (permalink / raw)
To: linux-fsdevel, linux-security-module
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?
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: gfp flags for security_inode_alloc()?
2012-03-28 4:22 gfp flags for security_inode_alloc()? Tetsuo Handa
@ 2012-03-28 15:50 ` Casey Schaufler
2012-03-29 5:42 ` Dave Chinner
1 sibling, 0 replies; 6+ messages in thread
From: Casey Schaufler @ 2012-03-28 15:50 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-fsdevel, linux-security-module, Casey Schaufler
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: gfp flags for security_inode_alloc()?
2012-03-28 4:22 gfp flags for security_inode_alloc()? Tetsuo Handa
2012-03-28 15:50 ` Casey Schaufler
@ 2012-03-29 5:42 ` Dave Chinner
2012-03-29 7:19 ` Tetsuo Handa
1 sibling, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2012-03-29 5:42 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-fsdevel, linux-security-module
On Wed, Mar 28, 2012 at 01:22:17PM +0900, Tetsuo Handa wrote:
> security_inode_alloc() is called from inode_init_always().
> inode_init_always() is called from xfs_inode_alloc().
....
> Therefore, should SMACK use GFP_NOFS like SELinux does?
Yes, because you have no idea what the calling context is except
for the fact that is from somewhere inside filesystem code and the
filesystem could be holding locks. Therefore, GFP_NOFS is really the
only really safe way to allocate memory here.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: gfp flags for security_inode_alloc()?
2012-03-29 5:42 ` Dave Chinner
@ 2012-03-29 7:19 ` Tetsuo Handa
2012-03-29 13:36 ` Casey Schaufler
2012-04-09 16:22 ` Casey Schaufler
0 siblings, 2 replies; 6+ messages in thread
From: Tetsuo Handa @ 2012-03-29 7:19 UTC (permalink / raw)
To: david, casey; +Cc: linux-fsdevel, linux-security-module
Dave Chinner wrote:
> Yes, because you have no idea what the calling context is except
> for the fact that is from somewhere inside filesystem code and the
> filesystem could be holding locks. Therefore, GFP_NOFS is really the
> only really safe way to allocate memory here.
I see. Thank you.
I'm not sure, but can call trace happen where somewhere inside network
filesystem or stackable filesystem code with locks held invokes operations that
involves GFP_KENREL memory allocation outside that filesystem?
----------
[PATCH] SMACK: Fix incorrect GFP_KERNEL usage.
new_inode_smack() which can be called from smack_inode_alloc_security() needs
to use GFP_NOFS like SELinux's inode_alloc_security() does, for
security_inode_alloc() is called from inode_init_always() and
inode_init_always() is called from xfs_inode_alloc() which is using GFP_NOFS.
smack_inode_init_security() needs to use GFP_NOFS like
selinux_inode_init_security() does, for initxattrs() callback function (e.g.
btrfs_initxattrs()) which is called from security_inode_init_security() is
using GFP_NOFS.
smack_audit_rule_match() needs to use GFP_ATOMIC, for
security_audit_rule_match() can be called from audit_filter_user_rules() and
audit_filter_user_rules() is called from audit_filter_user() with RCU read lock
held.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index cd667b4..697cf85 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -79,7 +79,7 @@ struct inode_smack *new_inode_smack(char *smack)
{
struct inode_smack *isp;
- isp = kzalloc(sizeof(struct inode_smack), GFP_KERNEL);
+ isp = kzalloc(sizeof(struct inode_smack), GFP_NOFS);
if (isp == NULL)
return NULL;
@@ -562,7 +562,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
int may;
if (name) {
- *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_KERNEL);
+ *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
if (*name == NULL)
return -ENOMEM;
}
@@ -582,7 +582,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
smk_inode_transmutable(dir))
isp = dsp;
- *value = kstrdup(isp, GFP_KERNEL);
+ *value = kstrdup(isp, GFP_NOFS);
if (*value == NULL)
return -ENOMEM;
}
@@ -3384,7 +3384,7 @@ static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule,
char *rule = vrule;
if (!rule) {
- audit_log(actx, GFP_KERNEL, AUDIT_SELINUX_ERR,
+ audit_log(actx, GFP_ATOMIC, AUDIT_SELINUX_ERR,
"Smack: missing rule\n");
return -ENOENT;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: gfp flags for security_inode_alloc()?
2012-03-29 7:19 ` Tetsuo Handa
@ 2012-03-29 13:36 ` Casey Schaufler
2012-04-09 16:22 ` Casey Schaufler
1 sibling, 0 replies; 6+ messages in thread
From: Casey Schaufler @ 2012-03-29 13:36 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: david, linux-fsdevel, linux-security-module, Casey Schaufler
Thank you. I will test and assuming all goes well apply to the
smack-next tree.
On 3/29/2012 12:19 AM, Tetsuo Handa wrote:
> Dave Chinner wrote:
>> Yes, because you have no idea what the calling context is except
>> for the fact that is from somewhere inside filesystem code and the
>> filesystem could be holding locks. Therefore, GFP_NOFS is really the
>> only really safe way to allocate memory here.
> I see. Thank you.
>
> I'm not sure, but can call trace happen where somewhere inside network
> filesystem or stackable filesystem code with locks held invokes operations that
> involves GFP_KENREL memory allocation outside that filesystem?
> ----------
> [PATCH] SMACK: Fix incorrect GFP_KERNEL usage.
>
> new_inode_smack() which can be called from smack_inode_alloc_security() needs
> to use GFP_NOFS like SELinux's inode_alloc_security() does, for
> security_inode_alloc() is called from inode_init_always() and
> inode_init_always() is called from xfs_inode_alloc() which is using GFP_NOFS.
>
> smack_inode_init_security() needs to use GFP_NOFS like
> selinux_inode_init_security() does, for initxattrs() callback function (e.g.
> btrfs_initxattrs()) which is called from security_inode_init_security() is
> using GFP_NOFS.
>
> smack_audit_rule_match() needs to use GFP_ATOMIC, for
> security_audit_rule_match() can be called from audit_filter_user_rules() and
> audit_filter_user_rules() is called from audit_filter_user() with RCU read lock
> held.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index cd667b4..697cf85 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -79,7 +79,7 @@ struct inode_smack *new_inode_smack(char *smack)
> {
> struct inode_smack *isp;
>
> - isp = kzalloc(sizeof(struct inode_smack), GFP_KERNEL);
> + isp = kzalloc(sizeof(struct inode_smack), GFP_NOFS);
> if (isp == NULL)
> return NULL;
>
> @@ -562,7 +562,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> int may;
>
> if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_KERNEL);
> + *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> if (*name == NULL)
> return -ENOMEM;
> }
> @@ -582,7 +582,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> smk_inode_transmutable(dir))
> isp = dsp;
>
> - *value = kstrdup(isp, GFP_KERNEL);
> + *value = kstrdup(isp, GFP_NOFS);
> if (*value == NULL)
> return -ENOMEM;
> }
> @@ -3384,7 +3384,7 @@ static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule,
> char *rule = vrule;
>
> if (!rule) {
> - audit_log(actx, GFP_KERNEL, AUDIT_SELINUX_ERR,
> + audit_log(actx, GFP_ATOMIC, AUDIT_SELINUX_ERR,
> "Smack: missing rule\n");
> return -ENOENT;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: gfp flags for security_inode_alloc()?
2012-03-29 7:19 ` Tetsuo Handa
2012-03-29 13:36 ` Casey Schaufler
@ 2012-04-09 16:22 ` Casey Schaufler
1 sibling, 0 replies; 6+ messages in thread
From: Casey Schaufler @ 2012-04-09 16:22 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: david, linux-fsdevel, linux-security-module, Casey Schaufler
On 3/29/2012 12:19 AM, Tetsuo Handa wrote:
> Dave Chinner wrote:
>> Yes, because you have no idea what the calling context is except
>> for the fact that is from somewhere inside filesystem code and the
>> filesystem could be holding locks. Therefore, GFP_NOFS is really the
>> only really safe way to allocate memory here.
> I see. Thank you.
>
> I'm not sure, but can call trace happen where somewhere inside network
> filesystem or stackable filesystem code with locks held invokes operations that
> involves GFP_KENREL memory allocation outside that filesystem?
> ----------
> [PATCH] SMACK: Fix incorrect GFP_KERNEL usage.
>
> new_inode_smack() which can be called from smack_inode_alloc_security() needs
> to use GFP_NOFS like SELinux's inode_alloc_security() does, for
> security_inode_alloc() is called from inode_init_always() and
> inode_init_always() is called from xfs_inode_alloc() which is using GFP_NOFS.
>
> smack_inode_init_security() needs to use GFP_NOFS like
> selinux_inode_init_security() does, for initxattrs() callback function (e.g.
> btrfs_initxattrs()) which is called from security_inode_init_security() is
> using GFP_NOFS.
>
> smack_audit_rule_match() needs to use GFP_ATOMIC, for
> security_audit_rule_match() can be called from audit_filter_user_rules() and
> audit_filter_user_rules() is called from audit_filter_user() with RCU read lock
> held.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Applied after minor context adjustment to
git://gitorious.org/smack-next/kernel.git
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index cd667b4..697cf85 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -79,7 +79,7 @@ struct inode_smack *new_inode_smack(char *smack)
> {
> struct inode_smack *isp;
>
> - isp = kzalloc(sizeof(struct inode_smack), GFP_KERNEL);
> + isp = kzalloc(sizeof(struct inode_smack), GFP_NOFS);
> if (isp == NULL)
> return NULL;
>
> @@ -562,7 +562,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> int may;
>
> if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_KERNEL);
> + *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> if (*name == NULL)
> return -ENOMEM;
> }
> @@ -582,7 +582,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> smk_inode_transmutable(dir))
> isp = dsp;
>
> - *value = kstrdup(isp, GFP_KERNEL);
> + *value = kstrdup(isp, GFP_NOFS);
> if (*value == NULL)
> return -ENOMEM;
> }
> @@ -3384,7 +3384,7 @@ static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule,
> char *rule = vrule;
>
> if (!rule) {
> - audit_log(actx, GFP_KERNEL, AUDIT_SELINUX_ERR,
> + audit_log(actx, GFP_ATOMIC, AUDIT_SELINUX_ERR,
> "Smack: missing rule\n");
> return -ENOENT;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-04-09 16:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-28 4:22 gfp flags for security_inode_alloc()? Tetsuo Handa
2012-03-28 15:50 ` Casey Schaufler
2012-03-29 5:42 ` Dave Chinner
2012-03-29 7:19 ` Tetsuo Handa
2012-03-29 13:36 ` Casey Schaufler
2012-04-09 16:22 ` Casey Schaufler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.