LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/4] IMA: making i_readcount a first class inode citizen
@ 2011-02-14 20:29 Mimi Zohar
  2011-02-14 20:29 ` [PATCH 1/4] IMA: convert i_readcount to atomic Mimi Zohar
                   ` (4 more replies)
  0 siblings, 5 replies; 23+ messages in thread
From: Mimi Zohar @ 2011-02-14 20:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mimi Zohar, linux-security-module, linux-fsdevel, James Morris,
	torvalds, eparis, viro, Dave Chinner, J. Bruce Fields,
	David Safford

This is a repost of patches 1 - 4 of the ima-i_readcount patch set rebased
against security-testing/#next, as requested a while ago.  The patches are
also available from:
git://git.kernel.org/pub/scm/linux/kernel/git/zohar/ima-2.6.git/#next

This patchset separates the incrementing/decrementing of the i_readcount,
in the VFS layer, from other IMA functionality, by replacing the current
ima_counts_get() call with i_readcount_inc(). Its unclear whether this
call to increment i_readcount should be made earlier, like i_writecount.
Currently the call is situated immediately after the switch from put_filp()
to fput() for cleanup.

Mimi

Mimi Zohar (4):
  IMA: convert i_readcount to atomic
  IMA: define readcount functions
  IMA: maintain i_readcount in the VFS layer
  IMA: remove IMA imbalance checking

 fs/file_table.c                   |    5 +-
 fs/open.c                         |    3 +-
 include/linux/fs.h                |   23 ++++++-
 include/linux/ima.h               |    6 --
 security/integrity/ima/ima_iint.c |    5 --
 security/integrity/ima/ima_main.c |  130 ++++---------------------------------
 6 files changed, 40 insertions(+), 132 deletions(-)

-- 
1.7.3.4


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/4] IMA: convert i_readcount to atomic
  2011-02-14 20:29 [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
@ 2011-02-14 20:29 ` Mimi Zohar
  2011-02-14 20:29 ` [PATCH 2/4] IMA: define readcount functions Mimi Zohar
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: Mimi Zohar @ 2011-02-14 20:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mimi Zohar, linux-security-module, linux-fsdevel, James Morris,
	torvalds, eparis, viro, Dave Chinner, J. Bruce Fields,
	David Safford, Mimi Zohar

Convert the inode's i_readcount from an unsigned int to atomic.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 include/linux/fs.h                |    3 +--
 security/integrity/ima/ima_iint.c |    7 ++++---
 security/integrity/ima/ima_main.c |   11 ++++++-----
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index baf3e55..ef85322 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -794,8 +794,7 @@ struct inode {
 #endif
 
 #ifdef CONFIG_IMA
-	/* protected by i_lock */
-	unsigned int		i_readcount; /* struct files open RO */
+	atomic_t		i_readcount; /* struct files open RO */
 #endif
 	atomic_t		i_writecount;
 #ifdef CONFIG_SECURITY
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index c442e47..f005355 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -137,10 +137,11 @@ void ima_inode_free(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
-	if (inode->i_readcount)
-		printk(KERN_INFO "%s: readcount: %u\n", __func__, inode->i_readcount);
+	if (atomic_read(&inode->i_readcount))
+		printk(KERN_INFO "%s: readcount: %u\n", __func__,
+		       atomic_read(&inode->i_readcount));
 
-	inode->i_readcount = 0;
+	atomic_set(&inode->i_readcount, 0);
 
 	if (!IS_IMA(inode))
 		return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 203de97..6e8cb93 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -113,7 +113,7 @@ void ima_counts_get(struct file *file)
 		goto out;
 
 	if (mode & FMODE_WRITE) {
-		if (inode->i_readcount && IS_IMA(inode))
+		if (atomic_read(&inode->i_readcount) && IS_IMA(inode))
 			send_tomtou = true;
 		goto out;
 	}
@@ -127,7 +127,7 @@ void ima_counts_get(struct file *file)
 out:
 	/* remember the vfs deals with i_writecount */
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
-		inode->i_readcount++;
+		atomic_inc(&inode->i_readcount);
 
 	spin_unlock(&inode->i_lock);
 
@@ -149,15 +149,16 @@ static void ima_dec_counts(struct inode *inode, struct file *file)
 	assert_spin_locked(&inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-		if (unlikely(inode->i_readcount == 0)) {
+		if (unlikely(atomic_read(&inode->i_readcount) == 0)) {
 			if (!ima_limit_imbalance(file)) {
 				printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-				       __func__, inode->i_readcount);
+				       __func__,
+				       atomic_read(&inode->i_readcount));
 				dump_stack();
 			}
 			return;
 		}
-		inode->i_readcount--;
+		atomic_dec(&inode->i_readcount);
 	}
 }
 
-- 
1.7.3.4


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 2/4] IMA: define readcount functions
  2011-02-14 20:29 [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
  2011-02-14 20:29 ` [PATCH 1/4] IMA: convert i_readcount to atomic Mimi Zohar
@ 2011-02-14 20:29 ` Mimi Zohar
  2011-02-14 20:29 ` [PATCH 3/4] IMA: maintain i_readcount in the VFS layer Mimi Zohar
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: Mimi Zohar @ 2011-02-14 20:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mimi Zohar, linux-security-module, linux-fsdevel, James Morris,
	torvalds, eparis, viro, Dave Chinner, J. Bruce Fields,
	David Safford, Mimi Zohar

Define i_readcount_inc/dec() functions to be called from the VFS layer.

Changelog:
- renamed iget/iput_readcount to i_readcount_inc/dec (Dave Chinner's suggestion)
- removed i_lock in iput_readcount() (based on comments:Dave Chinner,Eric Paris)

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 include/linux/fs.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index ef85322..a3e8f02 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2186,6 +2186,26 @@ static inline void allow_write_access(struct file *file)
 	if (file)
 		atomic_inc(&file->f_path.dentry->d_inode->i_writecount);
 }
+#ifdef CONFIG_IMA
+static inline void i_readcount_dec(struct inode *inode)
+{
+	BUG_ON(!atomic_read(&inode->i_readcount));
+	atomic_dec(&inode->i_readcount);
+}
+static inline void i_readcount_inc(struct inode *inode)
+{
+	atomic_inc(&inode->i_readcount);
+}
+#else
+static inline void i_readcount_dec(struct inode *inode)
+{
+	return;
+}
+static inline void i_readcount_inc(struct inode *inode)
+{
+	return;
+}
+#endif
 extern int do_pipe_flags(int *, int);
 extern struct file *create_read_pipe(struct file *f, int flags);
 extern struct file *create_write_pipe(int flags);
-- 
1.7.3.4


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 3/4] IMA: maintain i_readcount in the VFS layer
  2011-02-14 20:29 [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
  2011-02-14 20:29 ` [PATCH 1/4] IMA: convert i_readcount to atomic Mimi Zohar
  2011-02-14 20:29 ` [PATCH 2/4] IMA: define readcount functions Mimi Zohar
@ 2011-02-14 20:29 ` Mimi Zohar
  2011-02-14 20:29 ` [PATCH 4/4] IMA: remove IMA imbalance checking Mimi Zohar
  2011-02-16 23:08 ` [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
  4 siblings, 0 replies; 23+ messages in thread
From: Mimi Zohar @ 2011-02-14 20:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mimi Zohar, linux-security-module, linux-fsdevel, James Morris,
	torvalds, eparis, viro, Dave Chinner, J. Bruce Fields,
	David Safford, Mimi Zohar

ima_counts_get() updated the readcount and invalidated the PCR,
as necessary. Only update the i_readcount in the VFS layer.
Move the PCR invalidation checks to ima_file_check(), where it
belongs.

Maintaining the i_readcount in the VFS layer, will allow other
subsystems to use i_readcount.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 fs/file_table.c                   |    5 ++++-
 fs/open.c                         |    3 ++-
 include/linux/ima.h               |    6 ------
 security/integrity/ima/ima_iint.c |    2 --
 security/integrity/ima/ima_main.c |   25 ++++++++-----------------
 5 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index c3dee38..0c724de 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -190,7 +190,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
 		file_take_write(file);
 		WARN_ON(mnt_clone_write(path->mnt));
 	}
-	ima_counts_get(file);
+	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		i_readcount_inc(path->dentry->d_inode);
 	return file;
 }
 EXPORT_SYMBOL(alloc_file);
@@ -251,6 +252,8 @@ static void __fput(struct file *file)
 	fops_put(file->f_op);
 	put_pid(file->f_owner.pid);
 	file_sb_list_del(file);
+	if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		i_readcount_dec(inode);
 	if (file->f_mode & FMODE_WRITE)
 		drop_file_write_access(file);
 	file->f_path.dentry = NULL;
diff --git a/fs/open.c b/fs/open.c
index 4197b9e..0d485c5 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -688,7 +688,8 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 		if (error)
 			goto cleanup_all;
 	}
-	ima_counts_get(f);
+	if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		i_readcount_inc(inode);
 
 	f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 975837e..09e6e62 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,6 @@ extern void ima_inode_free(struct inode *inode);
 extern int ima_file_check(struct file *file, int mask);
 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);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -53,10 +52,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;
-}
-
 #endif /* CONFIG_IMA_H */
 #endif /* _LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index f005355..68efe3b 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -141,8 +141,6 @@ void ima_inode_free(struct inode *inode)
 		printk(KERN_INFO "%s: readcount: %u\n", __func__,
 		       atomic_read(&inode->i_readcount));
 
-	atomic_set(&inode->i_readcount, 0);
-
 	if (!IS_IMA(inode))
 		return;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6e8cb93..69b4856 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -86,17 +86,16 @@ out:
 }
 
 /*
- * ima_counts_get - increment file counts
+ * ima_rdwr_violation_check
  *
- * Maintain read/write counters for all files, but only
- * invalidate the PCR for measured files:
+ * Only invalidate the PCR for measured files:
  * 	- Opening a file for write when already open for read,
  *	  results in a time of measure, time of use (ToMToU) error.
  *	- Opening a file for read when already open for write,
  * 	  could result in a file measurement error.
  *
  */
-void ima_counts_get(struct file *file)
+static void ima_rdwr_violation_check(struct file *file)
 {
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
@@ -104,13 +103,10 @@ void ima_counts_get(struct file *file)
 	int rc;
 	bool send_tomtou = false, send_writers = false;
 
-	if (!S_ISREG(inode->i_mode))
+	if (!S_ISREG(inode->i_mode) || !ima_initialized)
 		return;
 
-	spin_lock(&inode->i_lock);
-
-	if (!ima_initialized)
-		goto out;
+	mutex_lock(&inode->i_mutex);	/* file metadata: permissions, xattr */
 
 	if (mode & FMODE_WRITE) {
 		if (atomic_read(&inode->i_readcount) && IS_IMA(inode))
@@ -125,11 +121,7 @@ void ima_counts_get(struct file *file)
 	if (atomic_read(&inode->i_writecount) > 0)
 		send_writers = true;
 out:
-	/* remember the vfs deals with i_writecount */
-	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
-		atomic_inc(&inode->i_readcount);
-
-	spin_unlock(&inode->i_lock);
+	mutex_unlock(&inode->i_mutex);
 
 	if (send_tomtou)
 		ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
@@ -158,7 +150,6 @@ static void ima_dec_counts(struct inode *inode, struct file *file)
 			}
 			return;
 		}
-		atomic_dec(&inode->i_readcount);
 	}
 }
 
@@ -203,8 +194,7 @@ static void ima_file_free_noiint(struct inode *inode, struct file *file)
  * ima_file_free - called on __fput()
  * @file: pointer to file structure being freed
  *
- * Flag files that changed, based on i_version;
- * and decrement the i_readcount.
+ * Flag files that changed, based on i_version
  */
 void ima_file_free(struct file *file)
 {
@@ -318,6 +308,7 @@ int ima_file_check(struct file *file, int mask)
 {
 	int rc;
 
+	ima_rdwr_violation_check(file);
 	rc = process_measurement(file, file->f_dentry->d_name.name,
 				 mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
 				 FILE_CHECK);
-- 
1.7.3.4


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 4/4] IMA: remove IMA imbalance checking
  2011-02-14 20:29 [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
                   ` (2 preceding siblings ...)
  2011-02-14 20:29 ` [PATCH 3/4] IMA: maintain i_readcount in the VFS layer Mimi Zohar
@ 2011-02-14 20:29 ` Mimi Zohar
  2011-02-16 23:08 ` [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
  4 siblings, 0 replies; 23+ messages in thread
From: Mimi Zohar @ 2011-02-14 20:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mimi Zohar, linux-security-module, linux-fsdevel, James Morris,
	torvalds, eparis, viro, Dave Chinner, J. Bruce Fields,
	David Safford, Mimi Zohar

Now that i_readcount is maintained by the VFS layer, remove the
imbalance checking in IMA. Cleans up the IMA code nicely.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 security/integrity/ima/ima_iint.c |    4 --
 security/integrity/ima/ima_main.c |  104 ++-----------------------------------
 2 files changed, 4 insertions(+), 104 deletions(-)

diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 68efe3b..4ae7304 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -137,10 +137,6 @@ void ima_inode_free(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
-	if (atomic_read(&inode->i_readcount))
-		printk(KERN_INFO "%s: readcount: %u\n", __func__,
-		       atomic_read(&inode->i_readcount));
-
 	if (!IS_IMA(inode))
 		return;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 69b4856..2df9021 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -36,55 +36,6 @@ 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;
-}
-
 /*
  * ima_rdwr_violation_check
  *
@@ -131,65 +82,20 @@ out:
 				  "open_writers");
 }
 
-/*
- * Decrement ima counts
- */
-static void ima_dec_counts(struct inode *inode, struct file *file)
-{
-	mode_t mode = file->f_mode;
-
-	assert_spin_locked(&inode->i_lock);
-
-	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-		if (unlikely(atomic_read(&inode->i_readcount) == 0)) {
-			if (!ima_limit_imbalance(file)) {
-				printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-				       __func__,
-				       atomic_read(&inode->i_readcount));
-				dump_stack();
-			}
-			return;
-		}
-	}
-}
-
 static void ima_check_last_writer(struct ima_iint_cache *iint,
 				  struct inode *inode,
 				  struct file *file)
 {
 	mode_t mode = file->f_mode;
 
-	BUG_ON(!mutex_is_locked(&iint->mutex));
-	assert_spin_locked(&inode->i_lock);
-
+	mutex_lock(&iint->mutex);
 	if (mode & FMODE_WRITE &&
 	    atomic_read(&inode->i_writecount) == 1 &&
 	    iint->version != inode->i_version)
 		iint->flags &= ~IMA_MEASURED;
-}
-
-static void ima_file_free_iint(struct ima_iint_cache *iint, struct inode *inode,
-			       struct file *file)
-{
-	mutex_lock(&iint->mutex);
-	spin_lock(&inode->i_lock);
-
-	ima_dec_counts(inode, file);
-	ima_check_last_writer(iint, inode, file);
-
-	spin_unlock(&inode->i_lock);
 	mutex_unlock(&iint->mutex);
 }
 
-static void ima_file_free_noiint(struct inode *inode, struct file *file)
-{
-	spin_lock(&inode->i_lock);
-
-	ima_dec_counts(inode, file);
-
-	spin_unlock(&inode->i_lock);
-}
-
 /**
  * ima_file_free - called on __fput()
  * @file: pointer to file structure being freed
@@ -205,12 +111,10 @@ void ima_file_free(struct file *file)
 		return;
 
 	iint = ima_iint_find(inode);
+	if (!iint)
+		return;
 
-	if (iint)
-		ima_file_free_iint(iint, inode, file);
-	else
-		ima_file_free_noiint(inode, file);
-
+	ima_check_last_writer(iint, inode, file);
 }
 
 static int process_measurement(struct file *file, const unsigned char *filename,
-- 
1.7.3.4


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2011-02-14 20:29 [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
                   ` (3 preceding siblings ...)
  2011-02-14 20:29 ` [PATCH 4/4] IMA: remove IMA imbalance checking Mimi Zohar
@ 2011-02-16 23:08 ` Mimi Zohar
  2011-02-17  1:46   ` James Morris
  4 siblings, 1 reply; 23+ messages in thread
From: Mimi Zohar @ 2011-02-16 23:08 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-security-module, linux-fsdevel, James Morris, torvalds,
	eparis, viro, Dave Chinner, J. Bruce Fields, David Safford

Hi, 

As Linus has already agreed that the i_readcount patches (1 - 4) can be
upstreamed (https://lkml.org/lkml/2010/11/18/670), do I need to do
anything else (eg. send James a request to pull)?

thanks,

Mimi

On Mon, 2011-02-14 at 15:29 -0500, Mimi Zohar wrote:
> This is a repost of patches 1 - 4 of the ima-i_readcount patch set rebased
> against security-testing/#next, as requested a while ago.  The patches are
> also available from:
> git://git.kernel.org/pub/scm/linux/kernel/git/zohar/ima-2.6.git/#next
> 
> This patchset separates the incrementing/decrementing of the i_readcount,
> in the VFS layer, from other IMA functionality, by replacing the current
> ima_counts_get() call with i_readcount_inc(). Its unclear whether this
> call to increment i_readcount should be made earlier, like i_writecount.
> Currently the call is situated immediately after the switch from put_filp()
> to fput() for cleanup.
> 
> Mimi
> 
> Mimi Zohar (4):
>   IMA: convert i_readcount to atomic
>   IMA: define readcount functions
>   IMA: maintain i_readcount in the VFS layer
>   IMA: remove IMA imbalance checking
> 
>  fs/file_table.c                   |    5 +-
>  fs/open.c                         |    3 +-
>  include/linux/fs.h                |   23 ++++++-
>  include/linux/ima.h               |    6 --
>  security/integrity/ima/ima_iint.c |    5 --
>  security/integrity/ima/ima_main.c |  130 ++++---------------------------------
>  6 files changed, 40 insertions(+), 132 deletions(-)
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2011-02-16 23:08 ` [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
@ 2011-02-17  1:46   ` James Morris
  0 siblings, 0 replies; 23+ messages in thread
From: James Morris @ 2011-02-17  1:46 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-kernel, linux-security-module, linux-fsdevel, torvalds,
	eparis, viro, Dave Chinner, J. Bruce Fields, David Safford

On Wed, 16 Feb 2011, Mimi Zohar wrote:

> Hi, 
> 
> As Linus has already agreed that the i_readcount patches (1 - 4) can be
> upstreamed (https://lkml.org/lkml/2010/11/18/670), do I need to do
> anything else (eg. send James a request to pull)?


I've pulled these changes into
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

-- 
James Morris
<jmorris@namei.org>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-11-05 19:08         ` J. Bruce Fields
  2010-11-05 20:58           ` J. Bruce Fields
@ 2010-11-07  0:03           ` Mimi Zohar
  1 sibling, 0 replies; 23+ messages in thread
From: Mimi Zohar @ 2010-11-07  0:03 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: linux-kernel, linux-security-module, linux-fsdevel, hch,
	warthog9, david, jmorris, kyle, hpa, akpm, torvalds, mingo,
	eparis, viro, Matthew Wilcox

On Fri, 2010-11-05 at 15:08 -0400, J. Bruce Fields wrote:
> On Fri, Nov 05, 2010 at 01:38:05PM -0400, Mimi Zohar wrote:
> > On Fri, 2010-11-05 at 12:28 -0400, J. Bruce Fields wrote: 
> > > On Fri, Nov 05, 2010 at 07:08:06AM -0400, Mimi Zohar wrote:
> > 
> > > > Right, like the ima_file_check(), which is after the __dentry_open().
> > > > Al, is it possible to move the break_lease() in may_open() to later?
> > > 
> > > That would still leave a race like:
> > > 
> > > 			check count
> > > 	bump count
> > > 	break lease
> > > 			set lease
> > > 
> > > But we could extend the i_lock to prevent the lease being bumped between
> > > the two steps on the right-hand side.
> > 
> > The latest i_readcount patchset, i_readcount is atomic and doesn't
> > require i_lock, at least for IMA.  Have to think about this more ....
> > 
> > > At that point I think we'd be done?  We're assured the count is still
> > > zero while the lease is added to the inode, so anyone in the process of
> > > doing an open has yet to reach the break_lease, which will see the newly
> > > added lease.
> > > 
> > > That leaves the problem that leases really should be broken on anything
> > > that changes the attributes or the dentries pointing to the inode:
> > > setattr, link, unlink, rename, at least.
> > 
> > For this reason, IMA is now taking i_mutex, preventing file metadata
> > from changing.
> 
> Lease code could do that as well.  (Probably just with a trylock,
> failing the setlease if we can't get the lock.)
> 
> That misses rename, though, which doesn't take the i_mutex on the
> renamed file.  Which makes sense.

fs/namei.c: vfs_rename_other() seems to be taking the i_mutex.  Am I
looking in the wrong place?

> But a lease is used to give file server clients the right to do an open
> locally, and we want them to be able to guarantee to applications that
> the path (well, the last component, at least) still refers to the same
> file at open time.

Mimi


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-10-28 22:45     ` Eric Paris
  2010-10-29  0:30       ` Mimi Zohar
@ 2010-11-06 10:44       ` Pavel Machek
  1 sibling, 0 replies; 23+ messages in thread
From: Pavel Machek @ 2010-11-06 10:44 UTC (permalink / raw)
  To: Eric Paris
  Cc: Linus Torvalds, Dave Chinner, Mimi Zohar, linux-kernel,
	linux-security-module, linux-fsdevel, hch, warthog9, jmorris,
	kyle, hpa, akpm, mingo, viro, Matthew Wilcox

On Thu 2010-10-28 18:45:07, Eric Paris wrote:
> On Thu, 2010-10-28 at 15:29 -0700, Linus Torvalds wrote:
> > On Thu, Oct 28, 2010 at 3:24 PM, Dave Chinner <david@fromorbit.com> wrote:
> > >
> > > Why the wrapper functions and locking? Why not an atomic variable like
> > > i_writecount?
> > 
> > Indeed. With moving this more into the VFS, let's just make sure it
> > looks like i_writecount as much as possible.
> 
> My thought was that the IMA read/write checks should happen AFTER the
> i_writecount and i_readcount counters were updated.  Thus even if we
> raced with another task we can rest assured that the other task would
> catch the situation we missed....

Is not that too late? The other process may have already acted on that data...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-11-05 19:08         ` J. Bruce Fields
@ 2010-11-05 20:58           ` J. Bruce Fields
  2010-11-07  0:03           ` Mimi Zohar
  1 sibling, 0 replies; 23+ messages in thread
From: J. Bruce Fields @ 2010-11-05 20:58 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-kernel, linux-security-module, linux-fsdevel, hch,
	warthog9, david, jmorris, kyle, hpa, akpm, torvalds, mingo,
	eparis, viro, Matthew Wilcox

On Fri, Nov 05, 2010 at 03:08:27PM -0400, J. Bruce Fields wrote:
> On Fri, Nov 05, 2010 at 01:38:05PM -0400, Mimi Zohar wrote:
> > On Fri, 2010-11-05 at 12:28 -0400, J. Bruce Fields wrote: 
> > > On Fri, Nov 05, 2010 at 07:08:06AM -0400, Mimi Zohar wrote:
> > 
> > > > Right, like the ima_file_check(), which is after the __dentry_open().
> > > > Al, is it possible to move the break_lease() in may_open() to later?
> > > 
> > > That would still leave a race like:
> > > 
> > > 			check count
> > > 	bump count
> > > 	break lease
> > > 			set lease
> > > 
> > > But we could extend the i_lock to prevent the lease being bumped between
> > > the two steps on the right-hand side.
> > 
> > The latest i_readcount patchset, i_readcount is atomic and doesn't
> > require i_lock, at least for IMA.  Have to think about this more ....
> > 
> > > At that point I think we'd be done?  We're assured the count is still
> > > zero while the lease is added to the inode, so anyone in the process of
> > > doing an open has yet to reach the break_lease, which will see the newly
> > > added lease.
> > > 
> > > That leaves the problem that leases really should be broken on anything
> > > that changes the attributes or the dentries pointing to the inode:
> > > setattr, link, unlink, rename, at least.
> > 
> > For this reason, IMA is now taking i_mutex, preventing file metadata
> > from changing.

Apologies, by the way, if I'm hijacking the thread, but:

> 
> Lease code could do that as well.  (Probably just with a trylock,
> failing the setlease if we can't get the lock.)
> 
> That misses rename, though, which doesn't take the i_mutex on the
> renamed file.  Which makes sense.
> 
> But a lease is used to give file server clients the right to do an open
> locally, and we want them to be able to guarantee to applications that
> the path (well, the last component, at least) still refers to the same
> file at open time.

We could *also* do trylock on the parent directory that the open used
(yipes), but even that's not quite enough; rfc 5661:

	"If the object being renamed has file delegations held by
	clients other than the one doing the RENAME, the delegations
	MUST be recalled, and the operation cannot proceed until each
	such delegation is returned or revoked.  Note that in the case
	of multiply linked files, the delegation recall requirement
	applies even if the delegation was obtained through a different
	name than the one being renamed."

That means if a client gets a delegation on file "foo", then discovers
that "bar" is the same file, it can assume it is safe to do a local open
as "bar"; so we're stuck breaking leases on a rename of "bar" to
"baz".

For correct leases from NFS's point of view--Samba's requirements are
probably similar--

	- a lease has an owner, and no operations by the owner conflict
	  with the lease.  For operations by non-owners:

	- read leases must conflict with write opens, and with anything
	  that would modify data, metadata, or the set of hard links
	  naming the file (so setattr, link, unlink, rename).

	- write leases must conflict with everything read leases do,
	  plus *reads* of data or metadata.  (If a "make" stats a source
	  file with a write lease, it has to wait for the lease to
	  broken, or else it may miss the fact that a client has updated
	  the file in its local cache).

Non-requirements:

	- Leases are an optimization, and it's OK to turn them down even
	  when we don't strictly have to.  (Though if we fail to grant
	  them in common cases then it's a performance problem.)

	- Leases are only useful in situations where conflicts are rare,
	  and they can be held for a long time.  It's OK if lease
	  acquisition is somewhat expensive.

I'd be happy just to have correct read leases....

--b.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-11-05 17:38       ` Mimi Zohar
@ 2010-11-05 19:08         ` J. Bruce Fields
  2010-11-05 20:58           ` J. Bruce Fields
  2010-11-07  0:03           ` Mimi Zohar
  0 siblings, 2 replies; 23+ messages in thread
From: J. Bruce Fields @ 2010-11-05 19:08 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-kernel, linux-security-module, linux-fsdevel, hch,
	warthog9, david, jmorris, kyle, hpa, akpm, torvalds, mingo,
	eparis, viro, Matthew Wilcox

On Fri, Nov 05, 2010 at 01:38:05PM -0400, Mimi Zohar wrote:
> On Fri, 2010-11-05 at 12:28 -0400, J. Bruce Fields wrote: 
> > On Fri, Nov 05, 2010 at 07:08:06AM -0400, Mimi Zohar wrote:
> 
> > > Right, like the ima_file_check(), which is after the __dentry_open().
> > > Al, is it possible to move the break_lease() in may_open() to later?
> > 
> > That would still leave a race like:
> > 
> > 			check count
> > 	bump count
> > 	break lease
> > 			set lease
> > 
> > But we could extend the i_lock to prevent the lease being bumped between
> > the two steps on the right-hand side.
> 
> The latest i_readcount patchset, i_readcount is atomic and doesn't
> require i_lock, at least for IMA.  Have to think about this more ....
> 
> > At that point I think we'd be done?  We're assured the count is still
> > zero while the lease is added to the inode, so anyone in the process of
> > doing an open has yet to reach the break_lease, which will see the newly
> > added lease.
> > 
> > That leaves the problem that leases really should be broken on anything
> > that changes the attributes or the dentries pointing to the inode:
> > setattr, link, unlink, rename, at least.
> 
> For this reason, IMA is now taking i_mutex, preventing file metadata
> from changing.

Lease code could do that as well.  (Probably just with a trylock,
failing the setlease if we can't get the lock.)

That misses rename, though, which doesn't take the i_mutex on the
renamed file.  Which makes sense.

But a lease is used to give file server clients the right to do an open
locally, and we want them to be able to guarantee to applications that
the path (well, the last component, at least) still refers to the same
file at open time.

> > One approach: add another counter to the inode named disable_leases, and
> > have any of those operations do something like:
> > 
> > 	disable_lease++
> > 	break_lease
> > 	... do operation ...
> > 	disable_lease--
> > 
> > ?
> > 
> 
> lol, getting i_readcount was was hard enough. 

Argh.  It really is a serious bug, for NFS at least.  And I'm a little
out of non-inode-expanding ideas.

--b.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-11-05 16:28     ` J. Bruce Fields
@ 2010-11-05 17:38       ` Mimi Zohar
  2010-11-05 19:08         ` J. Bruce Fields
  0 siblings, 1 reply; 23+ messages in thread
From: Mimi Zohar @ 2010-11-05 17:38 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: linux-kernel, linux-security-module, linux-fsdevel, hch,
	warthog9, david, jmorris, kyle, hpa, akpm, torvalds, mingo,
	eparis, viro, Matthew Wilcox

On Fri, 2010-11-05 at 12:28 -0400, J. Bruce Fields wrote: 
> On Fri, Nov 05, 2010 at 07:08:06AM -0400, Mimi Zohar wrote:

> > Right, like the ima_file_check(), which is after the __dentry_open().
> > Al, is it possible to move the break_lease() in may_open() to later?
> 
> That would still leave a race like:
> 
> 			check count
> 	bump count
> 	break lease
> 			set lease
> 
> But we could extend the i_lock to prevent the lease being bumped between
> the two steps on the right-hand side.

The latest i_readcount patchset, i_readcount is atomic and doesn't
require i_lock, at least for IMA.  Have to think about this more ....

> At that point I think we'd be done?  We're assured the count is still
> zero while the lease is added to the inode, so anyone in the process of
> doing an open has yet to reach the break_lease, which will see the newly
> added lease.
> 
> That leaves the problem that leases really should be broken on anything
> that changes the attributes or the dentries pointing to the inode:
> setattr, link, unlink, rename, at least.

For this reason, IMA is now taking i_mutex, preventing file metadata
from changing.

> One approach: add another counter to the inode named disable_leases, and
> have any of those operations do something like:
> 
> 	disable_lease++
> 	break_lease
> 	... do operation ...
> 	disable_lease--
> 
> ?
> 
> --b.

lol, getting i_readcount was was hard enough. 

Mimi


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-11-05 11:08   ` Mimi Zohar
@ 2010-11-05 16:28     ` J. Bruce Fields
  2010-11-05 17:38       ` Mimi Zohar
  0 siblings, 1 reply; 23+ messages in thread
From: J. Bruce Fields @ 2010-11-05 16:28 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-kernel, linux-security-module, linux-fsdevel, hch,
	warthog9, david, jmorris, kyle, hpa, akpm, torvalds, mingo,
	eparis, viro, Matthew Wilcox

On Fri, Nov 05, 2010 at 07:08:06AM -0400, Mimi Zohar wrote:
> On Thu, 2010-11-04 at 21:12 -0400, J. Bruce Fields wrote:
> > On Thu, Oct 28, 2010 at 06:02:01PM -0400, Mimi Zohar wrote:
> > > On Mon, 2010-10-25 at 14:41 -0400, Eric Paris wrote:
> > > 
> > > <snip>
> > > 
> > > > I believe that IBM is going to look into making i_readcount a first
> > > > class citizen which can be used by both IMA and generic_setlease().
> > > > Then people could say IMA had 0 per inode overhead   :)
> > > 
> > > This patchset separates the incrementing/decrementing of the i_readcount,
> > > in the VFS layer, from other IMA functionality, by replacing the current
> > > ima_counts_get() call with iget_readcount(). Its unclear whether this
> > > call to increment i_readcount should be made earlier. 
> > > 
> > > The patch ordering is a bit redundant in order to leave removing the ifdef
> > > around i_readcount until the last patch. The first three patches: defines 
> > > iget/iput_readcount(), moves the IMA functionality in ima_counts_get() to
> > > ima_file_check(), and removes the IMA imbalance code, simplifying IMA. The
> > > last patch moves iget/iput_readcount() to the fs directory and removes the
> > > ifdef around i_readcount, making i_readcount into a "first class inode citizen".
> > > 
> > > The generic_setlease code could then take advantage of i_readcount, assuming
> > > it can take the spin_lock, by doing something like:
> > > 
> > > -		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
> > > +
> > > +		spin_lock(&inode->i_lock);
> > > +		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0)){
> > > +			spin_unlock(&inode->i_lock);
> > >  			goto out;
> > > -		if ((arg == F_WRLCK)
> > > -		    && ((atomic_read(&dentry->d_count) > 1)
> > > -			|| (atomic_read(&inode->i_count) > 1)))
> > > +		}
> > > +		if ((arg == F_WRLCK) && (inode->i_readcount > 1)) {
> > > +			spin_unlock(&inode->i_lock);
> > >  			goto out;
> > > +		}
> > > +		spin_unlock(&inode->i_lock);
> > >  	}
> > 
> > Seems like an improvement.
> > 
> > It still leaves the race:
> > 
> > 	may_open calls lease_break, finds no lease
> > 
> > 			setlease checks read/writecount, finds 0,
> > 			creates lease
> > 
> > 	__dentry_open bumps read/writecount
> > 
> > (Is there any reason we couldn't move the break_lease to after bumping
> > read or write count?)
> > 
> > --b.
> 
> Right, like the ima_file_check(), which is after the __dentry_open().
> Al, is it possible to move the break_lease() in may_open() to later?

That would still leave a race like:

			check count
	bump count
	break lease
			set lease

But we could extend the i_lock to prevent the lease being bumped between
the two steps on the right-hand side.

At that point I think we'd be done?  We're assured the count is still
zero while the lease is added to the inode, so anyone in the process of
doing an open has yet to reach the break_lease, which will see the newly
added lease.

That leaves the problem that leases really should be broken on anything
that changes the attributes or the dentries pointing to the inode:
setattr, link, unlink, rename, at least.

One approach: add another counter to the inode named disable_leases, and
have any of those operations do something like:

	disable_lease++
	break_lease
	... do operation ...
	disable_lease--

?

--b.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-11-05  1:12 ` J. Bruce Fields
@ 2010-11-05 11:08   ` Mimi Zohar
  2010-11-05 16:28     ` J. Bruce Fields
  0 siblings, 1 reply; 23+ messages in thread
From: Mimi Zohar @ 2010-11-05 11:08 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: linux-kernel, linux-security-module, linux-fsdevel, hch,
	warthog9, david, jmorris, kyle, hpa, akpm, torvalds, mingo,
	eparis, viro, Matthew Wilcox

On Thu, 2010-11-04 at 21:12 -0400, J. Bruce Fields wrote:
> On Thu, Oct 28, 2010 at 06:02:01PM -0400, Mimi Zohar wrote:
> > On Mon, 2010-10-25 at 14:41 -0400, Eric Paris wrote:
> > 
> > <snip>
> > 
> > > I believe that IBM is going to look into making i_readcount a first
> > > class citizen which can be used by both IMA and generic_setlease().
> > > Then people could say IMA had 0 per inode overhead   :)
> > 
> > This patchset separates the incrementing/decrementing of the i_readcount,
> > in the VFS layer, from other IMA functionality, by replacing the current
> > ima_counts_get() call with iget_readcount(). Its unclear whether this
> > call to increment i_readcount should be made earlier. 
> > 
> > The patch ordering is a bit redundant in order to leave removing the ifdef
> > around i_readcount until the last patch. The first three patches: defines 
> > iget/iput_readcount(), moves the IMA functionality in ima_counts_get() to
> > ima_file_check(), and removes the IMA imbalance code, simplifying IMA. The
> > last patch moves iget/iput_readcount() to the fs directory and removes the
> > ifdef around i_readcount, making i_readcount into a "first class inode citizen".
> > 
> > The generic_setlease code could then take advantage of i_readcount, assuming
> > it can take the spin_lock, by doing something like:
> > 
> > -		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
> > +
> > +		spin_lock(&inode->i_lock);
> > +		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0)){
> > +			spin_unlock(&inode->i_lock);
> >  			goto out;
> > -		if ((arg == F_WRLCK)
> > -		    && ((atomic_read(&dentry->d_count) > 1)
> > -			|| (atomic_read(&inode->i_count) > 1)))
> > +		}
> > +		if ((arg == F_WRLCK) && (inode->i_readcount > 1)) {
> > +			spin_unlock(&inode->i_lock);
> >  			goto out;
> > +		}
> > +		spin_unlock(&inode->i_lock);
> >  	}
> 
> Seems like an improvement.
> 
> It still leaves the race:
> 
> 	may_open calls lease_break, finds no lease
> 
> 			setlease checks read/writecount, finds 0,
> 			creates lease
> 
> 	__dentry_open bumps read/writecount
> 
> (Is there any reason we couldn't move the break_lease to after bumping
> read or write count?)
> 
> --b.

Right, like the ima_file_check(), which is after the __dentry_open().
Al, is it possible to move the break_lease() in may_open() to later?

thanks,

Mimi


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-10-28 22:02 Mimi Zohar
  2010-10-28 22:24 ` Dave Chinner
@ 2010-11-05  1:12 ` J. Bruce Fields
  2010-11-05 11:08   ` Mimi Zohar
  1 sibling, 1 reply; 23+ messages in thread
From: J. Bruce Fields @ 2010-11-05  1:12 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-kernel, linux-security-module, linux-fsdevel, hch,
	warthog9, david, jmorris, kyle, hpa, akpm, torvalds, mingo,
	eparis, viro, Matthew Wilcox

On Thu, Oct 28, 2010 at 06:02:01PM -0400, Mimi Zohar wrote:
> On Mon, 2010-10-25 at 14:41 -0400, Eric Paris wrote:
> 
> <snip>
> 
> > I believe that IBM is going to look into making i_readcount a first
> > class citizen which can be used by both IMA and generic_setlease().
> > Then people could say IMA had 0 per inode overhead   :)
> 
> This patchset separates the incrementing/decrementing of the i_readcount,
> in the VFS layer, from other IMA functionality, by replacing the current
> ima_counts_get() call with iget_readcount(). Its unclear whether this
> call to increment i_readcount should be made earlier. 
> 
> The patch ordering is a bit redundant in order to leave removing the ifdef
> around i_readcount until the last patch. The first three patches: defines 
> iget/iput_readcount(), moves the IMA functionality in ima_counts_get() to
> ima_file_check(), and removes the IMA imbalance code, simplifying IMA. The
> last patch moves iget/iput_readcount() to the fs directory and removes the
> ifdef around i_readcount, making i_readcount into a "first class inode citizen".
> 
> The generic_setlease code could then take advantage of i_readcount, assuming
> it can take the spin_lock, by doing something like:
> 
> -		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
> +
> +		spin_lock(&inode->i_lock);
> +		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0)){
> +			spin_unlock(&inode->i_lock);
>  			goto out;
> -		if ((arg == F_WRLCK)
> -		    && ((atomic_read(&dentry->d_count) > 1)
> -			|| (atomic_read(&inode->i_count) > 1)))
> +		}
> +		if ((arg == F_WRLCK) && (inode->i_readcount > 1)) {
> +			spin_unlock(&inode->i_lock);
>  			goto out;
> +		}
> +		spin_unlock(&inode->i_lock);
>  	}

Seems like an improvement.

It still leaves the race:

	may_open calls lease_break, finds no lease

			setlease checks read/writecount, finds 0,
			creates lease

	__dentry_open bumps read/writecount

(Is there any reason we couldn't move the break_lease to after bumping
read or write count?)

--b.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-10-28 22:45     ` Eric Paris
@ 2010-10-29  0:30       ` Mimi Zohar
  2010-11-06 10:44       ` Pavel Machek
  1 sibling, 0 replies; 23+ messages in thread
From: Mimi Zohar @ 2010-10-29  0:30 UTC (permalink / raw)
  To: Eric Paris
  Cc: Linus Torvalds, Dave Chinner, linux-kernel,
	linux-security-module, linux-fsdevel, hch, warthog9, jmorris,
	kyle, hpa, akpm, mingo, viro, Matthew Wilcox

On Thu, 2010-10-28 at 18:45 -0400, Eric Paris wrote:
> On Thu, 2010-10-28 at 15:29 -0700, Linus Torvalds wrote:
> > On Thu, Oct 28, 2010 at 3:24 PM, Dave Chinner <david@fromorbit.com> wrote:
> > >
> > > Why the wrapper functions and locking? Why not an atomic variable like
> > > i_writecount?
> > 
> > Indeed. With moving this more into the VFS, let's just make sure it
> > looks like i_writecount as much as possible.
> 
> My thought was that the IMA read/write checks should happen AFTER the
> i_writecount and i_readcount counters were updated.  Thus even if we
> raced with another task we can rest assured that the other task would
> catch the situation we missed....
> 
> -Eric

Thanks Eric. My misunderstanding. Will update the patches, making
i_readcount atomic.

Mimi


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-10-28 22:46       ` Linus Torvalds
@ 2010-10-28 23:25         ` Al Viro
  0 siblings, 0 replies; 23+ messages in thread
From: Al Viro @ 2010-10-28 23:25 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Mimi Zohar, Dave Chinner, linux-kernel, linux-security-module,
	linux-fsdevel, hch, warthog9, jmorris, kyle, hpa, akpm, mingo,
	eparis, Matthew Wilcox

On Thu, Oct 28, 2010 at 03:46:04PM -0700, Linus Torvalds wrote:
> On Thu, Oct 28, 2010 at 3:38 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >
> > Would making i_readcount atomic be enough in ima_rdwr_violation_check(),
> > or would it still need to take the spin_lock? IMA needs guarantees
> > that the i_readcount/i_writecount won't be updated in between.
> 
> If i_writecount is always updated under the i_lock, then the fix is
> probably to make that one non-atomic instead. There's no point in
> having an atomic that is always updated under a spinlock, that just
> makes everything slower.
> 
> Regardless, I don't think i_readcount should be different from i_writecount.
> 
> Al? Comments?

Well... the rules for i_writecount had been "protect zero->non-zero
transitions with spinlock".  Back then we had a single spinlock for that,
IIRC, and making it atomic had been an obvious solution.  Note that we
have a bunch of places in VM where we need to adjust it (VMA merges and
splitting) and I really wanted to avoid contention on that lock.

BTW, I suspect that the right first step would be to kill open-coded
manipulations of i_writecount in mmap.c and fork.c; there are inlined
helpers for that.

I *really* don't like what add_dquot_ref() is doing; it checks i_writecount
under inode_lock and skips the inodes for which it's zero.  It might be
OK if one of the quota locks held by callers will serialize it wrt the
relevant part of the open() path, but that needs a comment along those
lines, at the very least.

We probably can go for non-atomic; the lock is per-inode these days, so
it's less of a PITA.  I wonder if IMA holding it over its policy checks
would be a good thing, though - it calls LSM hooks and hell knows what
shit gets done from those...

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-10-28 22:38     ` Mimi Zohar
@ 2010-10-28 22:46       ` Linus Torvalds
  2010-10-28 23:25         ` Al Viro
  0 siblings, 1 reply; 23+ messages in thread
From: Linus Torvalds @ 2010-10-28 22:46 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Dave Chinner, linux-kernel, linux-security-module, linux-fsdevel,
	hch, warthog9, jmorris, kyle, hpa, akpm, mingo, eparis, viro,
	Matthew Wilcox

On Thu, Oct 28, 2010 at 3:38 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
> Would making i_readcount atomic be enough in ima_rdwr_violation_check(),
> or would it still need to take the spin_lock? IMA needs guarantees
> that the i_readcount/i_writecount won't be updated in between.

If i_writecount is always updated under the i_lock, then the fix is
probably to make that one non-atomic instead. There's no point in
having an atomic that is always updated under a spinlock, that just
makes everything slower.

Regardless, I don't think i_readcount should be different from i_writecount.

Al? Comments?

                  Linus

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-10-28 22:29   ` Linus Torvalds
  2010-10-28 22:38     ` Mimi Zohar
@ 2010-10-28 22:45     ` Eric Paris
  2010-10-29  0:30       ` Mimi Zohar
  2010-11-06 10:44       ` Pavel Machek
  1 sibling, 2 replies; 23+ messages in thread
From: Eric Paris @ 2010-10-28 22:45 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Dave Chinner, Mimi Zohar, linux-kernel, linux-security-module,
	linux-fsdevel, hch, warthog9, jmorris, kyle, hpa, akpm, mingo,
	viro, Matthew Wilcox

On Thu, 2010-10-28 at 15:29 -0700, Linus Torvalds wrote:
> On Thu, Oct 28, 2010 at 3:24 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > Why the wrapper functions and locking? Why not an atomic variable like
> > i_writecount?
> 
> Indeed. With moving this more into the VFS, let's just make sure it
> looks like i_writecount as much as possible.

My thought was that the IMA read/write checks should happen AFTER the
i_writecount and i_readcount counters were updated.  Thus even if we
raced with another task we can rest assured that the other task would
catch the situation we missed....

-Eric


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-10-28 22:29   ` Linus Torvalds
@ 2010-10-28 22:38     ` Mimi Zohar
  2010-10-28 22:46       ` Linus Torvalds
  2010-10-28 22:45     ` Eric Paris
  1 sibling, 1 reply; 23+ messages in thread
From: Mimi Zohar @ 2010-10-28 22:38 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Dave Chinner, linux-kernel, linux-security-module, linux-fsdevel,
	hch, warthog9, jmorris, kyle, hpa, akpm, mingo, eparis, viro,
	Matthew Wilcox

On Thu, 2010-10-28 at 15:29 -0700, Linus Torvalds wrote:
> On Thu, Oct 28, 2010 at 3:24 PM, Dave Chinner <david@fromorbit.com> wrote:
> >
> > Why the wrapper functions and locking? Why not an atomic variable like
> > i_writecount?
> 
> Indeed. With moving this more into the VFS, let's just make sure it
> looks like i_writecount as much as possible.
> 
>                       Linus

Would making i_readcount atomic be enough in ima_rdwr_violation_check(),
or would it still need to take the spin_lock? IMA needs guarantees
that the i_readcount/i_writecount won't be updated in between.

        spin_lock(&inode->i_lock);

        if (mode & FMODE_WRITE) {
                if (inode->i_readcount && IS_IMA(inode))
                        send_tomtou = true;
                goto out;
        }

        rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
        if (rc < 0)
                goto out;

        if (atomic_read(&inode->i_writecount) > 0)
                send_writers = true;
out:
        spin_unlock(&inode->i_lock);

Wouldn't the same be true in fs/locks:get_setleases()?

Mimi


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-10-28 22:24 ` Dave Chinner
@ 2010-10-28 22:29   ` Linus Torvalds
  2010-10-28 22:38     ` Mimi Zohar
  2010-10-28 22:45     ` Eric Paris
  0 siblings, 2 replies; 23+ messages in thread
From: Linus Torvalds @ 2010-10-28 22:29 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Mimi Zohar, linux-kernel, linux-security-module, linux-fsdevel,
	hch, warthog9, jmorris, kyle, hpa, akpm, mingo, eparis, viro,
	Matthew Wilcox

On Thu, Oct 28, 2010 at 3:24 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> Why the wrapper functions and locking? Why not an atomic variable like
> i_writecount?

Indeed. With moving this more into the VFS, let's just make sure it
looks like i_writecount as much as possible.

                      Linus

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen
  2010-10-28 22:02 Mimi Zohar
@ 2010-10-28 22:24 ` Dave Chinner
  2010-10-28 22:29   ` Linus Torvalds
  2010-11-05  1:12 ` J. Bruce Fields
  1 sibling, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2010-10-28 22:24 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-kernel, linux-security-module, linux-fsdevel, hch,
	warthog9, jmorris, kyle, hpa, akpm, torvalds, mingo, eparis,
	viro, Matthew Wilcox

On Thu, Oct 28, 2010 at 06:02:01PM -0400, Mimi Zohar wrote:
> On Mon, 2010-10-25 at 14:41 -0400, Eric Paris wrote:
> 
> <snip>
> 
> > I believe that IBM is going to look into making i_readcount a first
> > class citizen which can be used by both IMA and generic_setlease().
> > Then people could say IMA had 0 per inode overhead   :)
> 
> This patchset separates the incrementing/decrementing of the i_readcount,
> in the VFS layer, from other IMA functionality, by replacing the current
> ima_counts_get() call with iget_readcount(). Its unclear whether this
> call to increment i_readcount should be made earlier. 

Why the wrapper functions and locking? Why not an atomic variable like
i_writecount?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 0/4] IMA: making i_readcount a first class inode citizen 
@ 2010-10-28 22:02 Mimi Zohar
  2010-10-28 22:24 ` Dave Chinner
  2010-11-05  1:12 ` J. Bruce Fields
  0 siblings, 2 replies; 23+ messages in thread
From: Mimi Zohar @ 2010-10-28 22:02 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mimi Zohar, linux-security-module, linux-fsdevel, hch, warthog9,
	david, jmorris, kyle, hpa, akpm, torvalds, mingo, eparis, viro,
	Matthew Wilcox

On Mon, 2010-10-25 at 14:41 -0400, Eric Paris wrote:

<snip>

> I believe that IBM is going to look into making i_readcount a first
> class citizen which can be used by both IMA and generic_setlease().
> Then people could say IMA had 0 per inode overhead   :)

This patchset separates the incrementing/decrementing of the i_readcount,
in the VFS layer, from other IMA functionality, by replacing the current
ima_counts_get() call with iget_readcount(). Its unclear whether this
call to increment i_readcount should be made earlier. 

The patch ordering is a bit redundant in order to leave removing the ifdef
around i_readcount until the last patch. The first three patches: defines 
iget/iput_readcount(), moves the IMA functionality in ima_counts_get() to
ima_file_check(), and removes the IMA imbalance code, simplifying IMA. The
last patch moves iget/iput_readcount() to the fs directory and removes the
ifdef around i_readcount, making i_readcount into a "first class inode citizen".

The generic_setlease code could then take advantage of i_readcount, assuming
it can take the spin_lock, by doing something like:

-		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
+
+		spin_lock(&inode->i_lock);
+		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0)){
+			spin_unlock(&inode->i_lock);
 			goto out;
-		if ((arg == F_WRLCK)
-		    && ((atomic_read(&dentry->d_count) > 1)
-			|| (atomic_read(&inode->i_count) > 1)))
+		}
+		if ((arg == F_WRLCK) && (inode->i_readcount > 1)) {
+			spin_unlock(&inode->i_lock);
 			goto out;
+		}
+		spin_unlock(&inode->i_lock);
 	}
 
Mimi

Mimi Zohar (4):
  IMA: define readcount functions
  IMA: maintain i_readcount in the VFS layer
  IMA: remove IMA imbalance checking
  IMA: making i_readcount a first class inode citizen

 fs/file_table.c                   |   23 +++++++-
 fs/inode.c                        |    3 +
 fs/open.c                         |    3 +-
 include/linux/fs.h                |    4 +-
 include/linux/ima.h               |    6 --
 security/integrity/ima/ima_iint.c |    5 --
 security/integrity/ima/ima_main.c |  125 ++++--------------------------------
 7 files changed, 43 insertions(+), 126 deletions(-)

-- 
1.7.2.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2011-02-17  1:47 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-14 20:29 [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
2011-02-14 20:29 ` [PATCH 1/4] IMA: convert i_readcount to atomic Mimi Zohar
2011-02-14 20:29 ` [PATCH 2/4] IMA: define readcount functions Mimi Zohar
2011-02-14 20:29 ` [PATCH 3/4] IMA: maintain i_readcount in the VFS layer Mimi Zohar
2011-02-14 20:29 ` [PATCH 4/4] IMA: remove IMA imbalance checking Mimi Zohar
2011-02-16 23:08 ` [PATCH 0/4] IMA: making i_readcount a first class inode citizen Mimi Zohar
2011-02-17  1:46   ` James Morris
  -- strict thread matches above, loose matches on Subject: below --
2010-10-28 22:02 Mimi Zohar
2010-10-28 22:24 ` Dave Chinner
2010-10-28 22:29   ` Linus Torvalds
2010-10-28 22:38     ` Mimi Zohar
2010-10-28 22:46       ` Linus Torvalds
2010-10-28 23:25         ` Al Viro
2010-10-28 22:45     ` Eric Paris
2010-10-29  0:30       ` Mimi Zohar
2010-11-06 10:44       ` Pavel Machek
2010-11-05  1:12 ` J. Bruce Fields
2010-11-05 11:08   ` Mimi Zohar
2010-11-05 16:28     ` J. Bruce Fields
2010-11-05 17:38       ` Mimi Zohar
2010-11-05 19:08         ` J. Bruce Fields
2010-11-05 20:58           ` J. Bruce Fields
2010-11-07  0:03           ` Mimi Zohar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).