* [Ocfs2-devel] [PATCH] xattr: Constify ->name member of "struct xattr".
@ 2013-06-08 12:54 ` Tetsuo Handa
0 siblings, 0 replies; 18+ messages in thread
From: Tetsuo Handa @ 2013-06-08 12:55 UTC (permalink / raw)
To: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn
Cc: linux-fsdevel, linux-security-module
From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 8 Jun 2013 21:46:58 +0900
Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
checking by constifying ->name member of "struct xattr".
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
fs/ocfs2/xattr.h | 2 +-
include/linux/security.h | 8 ++++----
include/linux/xattr.h | 2 +-
include/uapi/linux/reiserfs_xattr.h | 2 +-
security/capability.c | 2 +-
security/integrity/evm/evm_main.c | 2 +-
security/security.c | 8 +++-----
security/selinux/hooks.c | 17 ++++++-----------
security/smack/smack_lsm.c | 9 +++------
9 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
index e5c7f15..19f134e 100644
--- a/fs/ocfs2/xattr.h
+++ b/fs/ocfs2/xattr.h
@@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
struct ocfs2_security_xattr_info {
int enable;
- char *name;
+ const char *name;
void *value;
size_t value_len;
};
diff --git a/include/linux/security.h b/include/linux/security.h
index 40560f4..2e64d73 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1466,7 +1466,7 @@ struct security_operations {
int (*inode_alloc_security) (struct inode *inode);
void (*inode_free_security) (struct inode *inode);
int (*inode_init_security) (struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len);
int (*inode_create) (struct inode *dir,
struct dentry *dentry, umode_t mode);
@@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len);
int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
int security_inode_link(struct dentry *old_dentry, struct inode *dir,
@@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
static inline int security_old_inode_init_security(struct inode *inode,
struct inode *dir,
const struct qstr *qstr,
- char **name, void **value,
- size_t *len)
+ const char **name,
+ void **value, size_t *len)
{
return -EOPNOTSUPP;
}
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index fdbafc6..91b0a68 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -31,7 +31,7 @@ struct xattr_handler {
};
struct xattr {
- char *name;
+ const char *name;
void *value;
size_t value_len;
};
diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
index d8ce17c..38fdd64 100644
--- a/include/uapi/linux/reiserfs_xattr.h
+++ b/include/uapi/linux/reiserfs_xattr.h
@@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
};
struct reiserfs_security_handle {
- char *name;
+ const char *name;
void *value;
size_t length;
};
diff --git a/security/capability.c b/security/capability.c
index 1728d4e..b311ff0 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
}
static int cap_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
return -EOPNOTSUPP;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index cdbde17..2787080 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
evm_xattr->value = xattr_data;
evm_xattr->value_len = sizeof(*xattr_data);
- evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
+ evm_xattr->name = XATTR_EVM_SUFFIX;
return 0;
out:
kfree(xattr_data);
diff --git a/security/security.c b/security/security.c
index a3dce87..8849691 100644
--- a/security/security.c
+++ b/security/security.c
@@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
if (unlikely(IS_PRIVATE(inode)))
return 0;
- memset(new_xattrs, 0, sizeof new_xattrs);
if (!initxattrs)
return security_ops->inode_init_security(inode, dir, qstr,
NULL, NULL, NULL);
+ memset(new_xattrs, 0, sizeof(new_xattrs));
lsm_xattr = new_xattrs;
ret = security_ops->inode_init_security(inode, dir, qstr,
&lsm_xattr->name,
@@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
goto out;
ret = initxattrs(inode, new_xattrs, fs_data);
out:
- for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
- kfree(xattr->name);
+ for (xattr = new_xattrs; xattr->value != NULL; xattr++)
kfree(xattr->value);
- }
return (ret == -EOPNOTSUPP) ? 0 : ret;
}
EXPORT_SYMBOL(security_inode_init_security);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
if (unlikely(IS_PRIVATE(inode)))
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5c6f2cd..16b8e51 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
}
static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr,
+ const char **name,
void **value, size_t *len)
{
const struct task_security_struct *tsec = current_security();
@@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
struct superblock_security_struct *sbsec;
u32 sid, newsid, clen;
int rc;
- char *namep = NULL, *context;
+ char *context;
dsec = dir->i_security;
sbsec = dir->i_sb->s_security;
@@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
return -EOPNOTSUPP;
- if (name) {
- namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
- if (!namep)
- return -ENOMEM;
- *name = namep;
- }
+ if (name)
+ *name = XATTR_SELINUX_SUFFIX;
if (value && len) {
rc = security_sid_to_context_force(newsid, &context, &clen);
- if (rc) {
- kfree(namep);
+ if (rc)
return rc;
- }
*value = context;
*len = clen;
}
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index d52c780..46e5b47 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode *inode)
* Returns 0 if it all works out, -ENOMEM if there's no memory
*/
static int smack_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
struct smack_known *skp;
@@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
char *dsp = smk_of_inode(dir);
int may;
- if (name) {
- *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
- if (*name == NULL)
- return -ENOMEM;
- }
+ if (name)
+ *name = XATTR_SMACK_SUFFIX;
if (value) {
skp = smk_find_entry(csp);
--
1.7.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH] xattr: Constify ->name member of "struct xattr".
@ 2013-06-08 12:54 ` Tetsuo Handa
0 siblings, 0 replies; 18+ messages in thread
From: Tetsuo Handa @ 2013-06-08 12:54 UTC (permalink / raw)
To: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn
Cc: linux-fsdevel, linux-security-module
From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 8 Jun 2013 21:46:58 +0900
Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
checking by constifying ->name member of "struct xattr".
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
fs/ocfs2/xattr.h | 2 +-
include/linux/security.h | 8 ++++----
include/linux/xattr.h | 2 +-
include/uapi/linux/reiserfs_xattr.h | 2 +-
security/capability.c | 2 +-
security/integrity/evm/evm_main.c | 2 +-
security/security.c | 8 +++-----
security/selinux/hooks.c | 17 ++++++-----------
security/smack/smack_lsm.c | 9 +++------
9 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
index e5c7f15..19f134e 100644
--- a/fs/ocfs2/xattr.h
+++ b/fs/ocfs2/xattr.h
@@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
struct ocfs2_security_xattr_info {
int enable;
- char *name;
+ const char *name;
void *value;
size_t value_len;
};
diff --git a/include/linux/security.h b/include/linux/security.h
index 40560f4..2e64d73 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1466,7 +1466,7 @@ struct security_operations {
int (*inode_alloc_security) (struct inode *inode);
void (*inode_free_security) (struct inode *inode);
int (*inode_init_security) (struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len);
int (*inode_create) (struct inode *dir,
struct dentry *dentry, umode_t mode);
@@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len);
int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
int security_inode_link(struct dentry *old_dentry, struct inode *dir,
@@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
static inline int security_old_inode_init_security(struct inode *inode,
struct inode *dir,
const struct qstr *qstr,
- char **name, void **value,
- size_t *len)
+ const char **name,
+ void **value, size_t *len)
{
return -EOPNOTSUPP;
}
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index fdbafc6..91b0a68 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -31,7 +31,7 @@ struct xattr_handler {
};
struct xattr {
- char *name;
+ const char *name;
void *value;
size_t value_len;
};
diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
index d8ce17c..38fdd64 100644
--- a/include/uapi/linux/reiserfs_xattr.h
+++ b/include/uapi/linux/reiserfs_xattr.h
@@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
};
struct reiserfs_security_handle {
- char *name;
+ const char *name;
void *value;
size_t length;
};
diff --git a/security/capability.c b/security/capability.c
index 1728d4e..b311ff0 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
}
static int cap_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
return -EOPNOTSUPP;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index cdbde17..2787080 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
evm_xattr->value = xattr_data;
evm_xattr->value_len = sizeof(*xattr_data);
- evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
+ evm_xattr->name = XATTR_EVM_SUFFIX;
return 0;
out:
kfree(xattr_data);
diff --git a/security/security.c b/security/security.c
index a3dce87..8849691 100644
--- a/security/security.c
+++ b/security/security.c
@@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
if (unlikely(IS_PRIVATE(inode)))
return 0;
- memset(new_xattrs, 0, sizeof new_xattrs);
if (!initxattrs)
return security_ops->inode_init_security(inode, dir, qstr,
NULL, NULL, NULL);
+ memset(new_xattrs, 0, sizeof(new_xattrs));
lsm_xattr = new_xattrs;
ret = security_ops->inode_init_security(inode, dir, qstr,
&lsm_xattr->name,
@@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
goto out;
ret = initxattrs(inode, new_xattrs, fs_data);
out:
- for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
- kfree(xattr->name);
+ for (xattr = new_xattrs; xattr->value != NULL; xattr++)
kfree(xattr->value);
- }
return (ret == -EOPNOTSUPP) ? 0 : ret;
}
EXPORT_SYMBOL(security_inode_init_security);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
if (unlikely(IS_PRIVATE(inode)))
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5c6f2cd..16b8e51 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
}
static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr,
+ const char **name,
void **value, size_t *len)
{
const struct task_security_struct *tsec = current_security();
@@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
struct superblock_security_struct *sbsec;
u32 sid, newsid, clen;
int rc;
- char *namep = NULL, *context;
+ char *context;
dsec = dir->i_security;
sbsec = dir->i_sb->s_security;
@@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
return -EOPNOTSUPP;
- if (name) {
- namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
- if (!namep)
- return -ENOMEM;
- *name = namep;
- }
+ if (name)
+ *name = XATTR_SELINUX_SUFFIX;
if (value && len) {
rc = security_sid_to_context_force(newsid, &context, &clen);
- if (rc) {
- kfree(namep);
+ if (rc)
return rc;
- }
*value = context;
*len = clen;
}
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index d52c780..46e5b47 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode *inode)
* Returns 0 if it all works out, -ENOMEM if there's no memory
*/
static int smack_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
struct smack_known *skp;
@@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
char *dsp = smk_of_inode(dir);
int may;
- if (name) {
- *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
- if (*name == NULL)
- return -ENOMEM;
- }
+ if (name)
+ *name = XATTR_SMACK_SUFFIX;
if (value) {
skp = smk_find_entry(csp);
--
1.7.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] xattr: Constify ->name member of "struct xattr".
2013-06-08 12:54 ` Tetsuo Handa
@ 2013-06-09 9:07 ` Joel Becker
-1 siblings, 0 replies; 18+ messages in thread
From: Joel Becker @ 2013-06-09 9:07 UTC (permalink / raw)
To: Tetsuo Handa
Cc: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn,
linux-fsdevel, linux-security-module
I can't really comment on the concept, but if security folks agree, the
ocfs2 part looks fine.
Reviewed-by: Joel Becker <jlbec@evilplan.org>
On Sat, Jun 08, 2013 at 09:54:56PM +0900, Tetsuo Handa wrote:
> >From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 8 Jun 2013 21:46:58 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
> checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> fs/ocfs2/xattr.h | 2 +-
> include/linux/security.h | 8 ++++----
> include/linux/xattr.h | 2 +-
> include/uapi/linux/reiserfs_xattr.h | 2 +-
> security/capability.c | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 8 +++-----
> security/selinux/hooks.c | 17 ++++++-----------
> security/smack/smack_lsm.c | 9 +++------
> 9 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index e5c7f15..19f134e 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
>
> struct ocfs2_security_xattr_info {
> int enable;
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 40560f4..2e64d73 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1466,7 +1466,7 @@ struct security_operations {
> int (*inode_alloc_security) (struct inode *inode);
> void (*inode_free_security) (struct inode *inode);
> int (*inode_init_security) (struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int (*inode_create) (struct inode *dir,
> struct dentry *dentry, umode_t mode);
> @@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
> int security_inode_link(struct dentry *old_dentry, struct inode *dir,
> @@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
> static inline int security_old_inode_init_security(struct inode *inode,
> struct inode *dir,
> const struct qstr *qstr,
> - char **name, void **value,
> - size_t *len)
> + const char **name,
> + void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index fdbafc6..91b0a68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -31,7 +31,7 @@ struct xattr_handler {
> };
>
> struct xattr {
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
> index d8ce17c..38fdd64 100644
> --- a/include/uapi/linux/reiserfs_xattr.h
> +++ b/include/uapi/linux/reiserfs_xattr.h
> @@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
> };
>
> struct reiserfs_security_handle {
> - char *name;
> + const char *name;
> void *value;
> size_t length;
> };
> diff --git a/security/capability.c b/security/capability.c
> index 1728d4e..b311ff0 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
> }
>
> static int cap_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index cdbde17..2787080 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
>
> evm_xattr->value = xattr_data;
> evm_xattr->value_len = sizeof(*xattr_data);
> - evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
> + evm_xattr->name = XATTR_EVM_SUFFIX;
> return 0;
> out:
> kfree(xattr_data);
> diff --git a/security/security.c b/security/security.c
> index a3dce87..8849691 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> if (unlikely(IS_PRIVATE(inode)))
> return 0;
>
> - memset(new_xattrs, 0, sizeof new_xattrs);
> if (!initxattrs)
> return security_ops->inode_init_security(inode, dir, qstr,
> NULL, NULL, NULL);
> + memset(new_xattrs, 0, sizeof(new_xattrs));
> lsm_xattr = new_xattrs;
> ret = security_ops->inode_init_security(inode, dir, qstr,
> &lsm_xattr->name,
> @@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> out:
> - for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
> - kfree(xattr->name);
> + for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> - }
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> if (unlikely(IS_PRIVATE(inode)))
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5c6f2cd..16b8e51 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
> }
>
> static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr,
> + const char **name,
> void **value, size_t *len)
> {
> const struct task_security_struct *tsec = current_security();
> @@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> struct superblock_security_struct *sbsec;
> u32 sid, newsid, clen;
> int rc;
> - char *namep = NULL, *context;
> + char *context;
>
> dsec = dir->i_security;
> sbsec = dir->i_sb->s_security;
> @@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
> return -EOPNOTSUPP;
>
> - if (name) {
> - namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
> - if (!namep)
> - return -ENOMEM;
> - *name = namep;
> - }
> + if (name)
> + *name = XATTR_SELINUX_SUFFIX;
>
> if (value && len) {
> rc = security_sid_to_context_force(newsid, &context, &clen);
> - if (rc) {
> - kfree(namep);
> + if (rc)
> return rc;
> - }
> *value = context;
> *len = clen;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..46e5b47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode *inode)
> * Returns 0 if it all works out, -ENOMEM if there's no memory
> */
> static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> struct smack_known *skp;
> @@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> char *dsp = smk_of_inode(dir);
> int may;
>
> - if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> - if (*name == NULL)
> - return -ENOMEM;
> - }
> + if (name)
> + *name = XATTR_SMACK_SUFFIX;
>
> if (value) {
> skp = smk_find_entry(csp);
> --
> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
"Sometimes I think the surest sign intelligent
life exists elsewhere in the universe is that
none of it has tried to contact us."
-Calvin & Hobbes
http://www.jlbec.org/
jlbec@evilplan.org
^ permalink raw reply [flat|nested] 18+ messages in thread* [Ocfs2-devel] [PATCH] xattr: Constify ->name member of "struct xattr".
@ 2013-06-09 9:07 ` Joel Becker
0 siblings, 0 replies; 18+ messages in thread
From: Joel Becker @ 2013-06-09 9:07 UTC (permalink / raw)
To: Tetsuo Handa
Cc: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn,
linux-fsdevel, linux-security-module
I can't really comment on the concept, but if security folks agree, the
ocfs2 part looks fine.
Reviewed-by: Joel Becker <jlbec@evilplan.org>
On Sat, Jun 08, 2013 at 09:54:56PM +0900, Tetsuo Handa wrote:
> >From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 8 Jun 2013 21:46:58 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
> checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> fs/ocfs2/xattr.h | 2 +-
> include/linux/security.h | 8 ++++----
> include/linux/xattr.h | 2 +-
> include/uapi/linux/reiserfs_xattr.h | 2 +-
> security/capability.c | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 8 +++-----
> security/selinux/hooks.c | 17 ++++++-----------
> security/smack/smack_lsm.c | 9 +++------
> 9 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index e5c7f15..19f134e 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
>
> struct ocfs2_security_xattr_info {
> int enable;
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 40560f4..2e64d73 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1466,7 +1466,7 @@ struct security_operations {
> int (*inode_alloc_security) (struct inode *inode);
> void (*inode_free_security) (struct inode *inode);
> int (*inode_init_security) (struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int (*inode_create) (struct inode *dir,
> struct dentry *dentry, umode_t mode);
> @@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
> int security_inode_link(struct dentry *old_dentry, struct inode *dir,
> @@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
> static inline int security_old_inode_init_security(struct inode *inode,
> struct inode *dir,
> const struct qstr *qstr,
> - char **name, void **value,
> - size_t *len)
> + const char **name,
> + void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index fdbafc6..91b0a68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -31,7 +31,7 @@ struct xattr_handler {
> };
>
> struct xattr {
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
> index d8ce17c..38fdd64 100644
> --- a/include/uapi/linux/reiserfs_xattr.h
> +++ b/include/uapi/linux/reiserfs_xattr.h
> @@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
> };
>
> struct reiserfs_security_handle {
> - char *name;
> + const char *name;
> void *value;
> size_t length;
> };
> diff --git a/security/capability.c b/security/capability.c
> index 1728d4e..b311ff0 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
> }
>
> static int cap_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index cdbde17..2787080 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
>
> evm_xattr->value = xattr_data;
> evm_xattr->value_len = sizeof(*xattr_data);
> - evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
> + evm_xattr->name = XATTR_EVM_SUFFIX;
> return 0;
> out:
> kfree(xattr_data);
> diff --git a/security/security.c b/security/security.c
> index a3dce87..8849691 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> if (unlikely(IS_PRIVATE(inode)))
> return 0;
>
> - memset(new_xattrs, 0, sizeof new_xattrs);
> if (!initxattrs)
> return security_ops->inode_init_security(inode, dir, qstr,
> NULL, NULL, NULL);
> + memset(new_xattrs, 0, sizeof(new_xattrs));
> lsm_xattr = new_xattrs;
> ret = security_ops->inode_init_security(inode, dir, qstr,
> &lsm_xattr->name,
> @@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> out:
> - for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
> - kfree(xattr->name);
> + for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> - }
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> if (unlikely(IS_PRIVATE(inode)))
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5c6f2cd..16b8e51 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
> }
>
> static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr,
> + const char **name,
> void **value, size_t *len)
> {
> const struct task_security_struct *tsec = current_security();
> @@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> struct superblock_security_struct *sbsec;
> u32 sid, newsid, clen;
> int rc;
> - char *namep = NULL, *context;
> + char *context;
>
> dsec = dir->i_security;
> sbsec = dir->i_sb->s_security;
> @@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
> return -EOPNOTSUPP;
>
> - if (name) {
> - namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
> - if (!namep)
> - return -ENOMEM;
> - *name = namep;
> - }
> + if (name)
> + *name = XATTR_SELINUX_SUFFIX;
>
> if (value && len) {
> rc = security_sid_to_context_force(newsid, &context, &clen);
> - if (rc) {
> - kfree(namep);
> + if (rc)
> return rc;
> - }
> *value = context;
> *len = clen;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..46e5b47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode *inode)
> * Returns 0 if it all works out, -ENOMEM if there's no memory
> */
> static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> struct smack_known *skp;
> @@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> char *dsp = smk_of_inode(dir);
> int may;
>
> - if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> - if (*name == NULL)
> - return -ENOMEM;
> - }
> + if (name)
> + *name = XATTR_SMACK_SUFFIX;
>
> if (value) {
> skp = smk_find_entry(csp);
> --
> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
"Sometimes I think the surest sign intelligent
life exists elsewhere in the universe is that
none of it has tried to contact us."
-Calvin & Hobbes
http://www.jlbec.org/
jlbec at evilplan.org
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xattr: Constify ->name member of "struct xattr".
2013-06-09 9:07 ` [Ocfs2-devel] " Joel Becker
@ 2013-06-09 12:10 ` Tetsuo Handa
-1 siblings, 0 replies; 18+ messages in thread
From: Tetsuo Handa @ 2013-06-09 12:09 UTC (permalink / raw)
To: jlbec
Cc: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn,
linux-fsdevel, linux-security-module
Joel Becker wrote:
> I can't really comment on the concept, but if security folks agree, the
> ocfs2 part looks fine.
This is merely a bugfix/cleanup with a bit of cpu usage reduction.
I noticed that EVM by error returns "struct xattr"->name == NULL and
"struct xattr"->value != NULL if kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS) failed,
resulting in EVM security attribute not associated with inode and leak memory.
The proposed (but now turned out to be incorrect) fix is
http://marc.info/?l=linux-security-module&m=136965357007829&w=2 .
But I also noticed that all of SELinux, Smack and EVM sets "struct xattr"->name
a kstrdup()ed constant suffix. Therefore, if passing constant suffix
("const char *") rather than kstrdup()ed constant suffix ("char *") does not
break "struct xattr"->name users, SELinux, Smack and EVM can omit kstrdup()ing
their constant suffix. Therefore, I'm querying you whether changing from
"char *name" to "const char *name" breaks your filesystem.
If my proposal breaks your filesystem, I'm happy to fix only EVM code.
Regards.
^ permalink raw reply [flat|nested] 18+ messages in thread* [Ocfs2-devel] [PATCH] xattr: Constify ->name member of "struct xattr".
@ 2013-06-09 12:10 ` Tetsuo Handa
0 siblings, 0 replies; 18+ messages in thread
From: Tetsuo Handa @ 2013-06-09 12:10 UTC (permalink / raw)
To: jlbec
Cc: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn,
linux-fsdevel, linux-security-module
Joel Becker wrote:
> I can't really comment on the concept, but if security folks agree, the
> ocfs2 part looks fine.
This is merely a bugfix/cleanup with a bit of cpu usage reduction.
I noticed that EVM by error returns "struct xattr"->name == NULL and
"struct xattr"->value != NULL if kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS) failed,
resulting in EVM security attribute not associated with inode and leak memory.
The proposed (but now turned out to be incorrect) fix is
http://marc.info/?l=linux-security-module&m=136965357007829&w=2 .
But I also noticed that all of SELinux, Smack and EVM sets "struct xattr"->name
a kstrdup()ed constant suffix. Therefore, if passing constant suffix
("const char *") rather than kstrdup()ed constant suffix ("char *") does not
break "struct xattr"->name users, SELinux, Smack and EVM can omit kstrdup()ing
their constant suffix. Therefore, I'm querying you whether changing from
"char *name" to "const char *name" breaks your filesystem.
If my proposal breaks your filesystem, I'm happy to fix only EVM code.
Regards.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xattr: Constify ->name member of "struct xattr".
2013-06-09 12:10 ` [Ocfs2-devel] " Tetsuo Handa
@ 2013-06-24 13:05 ` Tetsuo Handa
-1 siblings, 0 replies; 18+ messages in thread
From: Tetsuo Handa @ 2013-06-24 13:05 UTC (permalink / raw)
To: eparis, zohar, reiserfs-devel; +Cc: linux-security-module, linux-fsdevel
Eric and Mimi, can this patch go to linux-next.git via linux-security.git ?
----------
>From 3f4e9317d1c689a78ac1e94126c74826dbe9c9e3 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Mon, 24 Jun 2013 21:35:22 +0900
Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
checking by constifying ->name member of "struct xattr".
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-by: Joel Becker <jlbec@evilplan.org> [ocfs2]
Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
---
fs/ocfs2/xattr.h | 2 +-
include/linux/security.h | 8 ++++----
include/linux/xattr.h | 2 +-
include/uapi/linux/reiserfs_xattr.h | 2 +-
security/capability.c | 2 +-
security/integrity/evm/evm_main.c | 2 +-
security/security.c | 8 +++-----
security/selinux/hooks.c | 17 ++++++-----------
security/smack/smack_lsm.c | 9 +++------
9 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
index e5c7f15..19f134e 100644
--- a/fs/ocfs2/xattr.h
+++ b/fs/ocfs2/xattr.h
@@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
struct ocfs2_security_xattr_info {
int enable;
- char *name;
+ const char *name;
void *value;
size_t value_len;
};
diff --git a/include/linux/security.h b/include/linux/security.h
index 40560f4..2e64d73 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1466,7 +1466,7 @@ struct security_operations {
int (*inode_alloc_security) (struct inode *inode);
void (*inode_free_security) (struct inode *inode);
int (*inode_init_security) (struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len);
int (*inode_create) (struct inode *dir,
struct dentry *dentry, umode_t mode);
@@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len);
int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
int security_inode_link(struct dentry *old_dentry, struct inode *dir,
@@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
static inline int security_old_inode_init_security(struct inode *inode,
struct inode *dir,
const struct qstr *qstr,
- char **name, void **value,
- size_t *len)
+ const char **name,
+ void **value, size_t *len)
{
return -EOPNOTSUPP;
}
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index fdbafc6..91b0a68 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -31,7 +31,7 @@ struct xattr_handler {
};
struct xattr {
- char *name;
+ const char *name;
void *value;
size_t value_len;
};
diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
index d8ce17c..38fdd64 100644
--- a/include/uapi/linux/reiserfs_xattr.h
+++ b/include/uapi/linux/reiserfs_xattr.h
@@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
};
struct reiserfs_security_handle {
- char *name;
+ const char *name;
void *value;
size_t length;
};
diff --git a/security/capability.c b/security/capability.c
index 1728d4e..b311ff0 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
}
static int cap_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
return -EOPNOTSUPP;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index df0fa45..af9b685 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -418,7 +418,7 @@ int evm_inode_init_security(struct inode *inode,
evm_xattr->value = xattr_data;
evm_xattr->value_len = sizeof(*xattr_data);
- evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
+ evm_xattr->name = XATTR_EVM_SUFFIX;
return 0;
out:
kfree(xattr_data);
diff --git a/security/security.c b/security/security.c
index a3dce87..8849691 100644
--- a/security/security.c
+++ b/security/security.c
@@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
if (unlikely(IS_PRIVATE(inode)))
return 0;
- memset(new_xattrs, 0, sizeof new_xattrs);
if (!initxattrs)
return security_ops->inode_init_security(inode, dir, qstr,
NULL, NULL, NULL);
+ memset(new_xattrs, 0, sizeof(new_xattrs));
lsm_xattr = new_xattrs;
ret = security_ops->inode_init_security(inode, dir, qstr,
&lsm_xattr->name,
@@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
goto out;
ret = initxattrs(inode, new_xattrs, fs_data);
out:
- for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
- kfree(xattr->name);
+ for (xattr = new_xattrs; xattr->value != NULL; xattr++)
kfree(xattr->value);
- }
return (ret == -EOPNOTSUPP) ? 0 : ret;
}
EXPORT_SYMBOL(security_inode_init_security);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
if (unlikely(IS_PRIVATE(inode)))
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5c6f2cd..16b8e51 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
}
static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr,
+ const char **name,
void **value, size_t *len)
{
const struct task_security_struct *tsec = current_security();
@@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
struct superblock_security_struct *sbsec;
u32 sid, newsid, clen;
int rc;
- char *namep = NULL, *context;
+ char *context;
dsec = dir->i_security;
sbsec = dir->i_sb->s_security;
@@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
return -EOPNOTSUPP;
- if (name) {
- namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
- if (!namep)
- return -ENOMEM;
- *name = namep;
- }
+ if (name)
+ *name = XATTR_SELINUX_SUFFIX;
if (value && len) {
rc = security_sid_to_context_force(newsid, &context, &clen);
- if (rc) {
- kfree(namep);
+ if (rc)
return rc;
- }
*value = context;
*len = clen;
}
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 6a08330..2dfccb7 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -582,7 +582,7 @@ static void smack_inode_free_security(struct inode *inode)
* Returns 0 if it all works out, -ENOMEM if there's no memory
*/
static int smack_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
struct inode_smack *issp = inode->i_security;
@@ -591,11 +591,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
char *dsp = smk_of_inode(dir);
int may;
- if (name) {
- *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
- if (*name == NULL)
- return -ENOMEM;
- }
+ if (name)
+ *name = XATTR_SMACK_SUFFIX;
if (value) {
rcu_read_lock();
--
1.7.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH] xattr: Constify ->name member of "struct xattr".
@ 2013-06-24 13:05 ` Tetsuo Handa
0 siblings, 0 replies; 18+ messages in thread
From: Tetsuo Handa @ 2013-06-24 13:05 UTC (permalink / raw)
To: eparis, zohar, reiserfs-devel; +Cc: linux-security-module, linux-fsdevel
Eric and Mimi, can this patch go to linux-next.git via linux-security.git ?
----------
From 3f4e9317d1c689a78ac1e94126c74826dbe9c9e3 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Mon, 24 Jun 2013 21:35:22 +0900
Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
checking by constifying ->name member of "struct xattr".
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-by: Joel Becker <jlbec@evilplan.org> [ocfs2]
Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
---
fs/ocfs2/xattr.h | 2 +-
include/linux/security.h | 8 ++++----
include/linux/xattr.h | 2 +-
include/uapi/linux/reiserfs_xattr.h | 2 +-
security/capability.c | 2 +-
security/integrity/evm/evm_main.c | 2 +-
security/security.c | 8 +++-----
security/selinux/hooks.c | 17 ++++++-----------
security/smack/smack_lsm.c | 9 +++------
9 files changed, 21 insertions(+), 31 deletions(-)
diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
index e5c7f15..19f134e 100644
--- a/fs/ocfs2/xattr.h
+++ b/fs/ocfs2/xattr.h
@@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
struct ocfs2_security_xattr_info {
int enable;
- char *name;
+ const char *name;
void *value;
size_t value_len;
};
diff --git a/include/linux/security.h b/include/linux/security.h
index 40560f4..2e64d73 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1466,7 +1466,7 @@ struct security_operations {
int (*inode_alloc_security) (struct inode *inode);
void (*inode_free_security) (struct inode *inode);
int (*inode_init_security) (struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len);
int (*inode_create) (struct inode *dir,
struct dentry *dentry, umode_t mode);
@@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len);
int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
int security_inode_link(struct dentry *old_dentry, struct inode *dir,
@@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
static inline int security_old_inode_init_security(struct inode *inode,
struct inode *dir,
const struct qstr *qstr,
- char **name, void **value,
- size_t *len)
+ const char **name,
+ void **value, size_t *len)
{
return -EOPNOTSUPP;
}
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index fdbafc6..91b0a68 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -31,7 +31,7 @@ struct xattr_handler {
};
struct xattr {
- char *name;
+ const char *name;
void *value;
size_t value_len;
};
diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
index d8ce17c..38fdd64 100644
--- a/include/uapi/linux/reiserfs_xattr.h
+++ b/include/uapi/linux/reiserfs_xattr.h
@@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
};
struct reiserfs_security_handle {
- char *name;
+ const char *name;
void *value;
size_t length;
};
diff --git a/security/capability.c b/security/capability.c
index 1728d4e..b311ff0 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
}
static int cap_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
return -EOPNOTSUPP;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index df0fa45..af9b685 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -418,7 +418,7 @@ int evm_inode_init_security(struct inode *inode,
evm_xattr->value = xattr_data;
evm_xattr->value_len = sizeof(*xattr_data);
- evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
+ evm_xattr->name = XATTR_EVM_SUFFIX;
return 0;
out:
kfree(xattr_data);
diff --git a/security/security.c b/security/security.c
index a3dce87..8849691 100644
--- a/security/security.c
+++ b/security/security.c
@@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
if (unlikely(IS_PRIVATE(inode)))
return 0;
- memset(new_xattrs, 0, sizeof new_xattrs);
if (!initxattrs)
return security_ops->inode_init_security(inode, dir, qstr,
NULL, NULL, NULL);
+ memset(new_xattrs, 0, sizeof(new_xattrs));
lsm_xattr = new_xattrs;
ret = security_ops->inode_init_security(inode, dir, qstr,
&lsm_xattr->name,
@@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
goto out;
ret = initxattrs(inode, new_xattrs, fs_data);
out:
- for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
- kfree(xattr->name);
+ for (xattr = new_xattrs; xattr->value != NULL; xattr++)
kfree(xattr->value);
- }
return (ret == -EOPNOTSUPP) ? 0 : ret;
}
EXPORT_SYMBOL(security_inode_init_security);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
if (unlikely(IS_PRIVATE(inode)))
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5c6f2cd..16b8e51 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
}
static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr,
+ const char **name,
void **value, size_t *len)
{
const struct task_security_struct *tsec = current_security();
@@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
struct superblock_security_struct *sbsec;
u32 sid, newsid, clen;
int rc;
- char *namep = NULL, *context;
+ char *context;
dsec = dir->i_security;
sbsec = dir->i_sb->s_security;
@@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
return -EOPNOTSUPP;
- if (name) {
- namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
- if (!namep)
- return -ENOMEM;
- *name = namep;
- }
+ if (name)
+ *name = XATTR_SELINUX_SUFFIX;
if (value && len) {
rc = security_sid_to_context_force(newsid, &context, &clen);
- if (rc) {
- kfree(namep);
+ if (rc)
return rc;
- }
*value = context;
*len = clen;
}
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 6a08330..2dfccb7 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -582,7 +582,7 @@ static void smack_inode_free_security(struct inode *inode)
* Returns 0 if it all works out, -ENOMEM if there's no memory
*/
static int smack_inode_init_security(struct inode *inode, struct inode *dir,
- const struct qstr *qstr, char **name,
+ const struct qstr *qstr, const char **name,
void **value, size_t *len)
{
struct inode_smack *issp = inode->i_security;
@@ -591,11 +591,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
char *dsp = smk_of_inode(dir);
int may;
- if (name) {
- *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
- if (*name == NULL)
- return -ENOMEM;
- }
+ if (name)
+ *name = XATTR_SMACK_SUFFIX;
if (value) {
rcu_read_lock();
--
1.7.1
^ permalink raw reply related [flat|nested] 18+ messages in thread[parent not found: <201307231723.GBI69775.VSOLHFQFOOtJMF@I-love.SAKURA.ne.jp>]
* Re: [PATCH] xattr: Constify ->name member of "struct xattr".
[not found] ` <201307231723.GBI69775.VSOLHFQFOOtJMF@I-love.SAKURA.ne.jp>
@ 2013-07-24 19:34 ` Eric Paris
0 siblings, 0 replies; 18+ messages in thread
From: Eric Paris @ 2013-07-24 19:34 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: SE-Linux, LSM List
On Tue, Jul 23, 2013 at 4:23 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> I'm waiting for an ACK from SELinux people.
> ----------------------------------------
> >From 283a07233cfa2f64cb654f72bec86b9a61c60af0 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Tue, 23 Jul 2013 17:20:12 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
> checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reviewed-by: Joel Becker <jlbec@evilplan.org> [ocfs2]
> Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Looks fine to me.
Acked-by: Eric Paris <eparis@redhat.com>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xattr: Constify ->name member of "struct xattr".
2013-06-08 12:54 ` Tetsuo Handa
@ 2013-06-10 11:54 ` Serge Hallyn
-1 siblings, 0 replies; 18+ messages in thread
From: Serge Hallyn @ 2013-06-10 11:54 UTC (permalink / raw)
To: Tetsuo Handa
Cc: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn,
linux-fsdevel, linux-security-module
Quoting Tetsuo Handa (penguin-kernel@I-love.SAKURA.ne.jp):
> >From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 8 Jun 2013 21:46:58 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
> checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
It seems good to me.
Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> ---
> fs/ocfs2/xattr.h | 2 +-
> include/linux/security.h | 8 ++++----
> include/linux/xattr.h | 2 +-
> include/uapi/linux/reiserfs_xattr.h | 2 +-
> security/capability.c | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 8 +++-----
> security/selinux/hooks.c | 17 ++++++-----------
> security/smack/smack_lsm.c | 9 +++------
> 9 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index e5c7f15..19f134e 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
>
> struct ocfs2_security_xattr_info {
> int enable;
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 40560f4..2e64d73 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1466,7 +1466,7 @@ struct security_operations {
> int (*inode_alloc_security) (struct inode *inode);
> void (*inode_free_security) (struct inode *inode);
> int (*inode_init_security) (struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int (*inode_create) (struct inode *dir,
> struct dentry *dentry, umode_t mode);
> @@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
> int security_inode_link(struct dentry *old_dentry, struct inode *dir,
> @@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
> static inline int security_old_inode_init_security(struct inode *inode,
> struct inode *dir,
> const struct qstr *qstr,
> - char **name, void **value,
> - size_t *len)
> + const char **name,
> + void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index fdbafc6..91b0a68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -31,7 +31,7 @@ struct xattr_handler {
> };
>
> struct xattr {
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
> index d8ce17c..38fdd64 100644
> --- a/include/uapi/linux/reiserfs_xattr.h
> +++ b/include/uapi/linux/reiserfs_xattr.h
> @@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
> };
>
> struct reiserfs_security_handle {
> - char *name;
> + const char *name;
> void *value;
> size_t length;
> };
> diff --git a/security/capability.c b/security/capability.c
> index 1728d4e..b311ff0 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
> }
>
> static int cap_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index cdbde17..2787080 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
>
> evm_xattr->value = xattr_data;
> evm_xattr->value_len = sizeof(*xattr_data);
> - evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
> + evm_xattr->name = XATTR_EVM_SUFFIX;
> return 0;
> out:
> kfree(xattr_data);
> diff --git a/security/security.c b/security/security.c
> index a3dce87..8849691 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> if (unlikely(IS_PRIVATE(inode)))
> return 0;
>
> - memset(new_xattrs, 0, sizeof new_xattrs);
> if (!initxattrs)
> return security_ops->inode_init_security(inode, dir, qstr,
> NULL, NULL, NULL);
> + memset(new_xattrs, 0, sizeof(new_xattrs));
> lsm_xattr = new_xattrs;
> ret = security_ops->inode_init_security(inode, dir, qstr,
> &lsm_xattr->name,
> @@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> out:
> - for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
> - kfree(xattr->name);
> + for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> - }
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> if (unlikely(IS_PRIVATE(inode)))
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5c6f2cd..16b8e51 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
> }
>
> static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr,
> + const char **name,
> void **value, size_t *len)
> {
> const struct task_security_struct *tsec = current_security();
> @@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> struct superblock_security_struct *sbsec;
> u32 sid, newsid, clen;
> int rc;
> - char *namep = NULL, *context;
> + char *context;
>
> dsec = dir->i_security;
> sbsec = dir->i_sb->s_security;
> @@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
> return -EOPNOTSUPP;
>
> - if (name) {
> - namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
> - if (!namep)
> - return -ENOMEM;
> - *name = namep;
> - }
> + if (name)
> + *name = XATTR_SELINUX_SUFFIX;
>
> if (value && len) {
> rc = security_sid_to_context_force(newsid, &context, &clen);
> - if (rc) {
> - kfree(namep);
> + if (rc)
> return rc;
> - }
> *value = context;
> *len = clen;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..46e5b47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode *inode)
> * Returns 0 if it all works out, -ENOMEM if there's no memory
> */
> static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> struct smack_known *skp;
> @@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> char *dsp = smk_of_inode(dir);
> int may;
>
> - if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> - if (*name == NULL)
> - return -ENOMEM;
> - }
> + if (name)
> + *name = XATTR_SMACK_SUFFIX;
>
> if (value) {
> skp = smk_find_entry(csp);
> --
> 1.7.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [Ocfs2-devel] [PATCH] xattr: Constify ->name member of "struct xattr".
@ 2013-06-10 11:54 ` Serge Hallyn
0 siblings, 0 replies; 18+ messages in thread
From: Serge Hallyn @ 2013-06-10 11:54 UTC (permalink / raw)
To: Tetsuo Handa
Cc: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn,
linux-fsdevel, linux-security-module
Quoting Tetsuo Handa (penguin-kernel at I-love.SAKURA.ne.jp):
> >From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 8 Jun 2013 21:46:58 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
> checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
It seems good to me.
Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> ---
> fs/ocfs2/xattr.h | 2 +-
> include/linux/security.h | 8 ++++----
> include/linux/xattr.h | 2 +-
> include/uapi/linux/reiserfs_xattr.h | 2 +-
> security/capability.c | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 8 +++-----
> security/selinux/hooks.c | 17 ++++++-----------
> security/smack/smack_lsm.c | 9 +++------
> 9 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index e5c7f15..19f134e 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
>
> struct ocfs2_security_xattr_info {
> int enable;
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 40560f4..2e64d73 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1466,7 +1466,7 @@ struct security_operations {
> int (*inode_alloc_security) (struct inode *inode);
> void (*inode_free_security) (struct inode *inode);
> int (*inode_init_security) (struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int (*inode_create) (struct inode *dir,
> struct dentry *dentry, umode_t mode);
> @@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
> int security_inode_link(struct dentry *old_dentry, struct inode *dir,
> @@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
> static inline int security_old_inode_init_security(struct inode *inode,
> struct inode *dir,
> const struct qstr *qstr,
> - char **name, void **value,
> - size_t *len)
> + const char **name,
> + void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index fdbafc6..91b0a68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -31,7 +31,7 @@ struct xattr_handler {
> };
>
> struct xattr {
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
> index d8ce17c..38fdd64 100644
> --- a/include/uapi/linux/reiserfs_xattr.h
> +++ b/include/uapi/linux/reiserfs_xattr.h
> @@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
> };
>
> struct reiserfs_security_handle {
> - char *name;
> + const char *name;
> void *value;
> size_t length;
> };
> diff --git a/security/capability.c b/security/capability.c
> index 1728d4e..b311ff0 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
> }
>
> static int cap_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index cdbde17..2787080 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
>
> evm_xattr->value = xattr_data;
> evm_xattr->value_len = sizeof(*xattr_data);
> - evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
> + evm_xattr->name = XATTR_EVM_SUFFIX;
> return 0;
> out:
> kfree(xattr_data);
> diff --git a/security/security.c b/security/security.c
> index a3dce87..8849691 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> if (unlikely(IS_PRIVATE(inode)))
> return 0;
>
> - memset(new_xattrs, 0, sizeof new_xattrs);
> if (!initxattrs)
> return security_ops->inode_init_security(inode, dir, qstr,
> NULL, NULL, NULL);
> + memset(new_xattrs, 0, sizeof(new_xattrs));
> lsm_xattr = new_xattrs;
> ret = security_ops->inode_init_security(inode, dir, qstr,
> &lsm_xattr->name,
> @@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> out:
> - for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
> - kfree(xattr->name);
> + for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> - }
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> if (unlikely(IS_PRIVATE(inode)))
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5c6f2cd..16b8e51 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
> }
>
> static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr,
> + const char **name,
> void **value, size_t *len)
> {
> const struct task_security_struct *tsec = current_security();
> @@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> struct superblock_security_struct *sbsec;
> u32 sid, newsid, clen;
> int rc;
> - char *namep = NULL, *context;
> + char *context;
>
> dsec = dir->i_security;
> sbsec = dir->i_sb->s_security;
> @@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
> return -EOPNOTSUPP;
>
> - if (name) {
> - namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
> - if (!namep)
> - return -ENOMEM;
> - *name = namep;
> - }
> + if (name)
> + *name = XATTR_SELINUX_SUFFIX;
>
> if (value && len) {
> rc = security_sid_to_context_force(newsid, &context, &clen);
> - if (rc) {
> - kfree(namep);
> + if (rc)
> return rc;
> - }
> *value = context;
> *len = clen;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..46e5b47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode *inode)
> * Returns 0 if it all works out, -ENOMEM if there's no memory
> */
> static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> struct smack_known *skp;
> @@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> char *dsp = smk_of_inode(dir);
> int may;
>
> - if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> - if (*name == NULL)
> - return -ENOMEM;
> - }
> + if (name)
> + *name = XATTR_SMACK_SUFFIX;
>
> if (value) {
> skp = smk_find_entry(csp);
> --
> 1.7.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xattr: Constify ->name member of "struct xattr".
2013-06-08 12:54 ` Tetsuo Handa
@ 2013-06-10 15:53 ` Casey Schaufler
-1 siblings, 0 replies; 18+ messages in thread
From: Casey Schaufler @ 2013-06-10 15:53 UTC (permalink / raw)
To: Tetsuo Handa
Cc: ocfs2-devel, reiserfs-devel, eparis, zohar, serge.hallyn,
linux-fsdevel, linux-security-module, Casey Schaufler
On 6/8/2013 5:54 AM, Tetsuo Handa wrote:
> >From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 8 Jun 2013 21:46:58 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
> checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Unnecessary data duplication and memory allocation is bad.
Thanks for catching this.
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/ocfs2/xattr.h | 2 +-
> include/linux/security.h | 8 ++++----
> include/linux/xattr.h | 2 +-
> include/uapi/linux/reiserfs_xattr.h | 2 +-
> security/capability.c | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 8 +++-----
> security/selinux/hooks.c | 17 ++++++-----------
> security/smack/smack_lsm.c | 9 +++------
> 9 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index e5c7f15..19f134e 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
>
> struct ocfs2_security_xattr_info {
> int enable;
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 40560f4..2e64d73 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1466,7 +1466,7 @@ struct security_operations {
> int (*inode_alloc_security) (struct inode *inode);
> void (*inode_free_security) (struct inode *inode);
> int (*inode_init_security) (struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int (*inode_create) (struct inode *dir,
> struct dentry *dentry, umode_t mode);
> @@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
> int security_inode_link(struct dentry *old_dentry, struct inode *dir,
> @@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
> static inline int security_old_inode_init_security(struct inode *inode,
> struct inode *dir,
> const struct qstr *qstr,
> - char **name, void **value,
> - size_t *len)
> + const char **name,
> + void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index fdbafc6..91b0a68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -31,7 +31,7 @@ struct xattr_handler {
> };
>
> struct xattr {
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
> index d8ce17c..38fdd64 100644
> --- a/include/uapi/linux/reiserfs_xattr.h
> +++ b/include/uapi/linux/reiserfs_xattr.h
> @@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
> };
>
> struct reiserfs_security_handle {
> - char *name;
> + const char *name;
> void *value;
> size_t length;
> };
> diff --git a/security/capability.c b/security/capability.c
> index 1728d4e..b311ff0 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
> }
>
> static int cap_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index cdbde17..2787080 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
>
> evm_xattr->value = xattr_data;
> evm_xattr->value_len = sizeof(*xattr_data);
> - evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
> + evm_xattr->name = XATTR_EVM_SUFFIX;
> return 0;
> out:
> kfree(xattr_data);
> diff --git a/security/security.c b/security/security.c
> index a3dce87..8849691 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> if (unlikely(IS_PRIVATE(inode)))
> return 0;
>
> - memset(new_xattrs, 0, sizeof new_xattrs);
> if (!initxattrs)
> return security_ops->inode_init_security(inode, dir, qstr,
> NULL, NULL, NULL);
> + memset(new_xattrs, 0, sizeof(new_xattrs));
> lsm_xattr = new_xattrs;
> ret = security_ops->inode_init_security(inode, dir, qstr,
> &lsm_xattr->name,
> @@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> out:
> - for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
> - kfree(xattr->name);
> + for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> - }
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> if (unlikely(IS_PRIVATE(inode)))
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5c6f2cd..16b8e51 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
> }
>
> static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr,
> + const char **name,
> void **value, size_t *len)
> {
> const struct task_security_struct *tsec = current_security();
> @@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> struct superblock_security_struct *sbsec;
> u32 sid, newsid, clen;
> int rc;
> - char *namep = NULL, *context;
> + char *context;
>
> dsec = dir->i_security;
> sbsec = dir->i_sb->s_security;
> @@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
> return -EOPNOTSUPP;
>
> - if (name) {
> - namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
> - if (!namep)
> - return -ENOMEM;
> - *name = namep;
> - }
> + if (name)
> + *name = XATTR_SELINUX_SUFFIX;
>
> if (value && len) {
> rc = security_sid_to_context_force(newsid, &context, &clen);
> - if (rc) {
> - kfree(namep);
> + if (rc)
> return rc;
> - }
> *value = context;
> *len = clen;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..46e5b47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode *inode)
> * Returns 0 if it all works out, -ENOMEM if there's no memory
> */
> static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> struct smack_known *skp;
> @@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> char *dsp = smk_of_inode(dir);
> int may;
>
> - if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> - if (*name == NULL)
> - return -ENOMEM;
> - }
> + if (name)
> + *name = XATTR_SMACK_SUFFIX;
>
> if (value) {
> skp = smk_find_entry(csp);
^ permalink raw reply [flat|nested] 18+ messages in thread* [Ocfs2-devel] [PATCH] xattr: Constify ->name member of "struct xattr".
@ 2013-06-10 15:53 ` Casey Schaufler
0 siblings, 0 replies; 18+ messages in thread
From: Casey Schaufler @ 2013-06-10 15:53 UTC (permalink / raw)
To: Tetsuo Handa
Cc: ocfs2-devel, reiserfs-devel, eparis, zohar, serge.hallyn,
linux-fsdevel, linux-security-module, Casey Schaufler
On 6/8/2013 5:54 AM, Tetsuo Handa wrote:
> >From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 8 Jun 2013 21:46:58 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its failure
> checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Unnecessary data duplication and memory allocation is bad.
Thanks for catching this.
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/ocfs2/xattr.h | 2 +-
> include/linux/security.h | 8 ++++----
> include/linux/xattr.h | 2 +-
> include/uapi/linux/reiserfs_xattr.h | 2 +-
> security/capability.c | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 8 +++-----
> security/selinux/hooks.c | 17 ++++++-----------
> security/smack/smack_lsm.c | 9 +++------
> 9 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index e5c7f15..19f134e 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
>
> struct ocfs2_security_xattr_info {
> int enable;
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 40560f4..2e64d73 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1466,7 +1466,7 @@ struct security_operations {
> int (*inode_alloc_security) (struct inode *inode);
> void (*inode_free_security) (struct inode *inode);
> int (*inode_init_security) (struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int (*inode_create) (struct inode *dir,
> struct dentry *dentry, umode_t mode);
> @@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode);
> int security_inode_link(struct dentry *old_dentry, struct inode *dir,
> @@ -2048,8 +2048,8 @@ static inline int security_inode_init_security(struct inode *inode,
> static inline int security_old_inode_init_security(struct inode *inode,
> struct inode *dir,
> const struct qstr *qstr,
> - char **name, void **value,
> - size_t *len)
> + const char **name,
> + void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index fdbafc6..91b0a68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -31,7 +31,7 @@ struct xattr_handler {
> };
>
> struct xattr {
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/uapi/linux/reiserfs_xattr.h b/include/uapi/linux/reiserfs_xattr.h
> index d8ce17c..38fdd64 100644
> --- a/include/uapi/linux/reiserfs_xattr.h
> +++ b/include/uapi/linux/reiserfs_xattr.h
> @@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
> };
>
> struct reiserfs_security_handle {
> - char *name;
> + const char *name;
> void *value;
> size_t length;
> };
> diff --git a/security/capability.c b/security/capability.c
> index 1728d4e..b311ff0 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
> }
>
> static int cap_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index cdbde17..2787080 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
>
> evm_xattr->value = xattr_data;
> evm_xattr->value_len = sizeof(*xattr_data);
> - evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
> + evm_xattr->name = XATTR_EVM_SUFFIX;
> return 0;
> out:
> kfree(xattr_data);
> diff --git a/security/security.c b/security/security.c
> index a3dce87..8849691 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> if (unlikely(IS_PRIVATE(inode)))
> return 0;
>
> - memset(new_xattrs, 0, sizeof new_xattrs);
> if (!initxattrs)
> return security_ops->inode_init_security(inode, dir, qstr,
> NULL, NULL, NULL);
> + memset(new_xattrs, 0, sizeof(new_xattrs));
> lsm_xattr = new_xattrs;
> ret = security_ops->inode_init_security(inode, dir, qstr,
> &lsm_xattr->name,
> @@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> out:
> - for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
> - kfree(xattr->name);
> + for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> - }
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> int security_old_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> if (unlikely(IS_PRIVATE(inode)))
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5c6f2cd..16b8e51 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode *inode)
> }
>
> static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr,
> + const char **name,
> void **value, size_t *len)
> {
> const struct task_security_struct *tsec = current_security();
> @@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> struct superblock_security_struct *sbsec;
> u32 sid, newsid, clen;
> int rc;
> - char *namep = NULL, *context;
> + char *context;
>
> dsec = dir->i_security;
> sbsec = dir->i_sb->s_security;
> @@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> if (!ss_initialized || !(sbsec->flags & SE_SBLABELSUPP))
> return -EOPNOTSUPP;
>
> - if (name) {
> - namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
> - if (!namep)
> - return -ENOMEM;
> - *name = namep;
> - }
> + if (name)
> + *name = XATTR_SELINUX_SUFFIX;
>
> if (value && len) {
> rc = security_sid_to_context_force(newsid, &context, &clen);
> - if (rc) {
> - kfree(namep);
> + if (rc)
> return rc;
> - }
> *value = context;
> *len = clen;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..46e5b47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode *inode)
> * Returns 0 if it all works out, -ENOMEM if there's no memory
> */
> static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> struct smack_known *skp;
> @@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
> char *dsp = smk_of_inode(dir);
> int may;
>
> - if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> - if (*name == NULL)
> - return -ENOMEM;
> - }
> + if (name)
> + *name = XATTR_SMACK_SUFFIX;
>
> if (value) {
> skp = smk_find_entry(csp);
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] xattr: Constify ->name member of "struct xattr".
2013-06-08 12:54 ` Tetsuo Handa
(?)
@ 2013-07-23 18:27 ` Paul Moore
-1 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2013-07-23 18:27 UTC (permalink / raw)
To: Tetsuo Handa, linux-fsdevel, linux-security-module, selinux
Cc: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn
On Saturday, June 08, 2013 09:54:56 PM Tetsuo Handa wrote:
> >From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
>
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 8 Jun 2013 21:46:58 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its
> failure checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[NOTE: Added the SELinux list to the To/CC line.]
I looked over the patch and gave it a quick test on my system, everything
seems reasonable to me.
Reviewed-by: Paul Moore <paul@paul-moore.com>
Tested-by: Paul Moore <paul@paul-moore.com>
> ---
> fs/ocfs2/xattr.h | 2 +-
> include/linux/security.h | 8 ++++----
> include/linux/xattr.h | 2 +-
> include/uapi/linux/reiserfs_xattr.h | 2 +-
> security/capability.c | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 8 +++-----
> security/selinux/hooks.c | 17 ++++++-----------
> security/smack/smack_lsm.c | 9 +++------
> 9 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index e5c7f15..19f134e 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
>
> struct ocfs2_security_xattr_info {
> int enable;
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 40560f4..2e64d73 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1466,7 +1466,7 @@ struct security_operations {
> int (*inode_alloc_security) (struct inode *inode);
> void (*inode_free_security) (struct inode *inode);
> int (*inode_init_security) (struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int (*inode_create) (struct inode *dir,
> struct dentry *dentry, umode_t mode);
> @@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode,
> struct inode *dir, const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> int security_old_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t
> mode); int security_inode_link(struct dentry *old_dentry, struct inode
> *dir, @@ -2048,8 +2048,8 @@ static inline int
> security_inode_init_security(struct inode *inode, static inline int
> security_old_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> - char **name, void **value,
> - size_t *len)
> + const char **name,
> + void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index fdbafc6..91b0a68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -31,7 +31,7 @@ struct xattr_handler {
> };
>
> struct xattr {
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/uapi/linux/reiserfs_xattr.h
> b/include/uapi/linux/reiserfs_xattr.h index d8ce17c..38fdd64 100644
> --- a/include/uapi/linux/reiserfs_xattr.h
> +++ b/include/uapi/linux/reiserfs_xattr.h
> @@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
> };
>
> struct reiserfs_security_handle {
> - char *name;
> + const char *name;
> void *value;
> size_t length;
> };
> diff --git a/security/capability.c b/security/capability.c
> index 1728d4e..b311ff0 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
> }
>
> static int cap_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c index cdbde17..2787080 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
>
> evm_xattr->value = xattr_data;
> evm_xattr->value_len = sizeof(*xattr_data);
> - evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
> + evm_xattr->name = XATTR_EVM_SUFFIX;
> return 0;
> out:
> kfree(xattr_data);
> diff --git a/security/security.c b/security/security.c
> index a3dce87..8849691 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode,
> struct inode *dir, if (unlikely(IS_PRIVATE(inode)))
> return 0;
>
> - memset(new_xattrs, 0, sizeof new_xattrs);
> if (!initxattrs)
> return security_ops->inode_init_security(inode, dir, qstr,
> NULL, NULL, NULL);
> + memset(new_xattrs, 0, sizeof(new_xattrs));
> lsm_xattr = new_xattrs;
> ret = security_ops->inode_init_security(inode, dir, qstr,
> &lsm_xattr->name,
> @@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode,
> struct inode *dir, goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> out:
> - for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
> - kfree(xattr->name);
> + for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> - }
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> int security_old_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> if (unlikely(IS_PRIVATE(inode)))
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5c6f2cd..16b8e51 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode
> *inode) }
>
> static int selinux_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr,
> + const char **name,
> void **value, size_t *len)
> {
> const struct task_security_struct *tsec = current_security();
> @@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode
> *inode, struct inode *dir, struct superblock_security_struct *sbsec;
> u32 sid, newsid, clen;
> int rc;
> - char *namep = NULL, *context;
> + char *context;
>
> dsec = dir->i_security;
> sbsec = dir->i_sb->s_security;
> @@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode
> *inode, struct inode *dir, if (!ss_initialized || !(sbsec->flags &
> SE_SBLABELSUPP))
> return -EOPNOTSUPP;
>
> - if (name) {
> - namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
> - if (!namep)
> - return -ENOMEM;
> - *name = namep;
> - }
> + if (name)
> + *name = XATTR_SELINUX_SUFFIX;
>
> if (value && len) {
> rc = security_sid_to_context_force(newsid, &context, &clen);
> - if (rc) {
> - kfree(namep);
> + if (rc)
> return rc;
> - }
> *value = context;
> *len = clen;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..46e5b47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode
> *inode) * Returns 0 if it all works out, -ENOMEM if there's no memory
> */
> static int smack_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> struct smack_known *skp;
> @@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode
> *inode, struct inode *dir, char *dsp = smk_of_inode(dir);
> int may;
>
> - if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> - if (*name == NULL)
> - return -ENOMEM;
> - }
> + if (name)
> + *name = XATTR_SMACK_SUFFIX;
>
> if (value) {
> skp = smk_find_entry(csp);
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 18+ messages in thread* [Ocfs2-devel] [PATCH] xattr: Constify ->name member of "struct xattr".
@ 2013-07-23 18:27 ` Paul Moore
0 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2013-07-23 18:27 UTC (permalink / raw)
To: Tetsuo Handa, linux-fsdevel, linux-security-module, selinux
Cc: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn
On Saturday, June 08, 2013 09:54:56 PM Tetsuo Handa wrote:
> >From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
>
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 8 Jun 2013 21:46:58 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its
> failure checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[NOTE: Added the SELinux list to the To/CC line.]
I looked over the patch and gave it a quick test on my system, everything
seems reasonable to me.
Reviewed-by: Paul Moore <paul@paul-moore.com>
Tested-by: Paul Moore <paul@paul-moore.com>
> ---
> fs/ocfs2/xattr.h | 2 +-
> include/linux/security.h | 8 ++++----
> include/linux/xattr.h | 2 +-
> include/uapi/linux/reiserfs_xattr.h | 2 +-
> security/capability.c | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 8 +++-----
> security/selinux/hooks.c | 17 ++++++-----------
> security/smack/smack_lsm.c | 9 +++------
> 9 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index e5c7f15..19f134e 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
>
> struct ocfs2_security_xattr_info {
> int enable;
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 40560f4..2e64d73 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1466,7 +1466,7 @@ struct security_operations {
> int (*inode_alloc_security) (struct inode *inode);
> void (*inode_free_security) (struct inode *inode);
> int (*inode_init_security) (struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int (*inode_create) (struct inode *dir,
> struct dentry *dentry, umode_t mode);
> @@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode,
> struct inode *dir, const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> int security_old_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t
> mode); int security_inode_link(struct dentry *old_dentry, struct inode
> *dir, @@ -2048,8 +2048,8 @@ static inline int
> security_inode_init_security(struct inode *inode, static inline int
> security_old_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> - char **name, void **value,
> - size_t *len)
> + const char **name,
> + void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index fdbafc6..91b0a68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -31,7 +31,7 @@ struct xattr_handler {
> };
>
> struct xattr {
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/uapi/linux/reiserfs_xattr.h
> b/include/uapi/linux/reiserfs_xattr.h index d8ce17c..38fdd64 100644
> --- a/include/uapi/linux/reiserfs_xattr.h
> +++ b/include/uapi/linux/reiserfs_xattr.h
> @@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
> };
>
> struct reiserfs_security_handle {
> - char *name;
> + const char *name;
> void *value;
> size_t length;
> };
> diff --git a/security/capability.c b/security/capability.c
> index 1728d4e..b311ff0 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
> }
>
> static int cap_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c index cdbde17..2787080 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
>
> evm_xattr->value = xattr_data;
> evm_xattr->value_len = sizeof(*xattr_data);
> - evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
> + evm_xattr->name = XATTR_EVM_SUFFIX;
> return 0;
> out:
> kfree(xattr_data);
> diff --git a/security/security.c b/security/security.c
> index a3dce87..8849691 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode,
> struct inode *dir, if (unlikely(IS_PRIVATE(inode)))
> return 0;
>
> - memset(new_xattrs, 0, sizeof new_xattrs);
> if (!initxattrs)
> return security_ops->inode_init_security(inode, dir, qstr,
> NULL, NULL, NULL);
> + memset(new_xattrs, 0, sizeof(new_xattrs));
> lsm_xattr = new_xattrs;
> ret = security_ops->inode_init_security(inode, dir, qstr,
> &lsm_xattr->name,
> @@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode,
> struct inode *dir, goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> out:
> - for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
> - kfree(xattr->name);
> + for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> - }
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> int security_old_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> if (unlikely(IS_PRIVATE(inode)))
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5c6f2cd..16b8e51 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode
> *inode) }
>
> static int selinux_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr,
> + const char **name,
> void **value, size_t *len)
> {
> const struct task_security_struct *tsec = current_security();
> @@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode
> *inode, struct inode *dir, struct superblock_security_struct *sbsec;
> u32 sid, newsid, clen;
> int rc;
> - char *namep = NULL, *context;
> + char *context;
>
> dsec = dir->i_security;
> sbsec = dir->i_sb->s_security;
> @@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode
> *inode, struct inode *dir, if (!ss_initialized || !(sbsec->flags &
> SE_SBLABELSUPP))
> return -EOPNOTSUPP;
>
> - if (name) {
> - namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
> - if (!namep)
> - return -ENOMEM;
> - *name = namep;
> - }
> + if (name)
> + *name = XATTR_SELINUX_SUFFIX;
>
> if (value && len) {
> rc = security_sid_to_context_force(newsid, &context, &clen);
> - if (rc) {
> - kfree(namep);
> + if (rc)
> return rc;
> - }
> *value = context;
> *len = clen;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..46e5b47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode
> *inode) * Returns 0 if it all works out, -ENOMEM if there's no memory
> */
> static int smack_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> struct smack_known *skp;
> @@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode
> *inode, struct inode *dir, char *dsp = smk_of_inode(dir);
> int may;
>
> - if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> - if (*name == NULL)
> - return -ENOMEM;
> - }
> + if (name)
> + *name = XATTR_SMACK_SUFFIX;
>
> if (value) {
> skp = smk_find_entry(csp);
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] xattr: Constify ->name member of "struct xattr".
@ 2013-07-23 18:27 ` Paul Moore
0 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2013-07-23 18:27 UTC (permalink / raw)
To: Tetsuo Handa, linux-fsdevel, linux-security-module, selinux
Cc: ocfs2-devel, reiserfs-devel, eparis, casey, zohar, serge.hallyn
On Saturday, June 08, 2013 09:54:56 PM Tetsuo Handa wrote:
> >From 96df8b4be7f8702913a0245b2312958e2eea6caf Mon Sep 17 00:00:00 2001
>
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Sat, 8 Jun 2013 21:46:58 +0900
> Subject: [PATCH] xattr: Constify ->name member of "struct xattr".
>
> Since everybody sets kstrdup()ed constant string to "struct xattr"->name but
> nobody modifies "struct xattr"->name , we can omit kstrdup() and its
> failure checking by constifying ->name member of "struct xattr".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[NOTE: Added the SELinux list to the To/CC line.]
I looked over the patch and gave it a quick test on my system, everything
seems reasonable to me.
Reviewed-by: Paul Moore <paul@paul-moore.com>
Tested-by: Paul Moore <paul@paul-moore.com>
> ---
> fs/ocfs2/xattr.h | 2 +-
> include/linux/security.h | 8 ++++----
> include/linux/xattr.h | 2 +-
> include/uapi/linux/reiserfs_xattr.h | 2 +-
> security/capability.c | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 8 +++-----
> security/selinux/hooks.c | 17 ++++++-----------
> security/smack/smack_lsm.c | 9 +++------
> 9 files changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index e5c7f15..19f134e 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -32,7 +32,7 @@ enum ocfs2_xattr_type {
>
> struct ocfs2_security_xattr_info {
> int enable;
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 40560f4..2e64d73 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1466,7 +1466,7 @@ struct security_operations {
> int (*inode_alloc_security) (struct inode *inode);
> void (*inode_free_security) (struct inode *inode);
> int (*inode_init_security) (struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int (*inode_create) (struct inode *dir,
> struct dentry *dentry, umode_t mode);
> @@ -1737,7 +1737,7 @@ int security_inode_init_security(struct inode *inode,
> struct inode *dir, const struct qstr *qstr,
> initxattrs initxattrs, void *fs_data);
> int security_old_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len);
> int security_inode_create(struct inode *dir, struct dentry *dentry, umode_t
> mode); int security_inode_link(struct dentry *old_dentry, struct inode
> *dir, @@ -2048,8 +2048,8 @@ static inline int
> security_inode_init_security(struct inode *inode, static inline int
> security_old_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> - char **name, void **value,
> - size_t *len)
> + const char **name,
> + void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index fdbafc6..91b0a68 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -31,7 +31,7 @@ struct xattr_handler {
> };
>
> struct xattr {
> - char *name;
> + const char *name;
> void *value;
> size_t value_len;
> };
> diff --git a/include/uapi/linux/reiserfs_xattr.h
> b/include/uapi/linux/reiserfs_xattr.h index d8ce17c..38fdd64 100644
> --- a/include/uapi/linux/reiserfs_xattr.h
> +++ b/include/uapi/linux/reiserfs_xattr.h
> @@ -16,7 +16,7 @@ struct reiserfs_xattr_header {
> };
>
> struct reiserfs_security_handle {
> - char *name;
> + const char *name;
> void *value;
> size_t length;
> };
> diff --git a/security/capability.c b/security/capability.c
> index 1728d4e..b311ff0 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -119,7 +119,7 @@ static void cap_inode_free_security(struct inode *inode)
> }
>
> static int cap_inode_init_security(struct inode *inode, struct inode *dir,
> - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> return -EOPNOTSUPP;
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c index cdbde17..2787080 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -405,7 +405,7 @@ int evm_inode_init_security(struct inode *inode,
>
> evm_xattr->value = xattr_data;
> evm_xattr->value_len = sizeof(*xattr_data);
> - evm_xattr->name = kstrdup(XATTR_EVM_SUFFIX, GFP_NOFS);
> + evm_xattr->name = XATTR_EVM_SUFFIX;
> return 0;
> out:
> kfree(xattr_data);
> diff --git a/security/security.c b/security/security.c
> index a3dce87..8849691 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -335,10 +335,10 @@ int security_inode_init_security(struct inode *inode,
> struct inode *dir, if (unlikely(IS_PRIVATE(inode)))
> return 0;
>
> - memset(new_xattrs, 0, sizeof new_xattrs);
> if (!initxattrs)
> return security_ops->inode_init_security(inode, dir, qstr,
> NULL, NULL, NULL);
> + memset(new_xattrs, 0, sizeof(new_xattrs));
> lsm_xattr = new_xattrs;
> ret = security_ops->inode_init_security(inode, dir, qstr,
> &lsm_xattr->name,
> @@ -353,16 +353,14 @@ int security_inode_init_security(struct inode *inode,
> struct inode *dir, goto out;
> ret = initxattrs(inode, new_xattrs, fs_data);
> out:
> - for (xattr = new_xattrs; xattr->name != NULL; xattr++) {
> - kfree(xattr->name);
> + for (xattr = new_xattrs; xattr->value != NULL; xattr++)
> kfree(xattr->value);
> - }
> return (ret == -EOPNOTSUPP) ? 0 : ret;
> }
> EXPORT_SYMBOL(security_inode_init_security);
>
> int security_old_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> if (unlikely(IS_PRIVATE(inode)))
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5c6f2cd..16b8e51 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2516,7 +2516,8 @@ static void selinux_inode_free_security(struct inode
> *inode) }
>
> static int selinux_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr,
> + const char **name,
> void **value, size_t *len)
> {
> const struct task_security_struct *tsec = current_security();
> @@ -2524,7 +2525,7 @@ static int selinux_inode_init_security(struct inode
> *inode, struct inode *dir, struct superblock_security_struct *sbsec;
> u32 sid, newsid, clen;
> int rc;
> - char *namep = NULL, *context;
> + char *context;
>
> dsec = dir->i_security;
> sbsec = dir->i_sb->s_security;
> @@ -2560,19 +2561,13 @@ static int selinux_inode_init_security(struct inode
> *inode, struct inode *dir, if (!ss_initialized || !(sbsec->flags &
> SE_SBLABELSUPP))
> return -EOPNOTSUPP;
>
> - if (name) {
> - namep = kstrdup(XATTR_SELINUX_SUFFIX, GFP_NOFS);
> - if (!namep)
> - return -ENOMEM;
> - *name = namep;
> - }
> + if (name)
> + *name = XATTR_SELINUX_SUFFIX;
>
> if (value && len) {
> rc = security_sid_to_context_force(newsid, &context, &clen);
> - if (rc) {
> - kfree(namep);
> + if (rc)
> return rc;
> - }
> *value = context;
> *len = clen;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..46e5b47 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -554,7 +554,7 @@ static void smack_inode_free_security(struct inode
> *inode) * Returns 0 if it all works out, -ENOMEM if there's no memory
> */
> static int smack_inode_init_security(struct inode *inode, struct inode
> *dir, - const struct qstr *qstr, char **name,
> + const struct qstr *qstr, const char **name,
> void **value, size_t *len)
> {
> struct smack_known *skp;
> @@ -564,11 +564,8 @@ static int smack_inode_init_security(struct inode
> *inode, struct inode *dir, char *dsp = smk_of_inode(dir);
> int may;
>
> - if (name) {
> - *name = kstrdup(XATTR_SMACK_SUFFIX, GFP_NOFS);
> - if (*name == NULL)
> - return -ENOMEM;
> - }
> + if (name)
> + *name = XATTR_SMACK_SUFFIX;
>
> if (value) {
> skp = smk_find_entry(csp);
--
paul moore
www.paul-moore.com
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.
^ permalink raw reply [flat|nested] 18+ messages in thread