[RFC PATCH 0/2] Fix untangling ima mess, part 2 with counters

[RFC PATCH 0/2] Fix untangling ima mess, part 2 with counters

[Mimi Zohar]

The "Untangling ima mess, part 2 with counters" patch not
only messed up the counters, but also doesn't measure files
which should be measured. The "Untangling ima mess ..."
patchset, applied some of Eric's patches, but not all, leaving
inodes allocated before late_initcall() not allocated/measured.
(8262bb85da ima: initialize ima before inodes can be allocated)

Up to now, measuring files and updating the IMA open/read/write
counters associated with the file were done at the same time
in ima_path_check().  An imbalanced counter was an indication
that the file hadn't been measured. Each case needed to be
inspected, resulting in adding either a new ima_counts_get()
or ima_path_check() call (e.g. nfsd, ecryptfs, openAFS).

This patchset separates incrementing the counters from measuring
the file.  However, the underlying assumption is that all regular
files are opened via do_filp_open(). Is this assumption correct or,
by incrementing the file counters separately, have we inadvertently
hidden the fact that a file wasn't measured?

Mimi

[RFC PATCH 1/2] Fix 1 untangling ima mess, part 2 with counters

[Mimi Zohar]

The "Untangling ima mess, part 2 with counters" patch messed
up the counters.  Based on conversations with Al Viro, this patch
streamlines ima_path_check() by removing the counter maintaince.
The counters are now updated independently, from measuring the file,
in __dentry_open() and alloc_file() by calling ima_counts_get().
ima_path_check() is called from nfsd and do_filp_open().

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
 fs/namei.c                        |    4 +-
 include/linux/ima.h               |    4 +-
 security/integrity/ima/ima_main.c |  234 ++++++++++++++-----------------------
 3 files changed, 94 insertions(+), 148 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index fabd939..a89ebc3 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1730,7 +1730,7 @@ do_last:
 		if (nd.root.mnt)
 			path_put(&nd.root);
 		if (!IS_ERR(filp)) {
-			error = ima_path_check(&filp->f_path, filp->f_mode &
+			error = ima_path_check(filp, filp->f_mode &
 				       (MAY_READ | MAY_WRITE | MAY_EXEC));
 			if (error) {
 				fput(filp);
@@ -1791,7 +1791,7 @@ ok:
 	}
 	filp = nameidata_to_filp(&nd, open_flag);
 	if (!IS_ERR(filp)) {
-		error = ima_path_check(&filp->f_path, filp->f_mode &
+		error = ima_path_check(filp, filp->f_mode &
 			       (MAY_READ | MAY_WRITE | MAY_EXEC));
 		if (error) {
 			fput(filp);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 99dc6d5..aa55a8f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -17,7 +17,7 @@ struct linux_binprm;
 extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_inode_alloc(struct inode *inode);
 extern void ima_inode_free(struct inode *inode);
-extern int ima_path_check(struct path *path, int mask);
+extern int ima_path_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);
@@ -38,7 +38,7 @@ static inline void ima_inode_free(struct inode *inode)
 	return;
 }
 
-static inline int ima_path_check(struct path *path, int mask)
+static inline int ima_path_check(struct file *file, int mask)
 {
 	return 0;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a89f44d..1ffa54e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -84,6 +84,36 @@ out:
 	return found;
 }
 
+/* ima_read_write_check - reflect possible reading/writing errors in the PCR.
+ *
+ * When opening a file for read, if the file is already open for write,
+ * the file could change, resulting in a file measurement error.
+ *
+ * Opening a file for write, if the file is already open for read, results
+ * in a time of measure, time of use (ToMToU) error.
+ *
+ * In either case invalidate the PCR.
+ */
+enum iint_pcr_error { TOMTOU, OPEN_WRITERS };
+static void ima_read_write_check(enum iint_pcr_error error,
+				 struct ima_iint_cache *iint,
+				 struct inode *inode,
+				 const unsigned char *filename)
+{
+	switch (error) {
+	case TOMTOU:
+		if (iint->readcount > 0)
+			ima_add_violation(inode, filename, "invalid_pcr",
+					  "ToMToU");
+		break;
+	case OPEN_WRITERS:
+		if (iint->writecount > 0)
+			ima_add_violation(inode, filename, "invalid_pcr",
+					  "open_writers");
+		break;
+	}
+}
+
 /*
  * Update the counts given an fmode_t
  */
@@ -99,6 +129,47 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
 }
 
 /*
+ * ima_counts_get - increment file counts
+ *
+ * Maintain read/write counters for all files, but 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)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+	fmode_t mode = file->f_mode;
+	struct ima_iint_cache *iint;
+	int rc;
+
+	if (!ima_initialized || !S_ISREG(inode->i_mode))
+		return;
+	iint = ima_iint_find_get(inode);
+	if (!iint)
+		return;
+	mutex_lock(&iint->mutex);
+	rc = ima_must_measure(iint, inode, MAY_READ, PATH_CHECK);
+	if (rc < 0)
+		goto out;
+
+	if (mode & FMODE_WRITE) {
+		ima_read_write_check(TOMTOU, iint, inode, dentry->d_name.name);
+		goto out;
+	}
+	ima_read_write_check(OPEN_WRITERS, iint, inode, dentry->d_name.name);
+out:
+	ima_inc_counts(iint, file->f_mode);
+	mutex_unlock(&iint->mutex);
+
+	kref_put(&iint->refcount, iint_free);
+}
+
+/*
  * Decrement ima counts
  */
 static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
@@ -153,123 +224,6 @@ void ima_file_free(struct file *file)
 	kref_put(&iint->refcount, iint_free);
 }
 
-/* ima_read_write_check - reflect possible reading/writing errors in the PCR.
- *
- * When opening a file for read, if the file is already open for write,
- * the file could change, resulting in a file measurement error.
- *
- * Opening a file for write, if the file is already open for read, results
- * in a time of measure, time of use (ToMToU) error.
- *
- * In either case invalidate the PCR.
- */
-enum iint_pcr_error { TOMTOU, OPEN_WRITERS };
-static void ima_read_write_check(enum iint_pcr_error error,
-				 struct ima_iint_cache *iint,
-				 struct inode *inode,
-				 const unsigned char *filename)
-{
-	switch (error) {
-	case TOMTOU:
-		if (iint->readcount > 0)
-			ima_add_violation(inode, filename, "invalid_pcr",
-					  "ToMToU");
-		break;
-	case OPEN_WRITERS:
-		if (iint->writecount > 0)
-			ima_add_violation(inode, filename, "invalid_pcr",
-					  "open_writers");
-		break;
-	}
-}
-
-static int get_path_measurement(struct ima_iint_cache *iint, struct file *file,
-				const unsigned char *filename)
-{
-	int rc = 0;
-
-	ima_inc_counts(iint, file->f_mode);
-
-	rc = ima_collect_measurement(iint, file);
-	if (!rc)
-		ima_store_measurement(iint, file, filename);
-	return rc;
-}
-
-/**
- * ima_path_check - based on policy, collect/store measurement.
- * @path: contains a pointer to the path to be measured
- * @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE
- *
- * Measure the file being open for readonly, based on the
- * ima_must_measure() policy decision.
- *
- * Keep read/write counters for all files, but 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.
- *
- * Always return 0 and audit dentry_open failures.
- * (Return code will be based upon measurement appraisal.)
- */
-int ima_path_check(struct path *path, int mask)
-{
-	struct inode *inode = path->dentry->d_inode;
-	struct ima_iint_cache *iint;
-	struct file *file = NULL;
-	int rc;
-
-	if (!ima_initialized || !S_ISREG(inode->i_mode))
-		return 0;
-	iint = ima_iint_find_get(inode);
-	if (!iint)
-		return 0;
-
-	mutex_lock(&iint->mutex);
-
-	rc = ima_must_measure(iint, inode, MAY_READ, PATH_CHECK);
-	if (rc < 0)
-		goto out;
-
-	if ((mask & MAY_WRITE) || (mask == 0))
-		ima_read_write_check(TOMTOU, iint, inode,
-				     path->dentry->d_name.name);
-
-	if ((mask & (MAY_WRITE | MAY_READ | MAY_EXEC)) != MAY_READ)
-		goto out;
-
-	ima_read_write_check(OPEN_WRITERS, iint, inode,
-			     path->dentry->d_name.name);
-	if (!(iint->flags & IMA_MEASURED)) {
-		struct dentry *dentry = dget(path->dentry);
-		struct vfsmount *mnt = mntget(path->mnt);
-
-		file = dentry_open(dentry, mnt, O_RDONLY | O_LARGEFILE,
-				   current_cred());
-		if (IS_ERR(file)) {
-			int audit_info = 0;
-
-			integrity_audit_msg(AUDIT_INTEGRITY_PCR, inode,
-					    dentry->d_name.name,
-					    "add_measurement",
-					    "dentry_open failed",
-					    1, audit_info);
-			file = NULL;
-			goto out;
-		}
-		rc = get_path_measurement(iint, file, dentry->d_name.name);
-	}
-out:
-	mutex_unlock(&iint->mutex);
-	if (file)
-		fput(file);
-	kref_put(&iint->refcount, iint_free);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(ima_path_check);
-
 static int process_measurement(struct file *file, const unsigned char *filename,
 			       int mask, int function)
 {
@@ -297,33 +251,6 @@ out:
 	return rc;
 }
 
-/*
- * ima_counts_get - increment file counts
- *
- * - for IPC shm and shmat file.
- * - for nfsd exported files.
- *
- * Increment the counts for these files to prevent unnecessary
- * imbalance messages.
- */
-void ima_counts_get(struct file *file)
-{
-	struct inode *inode = file->f_dentry->d_inode;
-	struct ima_iint_cache *iint;
-
-	if (!ima_initialized || !S_ISREG(inode->i_mode))
-		return;
-	iint = ima_iint_find_get(inode);
-	if (!iint)
-		return;
-	mutex_lock(&iint->mutex);
-	ima_inc_counts(iint, file->f_mode);
-	mutex_unlock(&iint->mutex);
-
-	kref_put(&iint->refcount, iint_free);
-}
-EXPORT_SYMBOL_GPL(ima_counts_get);
-
 /**
  * ima_file_mmap - based on policy, collect/store measurement.
  * @file: pointer to the file to be measured (May be NULL)
@@ -369,6 +296,25 @@ int ima_bprm_check(struct linux_binprm *bprm)
 	return 0;
 }
 
+/**
+ * ima_path_check - based on policy, collect/store measurement.
+ * @file: pointer to the file to be measured
+ * @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE
+ *
+ * Measure files based on the ima_must_measure() policy decision.
+ *
+ * Always return 0 and audit dentry_open failures.
+ * (Return code will be based upon measurement appraisal.)
+ */
+int ima_path_check(struct file *file, int mask)
+{
+	int rc;
+
+	rc = process_measurement(file, file->f_dentry->d_name.name,
+				 mask, PATH_CHECK);
+	return 0;
+}
+
 static int __init init_ima(void)
 {
 	int error;
-- 
1.6.0.6

Re: [RFC PATCH 1/2] Fix 1 untangling ima mess, part 2 with counters

[Al Viro]

On Wed, Jan 20, 2010 at 03:35:40PM -0500, Mimi Zohar wrote:
> The "Untangling ima mess, part 2 with counters" patch messed
> up the counters.  Based on conversations with Al Viro, this patch
> streamlines ima_path_check() by removing the counter maintaince.
> The counters are now updated independently, from measuring the file,
> in __dentry_open() and alloc_file() by calling ima_counts_get().
> ima_path_check() is called from nfsd and do_filp_open().
> 
> Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> ---
>  fs/namei.c                        |    4 +-
>  include/linux/ima.h               |    4 +-
>  security/integrity/ima/ima_main.c |  234 ++++++++++++++-----------------------

Um...
	a) where's the nfsd part?
	b) will that work if we open file with O_WRONLY?

nfsd side of things is non-trivial.  Note that you have that thing called
an awful lot; nfsd_permission() is called by fh_verify().  For which
operations do you really want it to happen?  Should it just migrate to
nfsd_open()?

Re: [RFC PATCH 1/2] Fix 1 untangling ima mess, part 2 with counters

[Mimi Zohar]

On Sat, 2010-01-23 at 23:07 +0000, Al Viro wrote:
> On Wed, Jan 20, 2010 at 03:35:40PM -0500, Mimi Zohar wrote:
> > The "Untangling ima mess, part 2 with counters" patch messed
> > up the counters.  Based on conversations with Al Viro, this patch
> > streamlines ima_path_check() by removing the counter maintaince.
> > The counters are now updated independently, from measuring the file,
> > in __dentry_open() and alloc_file() by calling ima_counts_get().
> > ima_path_check() is called from nfsd and do_filp_open().
> > 
> > Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
> > ---
> >  fs/namei.c                        |    4 +-
> >  include/linux/ima.h               |    4 +-
> >  security/integrity/ima/ima_main.c |  234 ++++++++++++++-----------------------
> 
> Um...
> 	a) where's the nfsd part?
> 	b) will that work if we open file with O_WRONLY?

Files opened O_WRONLY are not measured.

> nfsd side of things is non-trivial.  Note that you have that thing called
> an awful lot; nfsd_permission() is called by fh_verify().  For which
> operations do you really want it to happen?  Should it just migrate to
> nfsd_open()?

The IMA counters are updated in alloc_file() and __dentry_open().
__dentry_open() is called from a couple of places:
lookup_instantiate_filp(), nameidata_to_filp() and dentry_open.  Of
these calls, files are only being measured in the nameidata_to_filp()
path.  So yes, the current ima_path_check() needs to be moved to after
the dentry_open() in nfsd_open(), and also added after each of the other
dentry_open() and lookup_instantiate_filp() calls.  Otherwise the
counters will be correct, but the files will not be measured.

Eric's proposed patches had two calls to ima_path_check().  Was sure
looking forward to not having to add ima_path_check() calls all over.

Mimi

Re: [RFC PATCH 1/2] Fix 1 untangling ima mess, part 2 with counters

[Al Viro]

On Mon, Jan 25, 2010 at 02:24:37PM -0500, Mimi Zohar wrote:

> The IMA counters are updated in alloc_file() and __dentry_open().
> __dentry_open() is called from a couple of places:
> lookup_instantiate_filp(), nameidata_to_filp() and dentry_open.  Of
> these calls, files are only being measured in the nameidata_to_filp()
> path.  So yes, the current ima_path_check() needs to be moved to after
> the dentry_open() in nfsd_open(), and also added after each of the other
> dentry_open() and lookup_instantiate_filp() calls.  Otherwise the
> counters will be correct, but the files will not be measured.

Wrong.  lookup_instantiate_filp() is followed by do_filp_open() ones.
So no, we don't need to add there.  As for other dentry_open(), I'm
not at all convinced that we *want* ima_path_check() done for all
of those; it should be decided on per-call basis and it's not a trivial
decision.

Re: [RFC PATCH 1/2] Fix 1 untangling ima mess, part 2 with counters

[Al Viro]

OK, this stuff is in for-next, with the following changes:
	* nfsd call has been moved to nfsd_open()
	* patches reordered
	* masking irrelevant bits (i.e. leaving only MAY_{READ,WRITE,EXEC})
has been taken to ima_path_check(); all callers do it and it's safer that
way anyway.

Please, see if it's OK with you in this form; other calls of dentry_open()
are separate story, we'll have to see which ones should and which ones
should not get ima_path_check().  Do you have any other problems with that
one?  If not, it's going to migrate into for-linus and into the mainline;
this stuff is definitely 2.6.33 fodder.

Re: [RFC PATCH 1/2] Fix 1 untangling ima mess, part 2 with counters

[Mimi Zohar]

On Tue, 2010-01-26 at 13:03 +0000, Al Viro wrote:
> OK, this stuff is in for-next, with the following changes:
> 	* nfsd call has been moved to nfsd_open()
> 	* patches reordered
> 	* masking irrelevant bits (i.e. leaving only MAY_{READ,WRITE,EXEC})
> has been taken to ima_path_check(); all callers do it and it's safer that
> way anyway.
> 
> Please, see if it's OK with you in this form; other calls of dentry_open()
> are separate story, we'll have to see which ones should and which ones
> should not get ima_path_check().  Do you have any other problems with that
> one?  If not, it's going to migrate into for-linus and into the mainline;
> this stuff is definitely 2.6.33 fodder.

I'll look at the patches, but in the meantime we also needed the
following patch.

commit 8262bb85da4a71c4ff8c9b22e03aff11f8427b6d
Author: Eric Paris <eparis@redhat.com>
Date:   Wed Dec 9 15:29:01 2009 -0500

    ima: initialize ima before inodes can be allocated
    
    ima wants to create an inode information struct (iint) when inodes are
    allocated.  This means that at least the part of ima which does this
    allocation (the allocation is filled with information later) should
    before any inodes are created.  To accomplish this we split the ima
    initialization routine placing the kmem cache allocator inside a
    security_initcall() function.  Since this makes use of radix trees we also
    need to make sure that is initialized before security_initcall().
    
    Signed-off-by: Eric Paris <eparis@redhat.com>
    Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

Index: security-testing-2.6/init/main.c
===================================================================
--- security-testing-2.6.orig/init/main.c
+++ security-testing-2.6/init/main.c
@@ -663,9 +663,9 @@ asmlinkage void __init start_kernel(void
 	proc_caches_init();
 	buffer_init();
 	key_init();
+	radix_tree_init();
 	security_init();
 	vfs_caches_init(totalram_pages);
-	radix_tree_init();
 	signals_init();
 	/* rootfs populating might need page-writeback */
 	page_writeback_init();
Index: security-testing-2.6/security/integrity/ima/ima.h
===================================================================
--- security-testing-2.6.orig/security/integrity/ima/ima.h
+++ security-testing-2.6/security/integrity/ima/ima.h
@@ -65,7 +65,6 @@ void integrity_audit_msg(int audit_msgno
 			 const char *cause, int result, int info);
 
 /* Internal IMA function definitions */
-void ima_iintcache_init(void);
 int ima_init(void);
 void ima_cleanup(void);
 int ima_fs_init(void);
Index: security-testing-2.6/security/integrity/ima/ima_iint.c
===================================================================
--- security-testing-2.6.orig/security/integrity/ima/ima_iint.c
+++ security-testing-2.6/security/integrity/ima/ima_iint.c
@@ -52,9 +52,6 @@ int ima_inode_alloc(struct inode *inode)
 	struct ima_iint_cache *iint = NULL;
 	int rc = 0;
 
-	if (!ima_initialized)
-		return 0;
-
 	iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
 	if (!iint)
 		return -ENOMEM;
@@ -118,8 +115,6 @@ void ima_inode_free(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
-	if (!ima_initialized)
-		return;
 	spin_lock(&ima_iint_lock);
 	iint = radix_tree_delete(&ima_iint_store, (unsigned long)inode);
 	spin_unlock(&ima_iint_lock);
@@ -141,9 +136,11 @@ static void init_once(void *foo)
 	kref_set(&iint->refcount, 1);
 }
 
-void __init ima_iintcache_init(void)
+static int __init ima_iintcache_init(void)
 {
 	iint_cache =
 	    kmem_cache_create("iint_cache", sizeof(struct ima_iint_cache), 0,
 			      SLAB_PANIC, init_once);
+	return 0;
 }
+security_initcall(ima_iintcache_init);
Index: security-testing-2.6/security/integrity/ima/ima_main.c
===================================================================
--- security-testing-2.6.orig/security/integrity/ima/ima_main.c
+++ security-testing-2.6/security/integrity/ima/ima_main.c
@@ -319,7 +319,6 @@ static int __init init_ima(void)
 {
 	int error;
 
-	ima_iintcache_init();
 	error = ima_init();
 	ima_initialized = 1;
 	return error;
 

Re: [RFC PATCH 1/2] Fix 1 untangling ima mess, part 2 with counters

[Al Viro]

On Tue, Jan 26, 2010 at 10:16:55AM -0500, Mimi Zohar wrote:
> --- security-testing-2.6.orig/init/main.c
> +++ security-testing-2.6/init/main.c
> @@ -663,9 +663,9 @@ asmlinkage void __init start_kernel(void
>  	proc_caches_init();
>  	buffer_init();
>  	key_init();
> +	radix_tree_init();
>  	security_init();
>  	vfs_caches_init(totalram_pages);
> -	radix_tree_init();
>  	signals_init();
>  	/* rootfs populating might need page-writeback */
>  	page_writeback_init();

FWIW, I'd take radix_tree_init() much earlier than that; it's a common
infrastructure and if we have it early it'll mean less headache.

AFAICS, it can go right after mm_init().  Objections, anyone?

Other than that, I'm OK with that one.

Open Intents, lookup_instantiate_filp() And All That Shit(tm)

[Al Viro]

[fsdevel Cc'd; see the horror story/semi-rant below]

On Tue, Jan 26, 2010 at 12:59:07PM -0500, Mimi Zohar wrote:

> I still don't see that the
> calls to do_filp_open() in the lookup_instantiate_filp(), but will take
> a closer look once these patches are out.  Also not sure if there is
> anything in the alloc_file() path.

OK...  lookup_instantiate_filp() is a god-awful mess, so it's OK to be
confused by it - its authors definitely had been ;-)

The thing is, currently lookup_instantiate_filp() is called *only* from
call chains starting in do_filp_open().  And place right after
nameidata_to_filp() is downstream from both those call chains and
from the call chain leading from do_filp_open() to dentry_open().

Gory and revolting details follow.

What's happening is easier to understand with code massage done in
vfs-2.6.git#untested; basically, the logics of do_filp_open() (in
_very_ obfuscated form in the mainline kernel) is this:
	* find parent directory + last component of pathname
	* do "open or create file with this name in that directory" actions
	* we can
		+ get opened struct file.  We are done.
		+ get ERR_PTR(-error).  Fail.
		+ be told that it's a symlink
	* try to follow it
		+ if it fails, fail.
		+ if it's a "direct" symlink a-la procfs, we just get to
open the file it points to.  It *will* exist, so O_CREAT|O_EXCL => fail,
and O_CREAT alone gets ignored.
		+ if it's a normal symlink, we'd just followed it up to
the last component.  Now we have new directory and new filename; repeat
the steps above.

That's the high-level overview.  The reason for that kind of loop lies in
the messy semantics of O_CREAT on dangling symlinks; nevermind that part
of the horror show for now.

The really interesting part is opening a file in known directory.
Normal filesystems do d_lookup + either ->lookup() or ->d_revalidate()
+ dentry_open() which will call ->open().  For file creation it may become
d_lookup + either ->lookup() or ->d_revalidate() + ->create() + dentry_open().

*However*, NFS really wants to issue a single request to server that would
do lookup + maybe create + open in one step.  Atomically.  Fair enough,
but they have a nasty problem - which of existing fs methods would do that?
Neither ->d_revalidate() nor ->lookup() are going to get struct file from
their caller, after all...

Now, the sane solution would be to introduce a new method that would do
lookup-or-d_revalidate+maybe-create+open.  With normal filesystems defaulting
to the usual sequence.  And have its caller pass all we need to it (struct
file, flags, mode, etc.).  That, BTW, is where #untested leads to - a large
part of things done in fs/namei.c:do_last() there will eventually bud off
into default instance of that new method.

However, that's *NOT* what had been done.  Instead of that the arguments
(struct file *, etc.) are hidden in struct nameidata and a pointer
to nameidata is added to argument lists of ->d_revalidate(), ->lookup(), etc.
And NFS has added "let's talk to server and try atomic open" into those
methods.  If that attempt succeeds, they call a helper
(lookup_instantiate_filp()) that fills the struct file indirectly passed
to the damn thing via pointer in nameidata.

Eventually, we emerge from all that mess back into do_filp_open().  There
we look at the struct file that had been refered to from nameidata and
check if it had already been filled.  _THEN_ we do dentry_open() in case
file hadn't been already filled by call of lookup_instantiate_filp() issued
by one of fs methods.   That's nameidata_to_filp().

So any call of lookup_instantiate_filp() will be followed by nameidata_to_filp()
and that's where we'll pick the results of the former.  If there'd been no
such call (we are on a normal fs, or NFS doesn't feel like doing atomic open
for that one), nameidata_to_filp() will be the place that calls dentry_open().

IOW, the point directly after nameidata_to_filp() is where all those paths
converge.  And yes, it's a horrible mess.  One I'd been slowly massaging into
saner shape for a year already ;-/

Eventually nameidata_to_filp() and lookup_instantiate_filp() will go to hell
and we'll get one or two methods getting unopened struct file, dentry (unhashed
new one or found in dcache) and the rest of open() arguments.  Default will
be to do ->lookup() or ->d_revalidate() + possibly ->create() + dentry_open();
NFS will be able to do its atomic open stuff ending with dentry_open().
In either case we'll get success, error or "it's a symlink, deal with it
yourself" and caller will act accordingly.  And that'll be the place to
do any immediately-after-open things, including ima_path_check().  With
only do_filp_open() ever calling that method.

Re: [RFC PATCH 1/2] Fix 1 untangling ima mess, part 2 with counters

[Mimi Zohar]

On Tue, 2010-01-26 at 13:03 +0000, Al Viro wrote: 
> OK, this stuff is in for-next, with the following changes:
> 	* nfsd call has been moved to nfsd_open()
> 	* patches reordered
> 	* masking irrelevant bits (i.e. leaving only MAY_{READ,WRITE,EXEC})
> has been taken to ima_path_check(); all callers do it and it's safer that
> way anyway.
> 
> Please, see if it's OK with you in this form; other calls of dentry_open()
> are separate story, we'll have to see which ones should and which ones
> should not get ima_path_check().  Do you have any other problems with that
> one?  If not, it's going to migrate into for-linus and into the mainline;
> this stuff is definitely 2.6.33 fodder.

Other than missing the export for ima_path_check() for nfs and the
re-ordering not being bisect safe, they look good.  I've updated
the following two patches, from Eric's patchset, and would appreciate
your applying them as well:

ima: rename ima_path_check to ima_file_check
ima: rename PATH_CHECK to FILE_CHECK

Posting them separately.

Mimi

[RFC PATCH 2/2] Fix 2 untangling ima mess, part 2 with counters

[Mimi Zohar]

The "Untangling ima mess, part 2 with counters" patch did
not measure all files, which should have been measured.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
---
 fs/namei.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index a89ebc3..e1f15e8 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1730,7 +1730,7 @@ do_last:
 		if (nd.root.mnt)
 			path_put(&nd.root);
 		if (!IS_ERR(filp)) {
-			error = ima_path_check(filp, filp->f_mode &
+			error = ima_path_check(filp, acc_mode &
 				       (MAY_READ | MAY_WRITE | MAY_EXEC));
 			if (error) {
 				fput(filp);
@@ -1791,7 +1791,7 @@ ok:
 	}
 	filp = nameidata_to_filp(&nd, open_flag);
 	if (!IS_ERR(filp)) {
-		error = ima_path_check(filp, filp->f_mode &
+		error = ima_path_check(filp, acc_mode &
 			       (MAY_READ | MAY_WRITE | MAY_EXEC));
 		if (error) {
 			fput(filp);
-- 
1.6.0.6