2009-12-04 20:47:45

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 01/15] shmem: do not call fput_filp on an initialized filp

fput_filp is supposed to be used when the filp was not used. But in the
ifndef CONFIG_MMU case shmem_setup_file could call this one an initialized
filp. It should be using fput() instead. Since the fput() will dec the ima
counts we also need to move the ima hook to make sure that is set up before
the fput().

Signed-off-by: Eric Paris <[email protected]>
Acked-by: Miklos Szeredi <[email protected]>
---

mm/shmem.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 356dd99..e7f8968 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2656,12 +2656,15 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
&shmem_file_operations);

+ ima_counts_get(file);
+
#ifndef CONFIG_MMU
error = ramfs_nommu_expand_for_mapping(inode, size);
- if (error)
- goto close_file;
+ if (error) {
+ fput(file);
+ return error;
+ }
#endif
- ima_counts_get(file);
return file;

close_file:


2009-12-04 20:48:03

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 02/15] shmem: use alloc_file instead of init_file

shmem uses get_empty_filp() and then init_file(). Their is no good reason
not to just use alloc_file() like everything else.

Acked-by: Miklos Szeredi <[email protected]>
Signed-off-by: Eric Paris <[email protected]>
---

mm/shmem.c | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index e7f8968..b212184 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2640,21 +2640,20 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
if (!dentry)
goto put_memory;

- error = -ENFILE;
- file = get_empty_filp();
- if (!file)
- goto put_dentry;
-
error = -ENOSPC;
inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
if (!inode)
- goto close_file;
+ goto put_dentry;

d_instantiate(dentry, inode);
inode->i_size = size;
inode->i_nlink = 0; /* It is unlinked */
- init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
- &shmem_file_operations);
+
+ error = -ENFILE;
+ file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
+ &shmem_file_operations);
+ if (!file)
+ goto put_dentry;

ima_counts_get(file);

@@ -2667,8 +2666,6 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
#endif
return file;

-close_file:
- put_filp(file);
put_dentry:
dput(dentry);
put_memory:

2009-12-04 20:48:14

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 03/15] pipes: use alloc-file instead of duplicating code

The pipe code duplicates the functionality of alloc-file and init-file. Use
the generic vfs functions instead of duplicating code.

Signed-off-by: Eric Paris <[email protected]>
Acked-by: Miklos Szeredi <[email protected]>
---

fs/pipe.c | 21 +++++++++------------
1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index ae17d02..5d6c969 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1028,20 +1028,17 @@ void free_write_pipe(struct file *f)

struct file *create_read_pipe(struct file *wrf, int flags)
{
- struct file *f = get_empty_filp();
- if (!f)
- return ERR_PTR(-ENFILE);
-
- /* Grab pipe from the writer */
- f->f_path = wrf->f_path;
- path_get(&wrf->f_path);
- f->f_mapping = wrf->f_path.dentry->d_inode->i_mapping;
+ struct file *f;
+ struct dentry *dentry = wrf->f_path.dentry;
+ struct vfsmount *mnt = wrf->f_path.mnt;

- f->f_pos = 0;
+ dentry = dget(dentry);
+ f = alloc_file(mnt, dentry, FMODE_READ, &read_pipefifo_fops);
+ if (!f) {
+ dput(dentry);
+ return ERR_PTR(-ENFILE);
+ }
f->f_flags = O_RDONLY | (flags & O_NONBLOCK);
- f->f_op = &read_pipefifo_fops;
- f->f_mode = FMODE_READ;
- f->f_version = 0;

return f;
}

2009-12-04 20:47:55

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 04/15] inotify: use alloc_file instead of doing it internally

inotify basically duplicates everything from alloc-file and init-file. Use
the generic vfs functions instead.

Signed-off-by: Eric Paris <[email protected]>
Acked-by: Miklos Szeredi <[email protected]>
---

fs/notify/inotify/inotify_user.c | 23 +++++++++--------------
1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index c40894a..3e03803 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -725,6 +725,7 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
struct fsnotify_group *group;
struct user_struct *user;
struct file *filp;
+ struct dentry *dentry;
int fd, ret;

/* Check the IN_* constants for consistency. */
@@ -738,12 +739,6 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
if (fd < 0)
return fd;

- filp = get_empty_filp();
- if (!filp) {
- ret = -ENFILE;
- goto out_put_fd;
- }
-
user = get_current_user();
if (unlikely(atomic_read(&user->inotify_devs) >=
inotify_max_user_instances)) {
@@ -758,11 +753,12 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
goto out_free_uid;
}

- filp->f_op = &inotify_fops;
- filp->f_path.mnt = mntget(inotify_mnt);
- filp->f_path.dentry = dget(inotify_mnt->mnt_root);
- filp->f_mapping = filp->f_path.dentry->d_inode->i_mapping;
- filp->f_mode = FMODE_READ;
+ dentry = dget(inotify_mnt->mnt_root);
+ filp = alloc_file(inotify_mnt, dentry, FMODE_READ, &inotify_fops);
+ if (!filp) {
+ ret = -ENFILE;
+ goto out_dput;
+ }
filp->f_flags = O_RDONLY | (flags & O_NONBLOCK);
filp->private_data = group;

@@ -771,11 +767,10 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
fd_install(fd, filp);

return fd;
-
+out_dput:
+ dput(dentry);
out_free_uid:
free_uid(user);
- put_filp(filp);
-out_put_fd:
put_unused_fd(fd);
return ret;
}

2009-12-04 20:48:10

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 05/15] networking: rework socket to fd mapping using alloc-file

Currently the networking code does interesting things allocating its struct
file and file descriptors. This patch attempts to unify all of that and
simplify the error paths. It is also a part of my patch series trying to get
rid of init-file and get-empty_filp and friends.

Signed-off-by: Eric Paris <[email protected]>
Acked-by: David S. Miller <[email protected]>
Acked-by: Miklos Szeredi <[email protected]>
---

net/socket.c | 123 ++++++++++++++++++++++------------------------------------
1 files changed, 46 insertions(+), 77 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index b94c3dd..1a17279 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -355,32 +355,24 @@ static const struct dentry_operations sockfs_dentry_operations = {
* but we take care of internal coherence yet.
*/

-static int sock_alloc_fd(struct file **filep, int flags)
+static int sock_alloc_fd(struct file **filep, struct socket *sock, int flags)
{
- int fd;
-
- fd = get_unused_fd_flags(flags);
- if (likely(fd >= 0)) {
- struct file *file = get_empty_filp();
-
- *filep = file;
- if (unlikely(!file)) {
- put_unused_fd(fd);
- return -ENFILE;
- }
- } else
- *filep = NULL;
- return fd;
-}
-
-static int sock_attach_fd(struct socket *sock, struct file *file, int flags)
-{
- struct dentry *dentry;
+ int fd, rc;
+ struct file *file;
+ struct dentry *dentry = NULL;
struct qstr name = { .name = "" };

+ fd = get_unused_fd_flags(flags & O_CLOEXEC);
+ if (unlikely(fd < 0)) {
+ rc = fd;
+ goto out;
+ }
+
dentry = d_alloc(sock_mnt->mnt_sb->s_root, &name);
- if (unlikely(!dentry))
- return -ENOMEM;
+ if (unlikely(!dentry)) {
+ rc = -ENOMEM;
+ goto out_fd;
+ }

dentry->d_op = &sockfs_dentry_operations;
/*
@@ -391,32 +383,38 @@ static int sock_attach_fd(struct socket *sock, struct file *file, int flags)
dentry->d_flags &= ~DCACHE_UNHASHED;
d_instantiate(dentry, SOCK_INODE(sock));

+ file = alloc_file(sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
+ &socket_file_ops);
+ if (unlikely(!file)) {
+ rc = -ENFILE;
+ goto out_dentry;
+ }
+
sock->file = file;
- init_file(file, sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
- &socket_file_ops);
SOCK_INODE(sock)->i_fop = &socket_file_ops;
file->f_flags = O_RDWR | (flags & O_NONBLOCK);
- file->f_pos = 0;
file->private_data = sock;
+ *filep = file;

- return 0;
+ return fd;
+out_dentry:
+ dput(dentry);
+out_fd:
+ put_unused_fd(fd);
+out:
+ *filep = NULL;
+ return rc;
}

int sock_map_fd(struct socket *sock, int flags)
{
struct file *newfile;
- int fd = sock_alloc_fd(&newfile, flags);
-
- if (likely(fd >= 0)) {
- int err = sock_attach_fd(sock, newfile, flags);
+ int fd;

- if (unlikely(err < 0)) {
- put_filp(newfile);
- put_unused_fd(fd);
- return err;
- }
+ fd = sock_alloc_fd(&newfile, sock, flags);
+ if (likely(fd >= 0))
fd_install(fd, newfile);
- }
+
return fd;
}

@@ -1384,35 +1382,22 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,

err = sock_create(family, type, protocol, &sock2);
if (err < 0)
- goto out_release_1;
+ goto out_release_sock_1;

err = sock1->ops->socketpair(sock1, sock2);
if (err < 0)
- goto out_release_both;
+ goto out_release_sock_2;

- fd1 = sock_alloc_fd(&newfile1, flags & O_CLOEXEC);
+ fd1 = sock_alloc_fd(&newfile1, sock1, flags & (O_CLOEXEC | O_NONBLOCK));
if (unlikely(fd1 < 0)) {
err = fd1;
- goto out_release_both;
+ goto out_release_sock_2;
}

- fd2 = sock_alloc_fd(&newfile2, flags & O_CLOEXEC);
+ fd2 = sock_alloc_fd(&newfile2, sock2, flags & (O_CLOEXEC | O_NONBLOCK));
if (unlikely(fd2 < 0)) {
err = fd2;
- put_filp(newfile1);
- put_unused_fd(fd1);
- goto out_release_both;
- }
-
- err = sock_attach_fd(sock1, newfile1, flags & O_NONBLOCK);
- if (unlikely(err < 0)) {
- goto out_fd2;
- }
-
- err = sock_attach_fd(sock2, newfile2, flags & O_NONBLOCK);
- if (unlikely(err < 0)) {
- fput(newfile1);
- goto out_fd1;
+ goto out_release_fd_1;
}

audit_fd_pair(fd1, fd2);
@@ -1432,22 +1417,15 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
sys_close(fd1);
return err;

-out_release_both:
+out_release_fd_1:
+ fput(newfile1);
+ put_unused_fd(fd1);
+out_release_sock_2:
sock_release(sock2);
-out_release_1:
+out_release_sock_1:
sock_release(sock1);
out:
return err;
-
-out_fd2:
- put_filp(newfile1);
- sock_release(sock1);
-out_fd1:
- put_filp(newfile2);
- sock_release(sock2);
- put_unused_fd(fd1);
- put_unused_fd(fd2);
- goto out;
}

/*
@@ -1551,17 +1529,13 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
*/
__module_get(newsock->ops->owner);

- newfd = sock_alloc_fd(&newfile, flags & O_CLOEXEC);
+ newfd = sock_alloc_fd(&newfile, newsock, flags & (O_CLOEXEC | O_NONBLOCK));
if (unlikely(newfd < 0)) {
err = newfd;
sock_release(newsock);
goto out_put;
}

- err = sock_attach_fd(newsock, newfile, flags & O_NONBLOCK);
- if (err < 0)
- goto out_fd_simple;
-
err = security_socket_accept(sock, newsock);
if (err)
goto out_fd;
@@ -1591,11 +1565,6 @@ out_put:
fput_light(sock->file, fput_needed);
out:
return err;
-out_fd_simple:
- sock_release(newsock);
- put_filp(newfile);
- put_unused_fd(newfd);
- goto out_put;
out_fd:
fput(newfile);
put_unused_fd(newfd);

2009-12-04 20:48:21

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 06/15] vfs: make init-file static

init-file is no longer used by anything except alloc_file. Make it static and
remove from headers.

Signed-off-by: Eric Paris <[email protected]>
Acked-by: Miklos Szeredi <[email protected]>
---

fs/file_table.c | 73 ++++++++++++++++++++++----------------------------
include/linux/file.h | 3 --
2 files changed, 32 insertions(+), 44 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 4bef4c0..0f9d2f2 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -150,53 +150,16 @@ fail:
EXPORT_SYMBOL(get_empty_filp);

/**
- * alloc_file - allocate and initialize a 'struct file'
- * @mnt: the vfsmount on which the file will reside
- * @dentry: the dentry representing the new file
- * @mode: the mode with which the new file will be opened
- * @fop: the 'struct file_operations' for the new file
- *
- * Use this instead of get_empty_filp() to get a new
- * 'struct file'. Do so because of the same initialization
- * pitfalls reasons listed for init_file(). This is a
- * preferred interface to using init_file().
- *
- * If all the callers of init_file() are eliminated, its
- * code should be moved into this function.
- */
-struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
- fmode_t mode, const struct file_operations *fop)
-{
- struct file *file;
-
- file = get_empty_filp();
- if (!file)
- return NULL;
-
- init_file(file, mnt, dentry, mode, fop);
- return file;
-}
-EXPORT_SYMBOL(alloc_file);
-
-/**
* init_file - initialize a 'struct file'
* @file: the already allocated 'struct file' to initialized
* @mnt: the vfsmount on which the file resides
* @dentry: the dentry representing this file
* @mode: the mode the file is opened with
* @fop: the 'struct file_operations' for this file
- *
- * Use this instead of setting the members directly. Doing so
- * avoids making mistakes like forgetting the mntget() or
- * forgetting to take a write on the mnt.
- *
- * Note: This is a crappy interface. It is here to make
- * merging with the existing users of get_empty_filp()
- * who have complex failure logic easier. All users
- * of this should be moving to alloc_file().
*/
-int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
- fmode_t mode, const struct file_operations *fop)
+static int init_file(struct file *file, struct vfsmount *mnt,
+ struct dentry *dentry, fmode_t mode,
+ const struct file_operations *fop)
{
int error = 0;
file->f_path.dentry = dentry;
@@ -218,7 +181,35 @@ int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
}
return error;
}
-EXPORT_SYMBOL(init_file);
+
+/**
+ * alloc_file - allocate and initialize a 'struct file'
+ * @mnt: the vfsmount on which the file will reside
+ * @dentry: the dentry representing the new file
+ * @mode: the mode with which the new file will be opened
+ * @fop: the 'struct file_operations' for the new file
+ *
+ * Use this instead of get_empty_filp() to get a new
+ * 'struct file'. Do so because of the same initialization
+ * pitfalls reasons listed for init_file(). This is a
+ * preferred interface to using init_file().
+ *
+ * If all the callers of init_file() are eliminated, its
+ * code should be moved into this function.
+ */
+struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
+ fmode_t mode, const struct file_operations *fop)
+{
+ struct file *file;
+
+ file = get_empty_filp();
+ if (!file)
+ return NULL;
+
+ init_file(file, mnt, dentry, mode, fop);
+ return file;
+}
+EXPORT_SYMBOL(alloc_file);

void fput(struct file *file)
{
diff --git a/include/linux/file.h b/include/linux/file.h
index 335a0a5..6a8d361 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -18,9 +18,6 @@ extern void drop_file_write_access(struct file *file);
struct file_operations;
struct vfsmount;
struct dentry;
-extern int init_file(struct file *, struct vfsmount *mnt,
- struct dentry *dentry, fmode_t mode,
- const struct file_operations *fop);
extern struct file *alloc_file(struct vfsmount *, struct dentry *dentry,
fmode_t mode, const struct file_operations *fop);

2009-12-04 20:48:36

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 07/15] fs: move get_empty_filp() deffinition to internal.h

All users outside of fs/ of get_empty_filp() have been removed. This patch
moves the definition from the include/ directory to internal.h so no new
users crop up and removes the EXPORT_SYMBOL. I'd love to see open intents
stop using it too, but that's a problem for another day and a smarter
developer!

Signed-off-by: Eric Paris <[email protected]>
Acked-by: Miklos Szeredi <[email protected]>
---

fs/file_table.c | 4 ++--
fs/internal.h | 1 +
fs/namei.c | 2 ++
fs/open.c | 2 ++
include/linux/fs.h | 1 -
5 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 0f9d2f2..629a167 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -24,6 +24,8 @@

#include <asm/atomic.h>

+#include "internal.h"
+
/* sysctl tunables... */
struct files_stat_struct files_stat = {
.max_files = NR_FILE
@@ -147,8 +149,6 @@ fail:
return NULL;
}

-EXPORT_SYMBOL(get_empty_filp);
-
/**
* init_file - initialize a 'struct file'
* @file: the already allocated 'struct file' to initialized
diff --git a/fs/internal.h b/fs/internal.h
index 515175b..f67cd14 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -79,6 +79,7 @@ extern void chroot_fs_refs(struct path *, struct path *);
* file_table.c
*/
extern void mark_files_ro(struct super_block *);
+extern struct file *get_empty_filp(void);

/*
* super.c
diff --git a/fs/namei.c b/fs/namei.c
index 87f97ba..d7ecd2f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -35,6 +35,8 @@
#include <linux/fs_struct.h>
#include <asm/uaccess.h>

+#include "internal.h"
+
#define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])

/* [Feb-1997 T. Schoebel-Theuer]
diff --git a/fs/open.c b/fs/open.c
index fa3bf4c..ebb74d4 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -31,6 +31,8 @@
#include <linux/falloc.h>
#include <linux/fs_struct.h>

+#include "internal.h"
+
int vfs_statfs(struct dentry *dentry, struct kstatfs *buf)
{
int retval = -ENODEV;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c0a3b6c..112d71a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2193,7 +2193,6 @@ static inline void insert_inode_hash(struct inode *inode) {
__insert_inode_hash(inode, inode->i_ino);
}

-extern struct file * get_empty_filp(void);
extern void file_move(struct file *f, struct list_head *list);
extern void file_kill(struct file *f);
#ifdef CONFIG_BLOCK

2009-12-04 20:49:00

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 08/15] ima: valid return code from ima_inode_alloc

ima_inode_alloc returns 0 and 1, but the LSM hooks expects an errno.

Signed-off-by: Eric Paris <[email protected]>
---

security/integrity/ima/ima_iint.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index a4e2b1d..4a53f39 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -87,8 +87,6 @@ out:
/**
* ima_inode_alloc - allocate an iint associated with an inode
* @inode: pointer to the inode
- *
- * Return 0 on success, 1 on failure.
*/
int ima_inode_alloc(struct inode *inode)
{
@@ -99,7 +97,7 @@ int ima_inode_alloc(struct inode *inode)

iint = ima_iint_insert(inode);
if (!iint)
- return 1;
+ return -ENOMEM;
return 0;
}

2009-12-04 20:49:06

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 09/15] ima: only insert at inode creation time

iints are supposed to be allocated when an inode is allocated (during
security_inode_alloc()) But we have code which will attempt to allocate
an iint during measurement calls. If we couldn't allocate the iint and we
cared, we should have died during security_inode_alloc(). Not make the
code more complex and less efficient.

Signed-off-by: Eric Paris <[email protected]>
---

security/integrity/ima/ima.h | 1 -
security/integrity/ima/ima_iint.c | 71 +++++--------------------------------
security/integrity/ima/ima_main.c | 8 ++--
3 files changed, 14 insertions(+), 66 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 165eb53..349aabc 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -128,7 +128,6 @@ void ima_template_show(struct seq_file *m, void *e,
*/
struct ima_iint_cache *ima_iint_insert(struct inode *inode);
struct ima_iint_cache *ima_iint_find_get(struct inode *inode);
-struct ima_iint_cache *ima_iint_find_insert_get(struct inode *inode);
void ima_iint_delete(struct inode *inode);
void iint_free(struct kref *kref);
void iint_rcu_free(struct rcu_head *rcu);
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 4a53f39..2f6ab52 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -45,22 +45,21 @@ out:
return iint;
}

-/* Allocate memory for the iint associated with the inode
- * from the iint_cache slab, initialize the iint, and
- * insert it into the radix tree.
- *
- * On success return a pointer to the iint; on failure return NULL.
+/**
+ * ima_inode_alloc - allocate an iint associated with an inode
+ * @inode: pointer to the inode
*/
-struct ima_iint_cache *ima_iint_insert(struct inode *inode)
+int ima_inode_alloc(struct inode *inode)
{
struct ima_iint_cache *iint = NULL;
int rc = 0;

if (!ima_initialized)
- return iint;
+ return 0;
+
iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
if (!iint)
- return iint;
+ return -ENOMEM;

rc = radix_tree_preload(GFP_NOFS);
if (rc < 0)
@@ -70,63 +69,13 @@ struct ima_iint_cache *ima_iint_insert(struct inode *inode)
rc = radix_tree_insert(&ima_iint_store, (unsigned long)inode, iint);
spin_unlock(&ima_iint_lock);
out:
- if (rc < 0) {
+ if (rc < 0)
kmem_cache_free(iint_cache, iint);
- if (rc == -EEXIST) {
- spin_lock(&ima_iint_lock);
- iint = radix_tree_lookup(&ima_iint_store,
- (unsigned long)inode);
- spin_unlock(&ima_iint_lock);
- } else
- iint = NULL;
- }
- radix_tree_preload_end();
- return iint;
-}
-
-/**
- * ima_inode_alloc - allocate an iint associated with an inode
- * @inode: pointer to the inode
- */
-int ima_inode_alloc(struct inode *inode)
-{
- struct ima_iint_cache *iint;
-
- if (!ima_initialized)
- return 0;
-
- iint = ima_iint_insert(inode);
- if (!iint)
- return -ENOMEM;
- return 0;
-}
-
-/* ima_iint_find_insert_get - get the iint associated with an inode
- *
- * Most insertions are done at inode_alloc, except those allocated
- * before late_initcall. When the iint does not exist, allocate it,
- * initialize and insert it, and increment the iint refcount.
- *
- * (Can't initialize at security_initcall before any inodes are
- * allocated, got to wait at least until proc_init.)
- *
- * Return the iint.
- */
-struct ima_iint_cache *ima_iint_find_insert_get(struct inode *inode)
-{
- struct ima_iint_cache *iint = NULL;

- iint = ima_iint_find_get(inode);
- if (iint)
- return iint;
-
- iint = ima_iint_insert(inode);
- if (iint)
- kref_get(&iint->refcount);
+ radix_tree_preload_end();

- return iint;
+ return rc;
}
-EXPORT_SYMBOL_GPL(ima_iint_find_insert_get);

/* iint_free - called when the iint refcount goes to zero */
void iint_free(struct kref *kref)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index b85e61b..96fafc0 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -161,7 +161,7 @@ int ima_path_check(struct path *path, int mask, int update_counts)

if (!ima_initialized || !S_ISREG(inode->i_mode))
return 0;
- iint = ima_iint_find_insert_get(inode);
+ iint = ima_iint_find_get(inode);
if (!iint)
return 0;

@@ -219,7 +219,7 @@ static int process_measurement(struct file *file, const unsigned char *filename,

if (!ima_initialized || !S_ISREG(inode->i_mode))
return 0;
- iint = ima_iint_find_insert_get(inode);
+ iint = ima_iint_find_get(inode);
if (!iint)
return -ENOMEM;

@@ -255,7 +255,7 @@ void ima_counts_put(struct path *path, int mask)
*/
if (!ima_initialized || !inode || !S_ISREG(inode->i_mode))
return;
- iint = ima_iint_find_insert_get(inode);
+ iint = ima_iint_find_get(inode);
if (!iint)
return;

@@ -286,7 +286,7 @@ void ima_counts_get(struct file *file)

if (!ima_initialized || !S_ISREG(inode->i_mode))
return;
- iint = ima_iint_find_insert_get(inode);
+ iint = ima_iint_find_get(inode);
if (!iint)
return;
mutex_lock(&iint->mutex);

2009-12-04 20:49:16

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 10/15] IMA: clean up the IMA counts updating code

We currently have a lot of duplicated code around ima file counts. Clean
that all up.

Signed-off-by: Eric Paris <[email protected]>
Acked-by: Serge Hallyn <[email protected]>
---

security/integrity/ima/ima.h | 1
security/integrity/ima/ima_main.c | 118 ++++++++++++++++++++++---------------
2 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 349aabc..268ef57 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -97,7 +97,6 @@ static inline unsigned long ima_hash_key(u8 *digest)

/* iint cache flags */
#define IMA_MEASURED 1
-#define IMA_IINT_DUMP_STACK 512

/* integrity data associated with an inode */
struct ima_iint_cache {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 96fafc0..e041233 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -13,8 +13,8 @@
* License.
*
* File: ima_main.c
- * implements the IMA hooks: ima_bprm_check, ima_file_mmap,
- * and ima_path_check.
+ * implements the IMA hooks: ima_bprm_check, ima_file_mmap,
+ * and ima_path_check.
*/
#include <linux/module.h>
#include <linux/file.h>
@@ -35,6 +35,69 @@ static int __init hash_setup(char *str)
}
__setup("ima_hash=", hash_setup);

+/*
+ * Update the counts given an fmode_t
+ */
+static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
+{
+ BUG_ON(!mutex_is_locked(&iint->mutex));
+
+ iint->opencount++;
+ if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+ iint->readcount++;
+ if (mode & FMODE_WRITE)
+ iint->writecount++;
+}
+
+/*
+ * Update the counts given open flags instead of fmode
+ */
+static void ima_inc_counts_flags(struct ima_iint_cache *iint, int flags)
+{
+ ima_inc_counts(iint, (__force fmode_t)((flags+1) & O_ACCMODE));
+}
+
+/*
+ * Decrement ima counts
+ */
+static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
+ fmode_t mode)
+{
+ BUG_ON(!mutex_is_locked(&iint->mutex));
+
+ iint->opencount--;
+ if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+ iint->readcount--;
+ if (mode & FMODE_WRITE) {
+ iint->writecount--;
+ if (iint->writecount == 0) {
+ if (iint->version != inode->i_version)
+ iint->flags &= ~IMA_MEASURED;
+ }
+ }
+
+ if ((iint->opencount < 0) ||
+ (iint->readcount < 0) ||
+ (iint->writecount < 0)) {
+ static int dumped;
+
+ if (dumped)
+ return;
+ dumped = 1;
+
+ printk(KERN_INFO "%s: open/free imbalance (r:%ld w:%ld o:%ld)\n",
+ __FUNCTION__, iint->readcount, iint->writecount,
+ iint->opencount);
+ dump_stack();
+ }
+}
+
+static void ima_dec_counts_flags(struct ima_iint_cache *iint,
+ struct inode *inode, int flags)
+{
+ ima_dec_counts(iint, inode, (__force fmode_t)((flags+1) & O_ACCMODE));
+}
+
/**
* ima_file_free - called on __fput()
* @file: pointer to file structure being freed
@@ -54,29 +117,7 @@ void ima_file_free(struct file *file)
return;

mutex_lock(&iint->mutex);
- if (iint->opencount <= 0) {
- printk(KERN_INFO
- "%s: %s open/free imbalance (r:%ld w:%ld o:%ld f:%ld)\n",
- __FUNCTION__, file->f_dentry->d_name.name,
- iint->readcount, iint->writecount,
- iint->opencount, atomic_long_read(&file->f_count));
- if (!(iint->flags & IMA_IINT_DUMP_STACK)) {
- dump_stack();
- iint->flags |= IMA_IINT_DUMP_STACK;
- }
- }
- iint->opencount--;
-
- if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
- iint->readcount--;
-
- if (file->f_mode & FMODE_WRITE) {
- iint->writecount--;
- if (iint->writecount == 0) {
- if (iint->version != inode->i_version)
- iint->flags &= ~IMA_MEASURED;
- }
- }
+ ima_dec_counts(iint, inode, file->f_mode);
mutex_unlock(&iint->mutex);
kref_put(&iint->refcount, iint_free);
}
@@ -116,8 +157,7 @@ static int get_path_measurement(struct ima_iint_cache *iint, struct file *file,
{
int rc = 0;

- iint->opencount++;
- iint->readcount++;
+ ima_inc_counts(iint, file->f_mode);

rc = ima_collect_measurement(iint, file);
if (!rc)
@@ -125,15 +165,6 @@ static int get_path_measurement(struct ima_iint_cache *iint, struct file *file,
return rc;
}

-static void ima_update_counts(struct ima_iint_cache *iint, int mask)
-{
- iint->opencount++;
- if ((mask & MAY_WRITE) || (mask == 0))
- iint->writecount++;
- else if (mask & (MAY_READ | MAY_EXEC))
- iint->readcount++;
-}
-
/**
* ima_path_check - based on policy, collect/store measurement.
* @path: contains a pointer to the path to be measured
@@ -167,7 +198,7 @@ int ima_path_check(struct path *path, int mask, int update_counts)

mutex_lock(&iint->mutex);
if (update_counts)
- ima_update_counts(iint, mask);
+ ima_inc_counts_flags(iint, mask);

rc = ima_must_measure(iint, inode, MAY_READ, PATH_CHECK);
if (rc < 0)
@@ -260,11 +291,7 @@ void ima_counts_put(struct path *path, int mask)
return;

mutex_lock(&iint->mutex);
- iint->opencount--;
- if ((mask & MAY_WRITE) || (mask == 0))
- iint->writecount--;
- else if (mask & (MAY_READ | MAY_EXEC))
- iint->readcount--;
+ ima_dec_counts_flags(iint, inode, mask);
mutex_unlock(&iint->mutex);

kref_put(&iint->refcount, iint_free);
@@ -290,12 +317,7 @@ void ima_counts_get(struct file *file)
if (!iint)
return;
mutex_lock(&iint->mutex);
- iint->opencount++;
- if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
- iint->readcount++;
-
- if (file->f_mode & FMODE_WRITE)
- iint->writecount++;
+ ima_inc_counts(iint, file->f_mode);
mutex_unlock(&iint->mutex);

kref_put(&iint->refcount, iint_free);

2009-12-04 20:48:48

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 11/15] ima: call ima_inode_free ima_inode_free

ima_inode_free() has some funky #define just to confuse the crap out of me.

#define ima_iint_free ima_inode_free

void ima_iint_delete(struct inode *inode)

and then things actually call ima_inode_free() and nothing calls
ima_iint_delete().

Signed-off-by: Eric Paris <[email protected]>
---

security/integrity/ima/ima.h | 1 -
security/integrity/ima/ima_iint.c | 6 ++----
2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 268ef57..c41afe6 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -127,7 +127,6 @@ void ima_template_show(struct seq_file *m, void *e,
*/
struct ima_iint_cache *ima_iint_insert(struct inode *inode);
struct ima_iint_cache *ima_iint_find_get(struct inode *inode);
-void ima_iint_delete(struct inode *inode);
void iint_free(struct kref *kref);
void iint_rcu_free(struct rcu_head *rcu);

diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 2f6ab52..fa592ff 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -19,8 +19,6 @@
#include <linux/radix-tree.h>
#include "ima.h"

-#define ima_iint_delete ima_inode_free
-
RADIX_TREE(ima_iint_store, GFP_ATOMIC);
DEFINE_SPINLOCK(ima_iint_lock);

@@ -111,12 +109,12 @@ void iint_rcu_free(struct rcu_head *rcu_head)
}

/**
- * ima_iint_delete - called on integrity_inode_free
+ * ima_inode_free - called on security_inode_free
* @inode: pointer to the inode
*
* Free the integrity information(iint) associated with an inode.
*/
-void ima_iint_delete(struct inode *inode)
+void ima_inode_free(struct inode *inode)
{
struct ima_iint_cache *iint;

2009-12-04 20:49:31

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 12/15] ima-path-check rework


---

fs/cachefiles/rdwr.c | 2
fs/ecryptfs/main.c | 3 -
fs/file_table.c | 4 +
fs/hugetlbfs/inode.c | 2
fs/namei.c | 33 ++------
fs/nfsd/vfs.c | 14 ---
fs/notify/fanotify/fanotify_user.c | 13 +--
fs/open.c | 12 ++-
include/asm-generic/fcntl.h | 5 +
include/linux/fs.h | 3 +
include/linux/ima.h | 16 ----
ipc/mqueue.c | 2
ipc/shm.c | 2
mm/shmem.c | 3 -
security/integrity/ima/ima_main.c | 141 ++++++++++-------------------------
security/integrity/ima/ima_policy.c | 10 +-
16 files changed, 79 insertions(+), 186 deletions(-)

diff --git a/fs/cachefiles/rdwr.c b/fs/cachefiles/rdwr.c
index a6c8c6f..1d83325 100644
--- a/fs/cachefiles/rdwr.c
+++ b/fs/cachefiles/rdwr.c
@@ -11,7 +11,6 @@

#include <linux/mount.h>
#include <linux/file.h>
-#include <linux/ima.h>
#include "internal.h"

/*
@@ -923,7 +922,6 @@ int cachefiles_write_page(struct fscache_storage *op, struct page *page)
if (IS_ERR(file)) {
ret = PTR_ERR(file);
} else {
- ima_counts_get(file);
ret = -EIO;
if (file->f_op->write) {
pos = (loff_t) page->index << PAGE_SHIFT;
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index c6ac85d..42b2961 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -35,7 +35,6 @@
#include <linux/key.h>
#include <linux/parser.h>
#include <linux/fs_stack.h>
-#include <linux/ima.h>
#include "ecryptfs_kernel.h"

/**
@@ -140,8 +139,6 @@ int ecryptfs_init_persistent_file(struct dentry *ecryptfs_dentry)
opened_lower_file = 1;
}
mutex_unlock(&inode_info->lower_file_mutex);
- if (opened_lower_file)
- ima_counts_get(inode_info->lower_file);
return rc;
}

diff --git a/fs/file_table.c b/fs/file_table.c
index 629a167..a4ef00e 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -10,6 +10,7 @@
#include <linux/file.h>
#include <linux/fdtable.h>
#include <linux/init.h>
+#include <linux/ima.h>
#include <linux/module.h>
#include <linux/fs.h>
#include <linux/security.h>
@@ -207,6 +208,9 @@ struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
return NULL;

init_file(file, mnt, dentry, mode, fop);
+
+ ima_path_check(file);
+
return file;
}
EXPORT_SYMBOL(alloc_file);
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index f1d80e5..4780d41 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -30,7 +30,6 @@
#include <linux/dnotify.h>
#include <linux/statfs.h>
#include <linux/security.h>
-#include <linux/ima.h>
#include <linux/magic.h>
#include <linux/xattr.h>

@@ -1021,7 +1020,6 @@ struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag,
&hugetlbfs_file_operations);
if (!file)
goto out_dentry; /* inode is already attached */
- ima_counts_get(file);

return file;

diff --git a/fs/namei.c b/fs/namei.c
index d7ecd2f..e9ea4f4 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -24,7 +24,6 @@
#include <linux/fsnotify.h>
#include <linux/personality.h>
#include <linux/security.h>
-#include <linux/ima.h>
#include <linux/syscalls.h>
#include <linux/mount.h>
#include <linux/audit.h>
@@ -1513,29 +1512,22 @@ int may_open(struct path *path, int acc_mode, int flag)
if (error)
return error;

- error = ima_path_check(path, acc_mode ?
- acc_mode & (MAY_READ | MAY_WRITE | MAY_EXEC) :
- ACC_MODE(flag) & (MAY_READ | MAY_WRITE),
- IMA_COUNT_UPDATE);
-
- if (error)
- return error;
/*
* An append-only file must be opened in append mode for writing.
*/
if (IS_APPEND(inode)) {
error = -EPERM;
if ((flag & FMODE_WRITE) && !(flag & O_APPEND))
- goto err_out;
+ return error;
if (flag & O_TRUNC)
- goto err_out;
+ return error;
}

/* O_NOATIME can only be set by the owner or superuser */
if (flag & O_NOATIME)
if (!is_owner_or_cap(inode)) {
error = -EPERM;
- goto err_out;
+ return error;
}

/*
@@ -1543,12 +1535,12 @@ int may_open(struct path *path, int acc_mode, int flag)
*/
error = break_lease(inode, flag);
if (error)
- goto err_out;
+ return error;

if (flag & O_TRUNC) {
error = get_write_access(inode);
if (error)
- goto err_out;
+ return error;

/*
* Refuse to truncate files with mandatory locks held on them.
@@ -1566,17 +1558,12 @@ int may_open(struct path *path, int acc_mode, int flag)
}
put_write_access(inode);
if (error)
- goto err_out;
+ return error;
} else
if (flag & FMODE_WRITE)
vfs_dq_init(inode);

return 0;
-err_out:
- ima_counts_put(path, acc_mode ?
- acc_mode & (MAY_READ | MAY_WRITE | MAY_EXEC) :
- ACC_MODE(flag) & (MAY_READ | MAY_WRITE));
- return error;
}

/*
@@ -1760,10 +1747,6 @@ do_last:
goto exit;
}
filp = nameidata_to_filp(&nd, open_flag);
- if (IS_ERR(filp))
- ima_counts_put(&nd.path,
- acc_mode & (MAY_READ | MAY_WRITE |
- MAY_EXEC));
mnt_drop_write(nd.path.mnt);
if (nd.root.mnt)
path_put(&nd.root);
@@ -1820,9 +1803,7 @@ ok:
goto exit;
}
filp = nameidata_to_filp(&nd, open_flag);
- if (IS_ERR(filp))
- ima_counts_put(&nd.path,
- acc_mode & (MAY_READ | MAY_WRITE | MAY_EXEC));
+
/*
* It is now safe to drop the mnt write
* because the filp has had a write taken
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index cf8ac12..54a80bb 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -55,7 +55,6 @@
#include <linux/security.h>
#endif /* CONFIG_NFSD_V4 */
#include <linux/jhash.h>
-#include <linux/ima.h>
#include "nfsd.h"
#include "vfs.h"

@@ -773,8 +772,6 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
flags, current_cred());
if (IS_ERR(*filp))
host_err = PTR_ERR(*filp);
- else
- ima_counts_get(*filp);
out_nfserr:
err = nfserrno(host_err);
out:
@@ -2072,7 +2069,6 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
struct dentry *dentry, int acc)
{
struct inode *inode = dentry->d_inode;
- struct path path;
int err;

if (acc == NFSD_MAY_NOP)
@@ -2145,17 +2141,7 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
if (err == -EACCES && S_ISREG(inode->i_mode) &&
acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE))
err = inode_permission(inode, MAY_EXEC);
- if (err)
- goto nfsd_out;

- /* Do integrity (permission) checking now, but defer incrementing
- * IMA counts to the actual file open.
- */
- path.mnt = exp->ex_path.mnt;
- path.dentry = dentry;
- err = ima_path_check(&path, acc & (MAY_READ | MAY_WRITE | MAY_EXEC),
- IMA_COUNT_LEAVE);
-nfsd_out:
return err? nfserrno(err) : 0;
}

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index c5f59d8..d5a73d8 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -3,7 +3,6 @@
#include <linux/fs.h>
#include <linux/anon_inodes.h>
#include <linux/fsnotify_backend.h>
-#include <linux/ima.h>
#include <linux/init.h>
#include <linux/mount.h>
#include <linux/namei.h>
@@ -48,7 +47,7 @@ static int create_and_fill_fd(struct fsnotify_group *group,
struct fanotify_event_metadata *metadata,
struct fsnotify_event *event)
{
- int client_fd, err;
+ int client_fd;
struct dentry *dentry;
struct vfsmount *mnt;
struct file *new_file;
@@ -75,13 +74,9 @@ static int create_and_fill_fd(struct fsnotify_group *group,
/* it's possible this event was an overflow event. in that case dentry and mnt
* are NULL; That's fine, just don't call dentry open */
if (dentry && mnt) {
- err = ima_path_check(&event->path, MAY_READ, IMA_COUNT_UPDATE);
- if (err)
- new_file = ERR_PTR(err);
- else
- new_file = dentry_open(dentry, mnt,
- O_RDONLY | O_LARGEFILE | FMODE_NONOTIFY,
- current_cred());
+ new_file = dentry_open(dentry, mnt,
+ O_RDONLY | O_LARGEFILE | FMODE_NONOTIFY,
+ current_cred());
} else
new_file = ERR_PTR(-EOVERFLOW);
if (IS_ERR(new_file)) {
diff --git a/fs/open.c b/fs/open.c
index ebb74d4..1ce3103 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -30,6 +30,7 @@
#include <linux/audit.h>
#include <linux/falloc.h>
#include <linux/fs_struct.h>
+#include <linux/ima.h>

#include "internal.h"

@@ -827,8 +828,9 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
struct inode *inode;
int error;

- f->f_flags = (flags & ~(FMODE_EXEC | FMODE_NONOTIFY));
- f->f_mode = (__force fmode_t)((flags+1) & O_ACCMODE) | (flags & FMODE_NONOTIFY) |
+ f->f_flags = (flags & ~(FMODE_EXEC | FMODE_NONOTIFY | FMODE_NOIMA));
+ f->f_mode = (__force fmode_t)((flags+1) & O_ACCMODE) |
+ (flags & (FMODE_NONOTIFY | FMODE_NOIMA)) |
FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;

inode = dentry->d_inode;
@@ -873,6 +875,12 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
}
}

+ error = ima_path_check(f);
+ if (error) {
+ fput(f);
+ f = ERR_PTR(error);
+ }
+
return f;

cleanup_all:
diff --git a/include/asm-generic/fcntl.h b/include/asm-generic/fcntl.h
index 7c0094f..67645ae 100644
--- a/include/asm-generic/fcntl.h
+++ b/include/asm-generic/fcntl.h
@@ -4,8 +4,9 @@
#include <linux/types.h>

/*
- * FMODE_EXEC is 0x20
- * FMODE_NONOTIFY is 0x800000
+ * FMODE_EXEC is 0x0000020
+ * FMODE_NONOTIFY is 0x0800000
+ * FMODE_NOIMA is 0x1000000
* These cannot be used by userspace O_* until internal and external open
* flags are split.
* -Eric Paris
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 112d71a..f435d5b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -91,6 +91,9 @@ struct inodes_stat_t {
/* File was opened by fanotify and shouldn't generate fanotify events */
#define FMODE_NONOTIFY ((__force fmode_t)8388608)

+/* File is being opened for IMA and should not do IMA measurements */
+#define FMODE_NOIMA ((__force fmode_t)16777216)
+
/*
* The below are the various read and write types that we support. Some of
* them include behavioral modifiers that send information down to the
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e3f2a4..4c68bf9 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,11 +20,9 @@ struct linux_binprm;
extern int ima_bprm_check(struct linux_binprm *bprm);
extern int ima_inode_alloc(struct inode *inode);
extern void ima_inode_free(struct inode *inode);
-extern int ima_path_check(struct path *path, int mask, int update_counts);
+extern int ima_path_check(struct file *file);
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);
-extern void ima_counts_get(struct file *file);
-extern void ima_counts_put(struct path *path, int mask);

#else
static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -42,7 +40,7 @@ static inline void ima_inode_free(struct inode *inode)
return;
}

-static inline int ima_path_check(struct path *path, int mask, int update_counts)
+static inline int ima_path_check(struct file *file)
{
return 0;
}
@@ -56,15 +54,5 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
{
return 0;
}
-
-static inline void ima_counts_get(struct file *file)
-{
- return;
-}
-
-static inline void ima_counts_put(struct path *path, int mask)
-{
- return;
-}
#endif /* CONFIG_IMA_H */
#endif /* _LINUX_IMA_H */
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index f5d4bd2..6c97934 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -32,7 +32,6 @@
#include <linux/nsproxy.h>
#include <linux/pid.h>
#include <linux/ipc_namespace.h>
-#include <linux/ima.h>

#include <net/sock.h>
#include "util.h"
@@ -734,7 +733,6 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
error = PTR_ERR(filp);
goto out_putfd;
}
- ima_counts_get(filp);

fd_install(fd, filp);
goto out_upsem;
diff --git a/ipc/shm.c b/ipc/shm.c
index 757f596..8bc7f0e 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -39,7 +39,6 @@
#include <linux/nsproxy.h>
#include <linux/mount.h>
#include <linux/ipc_namespace.h>
-#include <linux/ima.h>

#include <asm/uaccess.h>

@@ -891,7 +890,6 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr)
file = alloc_file(path.mnt, path.dentry, f_mode, &shm_file_operations);
if (!file)
goto out_free;
- ima_counts_get(file);

file->private_data = sfd;
file->f_mapping = shp->shm_file->f_mapping;
diff --git a/mm/shmem.c b/mm/shmem.c
index b212184..7bd8fd6 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -29,7 +29,6 @@
#include <linux/mm.h>
#include <linux/module.h>
#include <linux/swap.h>
-#include <linux/ima.h>

static struct vfsmount *shm_mnt;

@@ -2655,8 +2654,6 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
if (!file)
goto put_dentry;

- ima_counts_get(file);
-
#ifndef CONFIG_MMU
error = ramfs_nommu_expand_for_mapping(inode, size);
if (error) {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e041233..29d3723 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -36,10 +36,12 @@ static int __init hash_setup(char *str)
__setup("ima_hash=", hash_setup);

/*
- * Update the counts given an fmode_t
+ * Update the counts given a file
*/
-static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
+static void ima_inc_counts(struct ima_iint_cache *iint, struct file *file)
{
+ fmode_t mode = file->f_mode;
+
BUG_ON(!mutex_is_locked(&iint->mutex));

iint->opencount++;
@@ -50,19 +52,13 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
}

/*
- * Update the counts given open flags instead of fmode
- */
-static void ima_inc_counts_flags(struct ima_iint_cache *iint, int flags)
-{
- ima_inc_counts(iint, (__force fmode_t)((flags+1) & O_ACCMODE));
-}
-
-/*
* Decrement ima counts
*/
-static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
- fmode_t mode)
+static void ima_dec_counts(struct ima_iint_cache *iint, struct file *file)
{
+ struct inode *inode = file->f_path.dentry->d_inode;
+ fmode_t mode = file->f_mode;
+
BUG_ON(!mutex_is_locked(&iint->mutex));

iint->opencount--;
@@ -92,12 +88,6 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
}
}

-static void ima_dec_counts_flags(struct ima_iint_cache *iint,
- struct inode *inode, int flags)
-{
- ima_dec_counts(iint, inode, (__force fmode_t)((flags+1) & O_ACCMODE));
-}
-
/**
* ima_file_free - called on __fput()
* @file: pointer to file structure being freed
@@ -110,6 +100,8 @@ void ima_file_free(struct file *file)
struct inode *inode = file->f_dentry->d_inode;
struct ima_iint_cache *iint;

+ if (file->f_mode & FMODE_NOIMA)
+ return;
if (!ima_initialized || !S_ISREG(inode->i_mode))
return;
iint = ima_iint_find_get(inode);
@@ -117,7 +109,7 @@ void ima_file_free(struct file *file)
return;

mutex_lock(&iint->mutex);
- ima_dec_counts(iint, inode, file->f_mode);
+ ima_dec_counts(iint, file);
mutex_unlock(&iint->mutex);
kref_put(&iint->refcount, iint_free);
}
@@ -157,7 +149,7 @@ static int get_path_measurement(struct ima_iint_cache *iint, struct file *file,
{
int rc = 0;

- ima_inc_counts(iint, file->f_mode);
+ ima_inc_counts(iint, file);

rc = ima_collect_measurement(iint, file);
if (!rc)
@@ -166,7 +158,7 @@ static int get_path_measurement(struct ima_iint_cache *iint, struct file *file,
}

/**
- * ima_path_check - based on policy, collect/store measurement.
+ * ima_file_check - based on policy, collect/store measurement.
* @path: contains a pointer to the path to be measured
* @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE
*
@@ -183,13 +175,18 @@ static int get_path_measurement(struct ima_iint_cache *iint, struct file *file,
* Always return 0 and audit dentry_open failures.
* (Return code will be based upon measurement appraisal.)
*/
-int ima_path_check(struct path *path, int mask, int update_counts)
+int ima_path_check(struct file *file)
{
- struct inode *inode = path->dentry->d_inode;
+ struct dentry *dentry = file->f_path.dentry;
+ struct vfsmount *mnt = file->f_path.mnt;
+ struct inode *inode = dentry->d_inode;
+ fmode_t mode = file->f_mode;
struct ima_iint_cache *iint;
- struct file *file = NULL;
+ struct file *internal_file = NULL;
int rc;

+ if (mode & FMODE_NOIMA)
+ return 0;
if (!ima_initialized || !S_ISREG(inode->i_mode))
return 0;
iint = ima_iint_find_get(inode);
@@ -197,45 +194,45 @@ int ima_path_check(struct path *path, int mask, int update_counts)
return 0;

mutex_lock(&iint->mutex);
- if (update_counts)
- ima_inc_counts_flags(iint, mask);
+
+ ima_inc_counts(iint, file);

rc = ima_must_measure(iint, inode, MAY_READ, PATH_CHECK);
if (rc < 0)
goto out;

- if ((mask & MAY_WRITE) || (mask == 0))
+ /* if this file is writable, check if it is already open read */
+ if (mode & FMODE_WRITE) {
ima_read_write_check(TOMTOU, iint, inode,
- path->dentry->d_name.name);
-
- if ((mask & (MAY_WRITE | MAY_READ | MAY_EXEC)) != MAY_READ)
+ dentry->d_name.name);
goto out;
+ }

- ima_read_write_check(OPEN_WRITERS, iint, inode,
- path->dentry->d_name.name);
- if (!(iint->flags & IMA_MEASURED)) {
- struct dentry *dentry = dget(path->dentry);
- struct vfsmount *mnt = mntget(path->mnt);
+ /* if this is for read, does something else have this open for write? */
+ ima_read_write_check(OPEN_WRITERS, iint, inode, dentry->d_name.name);

- file = dentry_open(dentry, mnt, O_RDONLY | O_LARGEFILE,
- current_cred());
- if (IS_ERR(file)) {
- int audit_info = 0;
+ if (!(iint->flags & IMA_MEASURED)) {
+ dentry = dget(dentry);
+ mnt = mntget(mnt);

+ internal_file = dentry_open(dentry, mnt,
+ O_RDONLY | O_LARGEFILE | FMODE_NOIMA,
+ current_cred());
+ if (IS_ERR(internal_file)) {
integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
dentry->d_name.name,
"add_measurement",
"dentry_open failed",
- 1, audit_info);
- file = NULL;
+ 1, 0);
+ internal_file = NULL;
goto out;
}
rc = get_path_measurement(iint, file, dentry->d_name.name);
}
out:
mutex_unlock(&iint->mutex);
- if (file)
- fput(file);
+ if (internal_file)
+ fput(internal_file);
kref_put(&iint->refcount, iint_free);
return 0;
}
@@ -248,6 +245,8 @@ static int process_measurement(struct file *file, const unsigned char *filename,
struct ima_iint_cache *iint;
int rc;

+ if (file->f_mode & FMODE_NOIMA)
+ return 0;
if (!ima_initialized || !S_ISREG(inode->i_mode))
return 0;
iint = ima_iint_find_get(inode);
@@ -268,62 +267,6 @@ out:
return rc;
}

-/*
- * ima_counts_put - decrement file counts
- *
- * File counts are incremented in ima_path_check. On file open
- * error, such as ETXTBSY, decrement the counts to prevent
- * unnecessary imbalance messages.
- */
-void ima_counts_put(struct path *path, int mask)
-{
- struct inode *inode = path->dentry->d_inode;
- struct ima_iint_cache *iint;
-
- /* The inode may already have been freed, freeing the iint
- * with it. Verify the inode is not NULL before dereferencing
- * it.
- */
- if (!ima_initialized || !inode || !S_ISREG(inode->i_mode))
- return;
- iint = ima_iint_find_get(inode);
- if (!iint)
- return;
-
- mutex_lock(&iint->mutex);
- ima_dec_counts_flags(iint, inode, mask);
- mutex_unlock(&iint->mutex);
-
- kref_put(&iint->refcount, iint_free);
-}
-
-/*
- * ima_counts_get - increment file counts
- *
- * - for IPC shm and shmat file.
- * - for nfsd exported files.
- *
- * Increment the counts for these files to prevent unnecessary
- * imbalance messages.
- */
-void ima_counts_get(struct file *file)
-{
- struct inode *inode = file->f_dentry->d_inode;
- struct ima_iint_cache *iint;
-
- if (!ima_initialized || !S_ISREG(inode->i_mode))
- return;
- iint = ima_iint_find_get(inode);
- if (!iint)
- return;
- mutex_lock(&iint->mutex);
- ima_inc_counts(iint, file->f_mode);
- mutex_unlock(&iint->mutex);
-
- kref_put(&iint->refcount, iint_free);
-}
-EXPORT_SYMBOL_GPL(ima_counts_get);
-
/**
* ima_file_mmap - based on policy, collect/store measurement.
* @file: pointer to the file to be measured (May be NULL)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e127839..8c699b4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -205,7 +205,6 @@ void ima_update_policy(void)
const char *op = "policy_update";
const char *cause = "already exists";
int result = 1;
- int audit_info = 0;

if (ima_measure == &measure_default_rules) {
ima_measure = &measure_policy_rules;
@@ -213,7 +212,7 @@ void ima_update_policy(void)
result = 0;
}
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
- NULL, op, cause, result, audit_info);
+ NULL, op, cause, result, 0);
}

enum {
@@ -387,20 +386,19 @@ int ima_parse_add_rule(char *rule)
const char *op = "update_policy";
struct ima_measure_rule_entry *entry;
int result = 0;
- int audit_info = 0;

/* Prevent installed policy from changing */
if (ima_measure != &measure_default_rules) {
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
NULL, op, "already exists",
- -EACCES, audit_info);
+ -EACCES, 0);
return -EACCES;
}

entry = kzalloc(sizeof(*entry), GFP_KERNEL);
if (!entry) {
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
- NULL, op, "-ENOMEM", -ENOMEM, audit_info);
+ NULL, op, "-ENOMEM", -ENOMEM, 0);
return -ENOMEM;
}

@@ -415,7 +413,7 @@ int ima_parse_add_rule(char *rule)
kfree(entry);
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
NULL, op, "invalid policy", result,
- audit_info);
+ 0);
}
return result;
}

2009-12-04 20:50:16

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 13/15] ima: rename ima_path_check to ima_file_check

ima_path_check actually deals with files! call it ima_file_check instead.

Signed-off-by: Eric Paris <[email protected]>
---

fs/file_table.c | 2 +-
fs/open.c | 2 +-
include/linux/ima.h | 4 ++--
security/integrity/ima/ima_main.c | 6 +++---
4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index a4ef00e..51da7f9 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -209,7 +209,7 @@ struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,

init_file(file, mnt, dentry, mode, fop);

- ima_path_check(file);
+ ima_file_check(file);

return file;
}
diff --git a/fs/open.c b/fs/open.c
index 1ce3103..10bd04e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -875,7 +875,7 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
}
}

- error = ima_path_check(f);
+ error = ima_file_check(f);
if (error) {
fput(f);
f = ERR_PTR(error);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 4c68bf9..47ac315 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,7 @@ struct linux_binprm;
extern int ima_bprm_check(struct linux_binprm *bprm);
extern int ima_inode_alloc(struct inode *inode);
extern void ima_inode_free(struct inode *inode);
-extern int ima_path_check(struct file *file);
+extern int ima_file_check(struct file *file);
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);

@@ -40,7 +40,7 @@ static inline void ima_inode_free(struct inode *inode)
return;
}

-static inline int ima_path_check(struct file *file)
+static inline int ima_file_check(struct file *file)
{
return 0;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 29d3723..c721ddc 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -14,7 +14,7 @@
*
* File: ima_main.c
* implements the IMA hooks: ima_bprm_check, ima_file_mmap,
- * and ima_path_check.
+ * and ima_file_check.
*/
#include <linux/module.h>
#include <linux/file.h>
@@ -175,7 +175,7 @@ static int get_path_measurement(struct ima_iint_cache *iint, struct file *file,
* Always return 0 and audit dentry_open failures.
* (Return code will be based upon measurement appraisal.)
*/
-int ima_path_check(struct file *file)
+int ima_file_check(struct file *file)
{
struct dentry *dentry = file->f_path.dentry;
struct vfsmount *mnt = file->f_path.mnt;
@@ -236,7 +236,7 @@ out:
kref_put(&iint->refcount, iint_free);
return 0;
}
-EXPORT_SYMBOL_GPL(ima_path_check);
+EXPORT_SYMBOL_GPL(ima_file_check);

static int process_measurement(struct file *file, const unsigned char *filename,
int mask, int function)

2009-12-04 20:49:59

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 14/15] security: move ima_file_check() to lsm hook

From: Mimi Zohar <[email protected]>

Move the ima_file_check() hook from the vfs into the LSM hook.

Signed-off-by: Mimi Zohar <[email protected]>
Signed-off-by: Eric Paris <[email protected]>
---

fs/open.c | 7 -------
security/security.c | 8 +++++++-
2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 10bd04e..25c1436 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -30,7 +30,6 @@
#include <linux/audit.h>
#include <linux/falloc.h>
#include <linux/fs_struct.h>
-#include <linux/ima.h>

#include "internal.h"

@@ -875,12 +874,6 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
}
}

- error = ima_file_check(f);
- if (error) {
- fput(f);
- f = ERR_PTR(error);
- }
-
return f;

cleanup_all:
diff --git a/security/security.c b/security/security.c
index fd2d450..a42586b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -722,7 +722,13 @@ int security_file_receive(struct file *file)

int security_dentry_open(struct file *file, const struct cred *cred)
{
- return security_ops->dentry_open(file, cred);
+ int ret;
+
+ ret = security_ops->dentry_open(file, cred);
+ if (ret)
+ return ret;
+
+ return ima_file_check(file);
}

int security_task_create(unsigned long clone_flags)

2009-12-04 20:49:45

by Eric Paris

[permalink] [raw]
Subject: [RFC PATCH 15/15] ima: limit imbalance msg

From: Mimi Zohar <[email protected]>

Limit the number of imbalance messages to once per filesystem type instead of
once per system boot. (it's actually slightly racy and could give you a
couple per fs, but this isn't a real issue)

Signed-off-by: Mimi Zohar <[email protected]>
---

security/integrity/ima/ima_main.c | 62 ++++++++++++++++++++++++++++++++-----
1 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c721ddc..14d109b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -35,6 +35,55 @@ static int __init hash_setup(char *str)
}
__setup("ima_hash=", hash_setup);

+struct ima_imbalance {
+ struct hlist_node node;
+ unsigned long fsmagic;
+};
+
+/*
+ * ima_limit_imbalance - emit one imbalance message per filesystem type
+ *
+ * Maintain list of filesystem types that do not measure files properly.
+ * Return false if unknown, true if known.
+ */
+static bool ima_limit_imbalance(struct file *file)
+{
+ static DEFINE_SPINLOCK(ima_imbalance_lock);
+ static HLIST_HEAD(ima_imbalance_list);
+
+ struct super_block *sb = file->f_dentry->d_sb;
+ struct ima_imbalance *entry;
+ struct hlist_node *node;
+ bool found = false;
+
+ rcu_read_lock();
+ hlist_for_each_entry_rcu(entry, node, &ima_imbalance_list, node) {
+ if (entry->fsmagic == sb->s_magic) {
+ found = true;
+ break;
+ }
+ }
+ rcu_read_unlock();
+ if (found)
+ goto out;
+
+ entry = kmalloc(sizeof(*entry), GFP_NOFS);
+ if (!entry)
+ goto out;
+ entry->fsmagic = sb->s_magic;
+ spin_lock(&ima_imbalance_lock);
+ /*
+ * we could have raced and something else might have added this fs
+ * to the list, but we don't really care
+ */
+ hlist_add_head_rcu(&entry->node, &ima_imbalance_list);
+ spin_unlock(&ima_imbalance_lock);
+ printk(KERN_INFO "IMA: unmeasured files on fsmagic: %lX\n",
+ entry->fsmagic);
+out:
+ return found;
+}
+
/*
* Update the counts given a file
*/
@@ -72,15 +121,10 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct file *file)
}
}

- if ((iint->opencount < 0) ||
- (iint->readcount < 0) ||
- (iint->writecount < 0)) {
- static int dumped;
-
- if (dumped)
- return;
- dumped = 1;
-
+ if (((iint->opencount < 0) ||
+ (iint->readcount < 0) ||
+ (iint->writecount < 0)) &&
+ !ima_limit_imbalance(file)) {
printk(KERN_INFO "%s: open/free imbalance (r:%ld w:%ld o:%ld)\n",
__FUNCTION__, iint->readcount, iint->writecount,
iint->opencount);

2009-12-05 20:23:29

by Hugh Dickins

[permalink] [raw]
Subject: Re: [RFC PATCH 01/15] shmem: do not call fput_filp on an initialized filp

On Fri, 4 Dec 2009, Eric Paris wrote:

> fput_filp is supposed to be used when the filp was not used. But in the

put_filp

> ifndef CONFIG_MMU case shmem_setup_file could call this one an initialized

on

> filp. It should be using fput() instead. Since the fput() will dec the ima
> counts we also need to move the ima hook to make sure that is set up before
> the fput().
>
> Signed-off-by: Eric Paris <[email protected]>
> Acked-by: Miklos Szeredi <[email protected]>

Thanks,
Acked-by: Hugh Dickins <[email protected]>

> ---
>
> mm/shmem.c | 9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 356dd99..e7f8968 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2656,12 +2656,15 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
> init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> &shmem_file_operations);
>
> + ima_counts_get(file);
> +
> #ifndef CONFIG_MMU
> error = ramfs_nommu_expand_for_mapping(inode, size);
> - if (error)
> - goto close_file;
> + if (error) {
> + fput(file);
> + return error;
> + }
> #endif
> - ima_counts_get(file);
> return file;
>
> close_file:

2009-12-05 20:26:46

by Hugh Dickins

[permalink] [raw]
Subject: Re: [RFC PATCH 02/15] shmem: use alloc_file instead of init_file

On Fri, 4 Dec 2009, Eric Paris wrote:

> shmem uses get_empty_filp() and then init_file(). Their is no good reason

There

> not to just use alloc_file() like everything else.
>
> Acked-by: Miklos Szeredi <[email protected]>
> Signed-off-by: Eric Paris <[email protected]>

Right, what deterred me from using alloc_file() when it came in,
was that d_instantiate() done before the alloc_file(). But looking
through it now, I think it's okay, and I'm hoping you know it's okay.

Acked-by: Hugh Dickins <[email protected]>

> ---
>
> mm/shmem.c | 17 +++++++----------
> 1 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index e7f8968..b212184 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2640,21 +2640,20 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
> if (!dentry)
> goto put_memory;
>
> - error = -ENFILE;
> - file = get_empty_filp();
> - if (!file)
> - goto put_dentry;
> -
> error = -ENOSPC;
> inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
> if (!inode)
> - goto close_file;
> + goto put_dentry;
>
> d_instantiate(dentry, inode);
> inode->i_size = size;
> inode->i_nlink = 0; /* It is unlinked */
> - init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> - &shmem_file_operations);
> +
> + error = -ENFILE;
> + file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> + &shmem_file_operations);
> + if (!file)
> + goto put_dentry;
>
> ima_counts_get(file);
>
> @@ -2667,8 +2666,6 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
> #endif
> return file;
>
> -close_file:
> - put_filp(file);
> put_dentry:
> dput(dentry);
> put_memory:

2009-12-05 20:30:26

by Hugh Dickins

[permalink] [raw]
Subject: Re: [RFC PATCH 12/15] ima-path-check rework

On Fri, 4 Dec 2009, Eric Paris wrote:

I've not checked through this whole patch and your whole patchset,
but certainly when IMA came in, I felt (and said) that these calls
should be done at a lower level, so as not to affect each filesystem.
So I thoroughly approve of the direction of your patchset, even
though I cannot vouch for the details of it. Thank you, Eric.

Hugh

> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -29,7 +29,6 @@
> #include <linux/mm.h>
> #include <linux/module.h>
> #include <linux/swap.h>
> -#include <linux/ima.h>
>
> static struct vfsmount *shm_mnt;
>
> @@ -2655,8 +2654,6 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
> if (!file)
> goto put_dentry;
>
> - ima_counts_get(file);
> -
> #ifndef CONFIG_MMU
> error = ramfs_nommu_expand_for_mapping(inode, size);
> if (error) {