[PATCH 0/4] IMA: making i_readcount a first class inode citizen

[PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

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

[PATCH 1/4] IMA: define readcount functions

[Mimi Zohar]

Define iget/iput_readcount() functions to be called from the VFS layer.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
 include/linux/fs.h                     |   13 +++++++++++++
 security/integrity/ima/Makefile        |    2 +-
 security/integrity/ima/ima_readcount.c |   32 ++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 1 deletions(-)
 create mode 100644 security/integrity/ima/ima_readcount.c

diff --git a/include/linux/fs.h b/include/linux/fs.h
index bb20373..310d467 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2142,6 +2142,19 @@ static inline void allow_write_access(struct file *file)
 	if (file)
 		atomic_inc(&file->f_path.dentry->d_inode->i_writecount);
 }
+#ifdef CONFIG_IMA
+extern void iput_readcount(struct inode *inode);
+extern void iget_readcount(struct inode *inode);
+#else
+static inline void iput_readcount(struct inode *inode)
+{
+	return;
+}
+static inline void iget_readcount(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);
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 787c4cb..131eb1f 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -6,4 +6,4 @@
 obj-$(CONFIG_IMA) += ima.o
 
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
-	 ima_policy.o ima_iint.o ima_audit.o
+	 ima_policy.o ima_iint.o ima_audit.o ima_readcount.o
diff --git a/security/integrity/ima/ima_readcount.c b/security/integrity/ima/ima_readcount.c
new file mode 100644
index 0000000..6ee1163
--- /dev/null
+++ b/security/integrity/ima/ima_readcount.c
@@ -0,0 +1,32 @@
+/*
+ * Copyright (C) 2010 IBM Corporation
+ *
+ * Authors:
+ * Mimi Zohar <zohar@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ */
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/fs.h>
+
+void iput_readcount(struct inode *inode)
+{
+	spin_lock(&inode->i_lock);
+	if (unlikely(inode->i_readcount == 0))
+		printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
+		       inode->i_ino);
+	else
+		inode->i_readcount--;
+	spin_unlock(&inode->i_lock);
+}
+
+void iget_readcount(struct inode *inode)
+{
+	spin_lock(&inode->i_lock);
+	inode->i_readcount++;
+	spin_unlock(&inode->i_lock);
+}
-- 
1.7.2.2

[PATCH 2/4] IMA: maintain i_readcount in the VFS layer

[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>
---
 fs/file_table.c                   |    5 ++++-
 fs/inode.c                        |    3 +++
 fs/open.c                         |    3 ++-
 include/linux/ima.h               |    6 ------
 security/integrity/ima/ima_iint.c |    2 --
 security/integrity/ima/ima_main.c |   21 ++++++---------------
 6 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index a04bdd8..f7b7029 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -191,7 +191,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)
+		iget_readcount(path->dentry->d_inode);
 	return file;
 }
 EXPORT_SYMBOL(alloc_file);
@@ -252,6 +253,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)
+		iput_readcount(inode);
 	if (file->f_mode & FMODE_WRITE)
 		drop_file_write_access(file);
 	file->f_path.dentry = NULL;
diff --git a/fs/inode.c b/fs/inode.c
index 56d909d..1e806e0 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -139,6 +139,9 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	inode->i_nlink = 1;
 	inode->i_uid = 0;
 	inode->i_gid = 0;
+#ifdef CONFIG_IMA
+	inode->i_readcount = 0;
+#endif
 	atomic_set(&inode->i_writecount, 0);
 	inode->i_size = 0;
 	inode->i_blocks = 0;
diff --git a/fs/open.c b/fs/open.c
index d74e198..a6da4f7 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)
+		iget_readcount(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 c442e47..c4e381a 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -140,8 +140,6 @@ void ima_inode_free(struct inode *inode)
 	if (inode->i_readcount)
 		printk(KERN_INFO "%s: readcount: %u\n", __func__, inode->i_readcount);
 
-	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..ce2cd73 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,14 +103,11 @@ 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;
-
 	if (mode & FMODE_WRITE) {
 		if (inode->i_readcount && IS_IMA(inode))
 			send_tomtou = true;
@@ -125,10 +121,6 @@ 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)
-		inode->i_readcount++;
-
 	spin_unlock(&inode->i_lock);
 
 	if (send_tomtou)
@@ -157,7 +149,6 @@ static void ima_dec_counts(struct inode *inode, struct file *file)
 			}
 			return;
 		}
-		inode->i_readcount--;
 	}
 }
 
@@ -202,8 +193,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)
 {
@@ -317,6 +307,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.2.2

[PATCH 3/4] IMA: remove IMA imbalance checking

[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>
---
 security/integrity/ima/ima_iint.c |    3 -
 security/integrity/ima/ima_main.c |  104 +++----------------------------------
 2 files changed, 8 insertions(+), 99 deletions(-)

diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index c4e381a..4ae7304 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -137,9 +137,6 @@ 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 (!IS_IMA(inode))
 		return;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index ce2cd73..8af21e3 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,27 +82,6 @@ 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(inode->i_readcount == 0)) {
-			if (!ima_limit_imbalance(file)) {
-				printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-				       __func__, inode->i_readcount);
-				dump_stack();
-			}
-			return;
-		}
-	}
-}
-
 static void ima_check_last_writer(struct ima_iint_cache *iint,
 				  struct inode *inode,
 				  struct file *file)
@@ -167,28 +97,6 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
 		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
@@ -204,12 +112,16 @@ 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);
+	mutex_lock(&iint->mutex);
+	spin_lock(&inode->i_lock);
+
+	ima_check_last_writer(iint, inode, file);
 
+	spin_unlock(&inode->i_lock);
+	mutex_unlock(&iint->mutex);
 }
 
 static int process_measurement(struct file *file, const unsigned char *filename,
-- 
1.7.2.2

[PATCH 4/4] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

Finally, remove the ifdef's around i_readcount, making it a full
inode citizen so that other subsystems, such as leases, could use it.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
 fs/file_table.c                        |   18 ++++++++++++++++++
 include/linux/fs.h                     |   13 -------------
 security/integrity/ima/Makefile        |    2 +-
 security/integrity/ima/ima_readcount.c |   32 --------------------------------
 4 files changed, 19 insertions(+), 46 deletions(-)
 delete mode 100644 security/integrity/ima/ima_readcount.c

diff --git a/fs/file_table.c b/fs/file_table.c
index f7b7029..6821691 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -92,6 +92,24 @@ int proc_nr_files(ctl_table *table, int write,
 }
 #endif
 
+void iput_readcount(struct inode *inode)
+{
+	spin_lock(&inode->i_lock);
+	if (unlikely(inode->i_readcount == 0))
+		printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
+		       inode->i_ino);
+	else
+		inode->i_readcount--;
+	spin_unlock(&inode->i_lock);
+}
+
+void iget_readcount(struct inode *inode)
+{
+	spin_lock(&inode->i_lock);
+	inode->i_readcount++;
+	spin_unlock(&inode->i_lock);
+}
+
 /* Find an unused file structure and return a pointer to it.
  * Returns NULL, if there are no more free file structures or
  * we run out of memory.
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 310d467..b187c5c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -774,10 +774,8 @@ struct inode {
 
 	unsigned int		i_flags;
 
-#ifdef CONFIG_IMA
 	/* protected by i_lock */
 	unsigned int		i_readcount; /* struct files open RO */
-#endif
 	atomic_t		i_writecount;
 #ifdef CONFIG_SECURITY
 	void			*i_security;
@@ -2142,19 +2140,8 @@ static inline void allow_write_access(struct file *file)
 	if (file)
 		atomic_inc(&file->f_path.dentry->d_inode->i_writecount);
 }
-#ifdef CONFIG_IMA
 extern void iput_readcount(struct inode *inode);
 extern void iget_readcount(struct inode *inode);
-#else
-static inline void iput_readcount(struct inode *inode)
-{
-	return;
-}
-static inline void iget_readcount(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);
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 131eb1f..787c4cb 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -6,4 +6,4 @@
 obj-$(CONFIG_IMA) += ima.o
 
 ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
-	 ima_policy.o ima_iint.o ima_audit.o ima_readcount.o
+	 ima_policy.o ima_iint.o ima_audit.o
diff --git a/security/integrity/ima/ima_readcount.c b/security/integrity/ima/ima_readcount.c
deleted file mode 100644
index 6ee1163..0000000
--- a/security/integrity/ima/ima_readcount.c
+++ /dev/null
@@ -1,32 +0,0 @@
-/*
- * Copyright (C) 2010 IBM Corporation
- *
- * Authors:
- * Mimi Zohar <zohar@us.ibm.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation, version 2 of the
- * License.
- */
-#include <linux/module.h>
-#include <linux/spinlock.h>
-#include <linux/fs.h>
-
-void iput_readcount(struct inode *inode)
-{
-	spin_lock(&inode->i_lock);
-	if (unlikely(inode->i_readcount == 0))
-		printk(KERN_INFO "i_readcount: imbalance ino %ld\n",
-		       inode->i_ino);
-	else
-		inode->i_readcount--;
-	spin_unlock(&inode->i_lock);
-}
-
-void iget_readcount(struct inode *inode)
-{
-	spin_lock(&inode->i_lock);
-	inode->i_readcount++;
-	spin_unlock(&inode->i_lock);
-}
-- 
1.7.2.2

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Dave Chinner]

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

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Linus Torvalds]

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

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

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

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Eric Paris]

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

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Linus Torvalds]

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

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Al Viro]

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...

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

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

Re: [PATCH 2/4] IMA: maintain i_readcount in the VFS layer

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

On Thu, 28 Oct 2010 18:02:03 EDT, Mimi Zohar said:
> 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.

Would a symmetric i_writecount be useful for anything?  If it existed,
could any code be simplified elsewhere?


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/4] IMA: maintain i_readcount in the VFS layer

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 198 bytes --]

On Fri, 29 Oct 2010 10:13:38 EDT, Valdis.Kletnieks@vt.edu said:

> Would a symmetric i_writecount be useful for anything?  If it existed,
> could any code be simplified elsewhere?

-ENOCAFFIENE. :)

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[J. Bruce Fields]

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.

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

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

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[J. Bruce Fields]

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.

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

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

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[J. Bruce Fields]

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.

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[J. Bruce Fields]

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.

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Pavel Machek]

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

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

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