[PATCH 0/10] eCryptfs Updates

[PATCH 0/10] eCryptfs Updates

[Michael Halcrow]

This patch set addresses file locking, NULL pointer checks, and fsync
issues that Christoph Hellwig pointed out. There are other remaining
issues that Christoph brought up that require more work; we will be
sending another patch set to address those issues in the near future.

Mike

[PATCH 8/10] Remove unnecessary NULL checks

[Mike Halcrow]

Remove unnecessary checks for NULL for values that should never be
NULL. A couple of minor comment/spacing fixes.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/dentry.c |    4 ----
 fs/ecryptfs/file.c   |   24 ++++--------------------
 fs/ecryptfs/inode.c  |   18 ------------------
 fs/ecryptfs/mmap.c   |    7 -------
 fs/ecryptfs/super.c  |    3 ---
 5 files changed, 4 insertions(+), 52 deletions(-)

080439fe5ee41e1cff6f947722c7be027c295583
diff --git a/fs/ecryptfs/dentry.c b/fs/ecryptfs/dentry.c
index 12362c0..7b1018a 100644
--- a/fs/ecryptfs/dentry.c
+++ b/fs/ecryptfs/dentry.c
@@ -47,10 +47,6 @@ static int ecryptfs_d_revalidate(struct 
 	struct vfsmount *saved_vfsmount;
 
 	lower_dentry = ecryptfs_dentry_to_lower(dentry);
-	if (!lower_dentry) {
-		err = 0;
-		goto out;
-	}
 	if (!lower_dentry->d_op || !lower_dentry->d_op->d_revalidate)
 		goto out;
 	saved_dentry = nd->dentry;
diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index b8fcd0a..3a9cd15 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -337,12 +337,6 @@ ecryptfs_fsync(struct file *file, struct
 			mutex_unlock(&lower_dentry->d_inode->i_mutex);
 		}
 	} else {
-		if (!ecryptfs_file_to_private(file)) {
-			rc = -EINVAL;
-			ecryptfs_printk(KERN_ERR, "ecryptfs_file_to_private"
-					"(file=[%p]) is NULL\n", file);
-			goto out;
-		}
 		lower_file = ecryptfs_file_to_lower(file);
 		lower_dentry = ecryptfs_dentry_to_lower(dentry);
 		if (lower_file->f_op && lower_file->f_op->fsync) {
@@ -352,7 +346,6 @@ ecryptfs_fsync(struct file *file, struct
 			mutex_unlock(&lower_dentry->d_inode->i_mutex);
 		}
 	}
-out:
 	return rc;
 }
 
@@ -466,15 +459,9 @@ static int ecryptfs_fasync(int fd, struc
 	int rc = 0;
 	struct file *lower_file = NULL;
 
-	if (NULL != ecryptfs_file_to_private(file))
-		lower_file = ecryptfs_file_to_lower(file);
-	else {
-		rc = -EINVAL;
-		goto out;
-	}
+	lower_file = ecryptfs_file_to_lower(file);
 	if (lower_file->f_op && lower_file->f_op->fasync)
 		rc = lower_file->f_op->fasync(fd, lower_file, flag);
-out:
 	return rc;
 }
 
@@ -483,9 +470,7 @@ static int ecryptfs_lock(struct file *fi
 	int rc = 0;
 	struct file *lower_file = NULL;
 
-	if (ecryptfs_file_to_private(file))
-		lower_file = ecryptfs_file_to_lower(file);
-	BUG_ON(!lower_file);
+	lower_file = ecryptfs_file_to_lower(file);
 	rc = -EINVAL;
 	if (!fl)
 		goto out;
@@ -513,9 +498,7 @@ static ssize_t ecryptfs_sendfile(struct 
 	struct file *lower_file = NULL;
 	int rc = -EINVAL;
 
-	if (ecryptfs_file_to_private(file))
-		lower_file = ecryptfs_file_to_lower(file);
-	BUG_ON(!lower_file);
+	lower_file = ecryptfs_file_to_lower(file);
 	if (lower_file->f_op && lower_file->f_op->sendfile)
 		rc = lower_file->f_op->sendfile(lower_file, ppos, count,
 						actor, target);
@@ -561,6 +544,7 @@ ecryptfs_ioctl(struct inode *inode, stru
 {
 	int rc = 0;
 	struct file *lower_file = NULL;
+
 	if (ecryptfs_file_to_private(file))
 		lower_file = ecryptfs_file_to_lower(file);
 	if (lower_file && lower_file->f_op && lower_file->f_op->ioctl)
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index c24b043..47e4202 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -159,12 +159,6 @@ ecryptfs_do_create(struct inode *directo
 	struct dentry *lower_dir_dentry;
 
 	lower_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry);
-	if (IS_ERR(lower_dentry)) {
-		ecryptfs_printk(KERN_ERR, "ecryptfs dentry doesn't know"
-				"about its lower counterpart\n");
-		rc = PTR_ERR(lower_dentry);
-		goto out;
-	}
 	lower_dir_dentry = lock_parent(lower_dentry);
 	if (unlikely(IS_ERR(lower_dir_dentry))) {
 		ecryptfs_printk(KERN_ERR, "Error locking directory of "
@@ -252,12 +246,6 @@ static int ecryptfs_initialize_file(stru
 	struct vfsmount *lower_mnt;
 
 	lower_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry);
-	if (IS_ERR(lower_dentry)) {
-		ecryptfs_printk(KERN_ERR, "ecryptfs dentry doesn't know"
-				"about its lower counterpart\n");
-		rc = PTR_ERR(lower_dentry);
-		goto out;
-	}
 	ecryptfs_printk(KERN_DEBUG, "lower_dentry->d_name.name = [%s]\n",
 			lower_dentry->d_name.name);
 	inode = ecryptfs_dentry->d_inode;
@@ -831,12 +819,6 @@ int ecryptfs_truncate(struct dentry *den
 	if (unlikely((new_length == i_size)))
 		goto out;
 	crypt_stat = &ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat;
-	if (unlikely(!crypt_stat)) {
-		ecryptfs_printk(KERN_ERR, "NULL crypt_stat on dentry with "
-				"d_name.name = [%s]\n", dentry->d_name.name);
-		rc = -EINVAL;
-		goto out;
-	}
 	/* Set up a fake ecryptfs file, this is used to interface with
 	 * the file in the underlying filesystem so that the
 	 * truncation has an effect there as well. */
diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index 0c0411a..e9cff02 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -219,11 +219,6 @@ int ecryptfs_do_readpage(struct file *fi
 	const struct address_space_operations *lower_a_ops;
 
 	dentry = file->f_dentry;
-	if (!ecryptfs_file_to_private(file)) {
-		rc = -ENOENT;
-		ecryptfs_printk(KERN_ERR, "No lower file info\n");
-		goto out;
-	}
 	lower_file = ecryptfs_file_to_lower(file);
 	lower_dentry = ecryptfs_dentry_to_lower(dentry);
 	inode = dentry->d_inode;
@@ -646,8 +641,6 @@ static int ecryptfs_commit_write(struct 
 	mutex_lock(&lower_inode->i_mutex);
 	crypt_stat =
 		&ecryptfs_inode_to_private(file->f_dentry->d_inode)->crypt_stat;
-	BUG_ON(!crypt_stat);
-	BUG_ON(!lower_file);
 	if (ECRYPTFS_CHECK_FLAG(crypt_stat->flags, ECRYPTFS_NEW_FILE)) {
 		ecryptfs_printk(KERN_DEBUG, "ECRYPTFS_NEW_FILE flag set in "
 			"crypt_stat at memory location [%p]\n", crypt_stat);
diff --git a/fs/ecryptfs/super.c b/fs/ecryptfs/super.c
index 1d344e0..94c50f1 100644
--- a/fs/ecryptfs/super.c
+++ b/fs/ecryptfs/super.c
@@ -37,9 +37,6 @@ struct kmem_cache *ecryptfs_inode_info_c
  *
  * Called to bring an inode into existence.
  *
- * Note that setting the self referencing pointer doesn't work here:
- * 	i.e. ECRYPTFS_INODE_TO_PRIVATE_SM(inode) = ei;
- *
  * Only handle allocation, setting up structures should be done in
  * ecryptfs_read_inode. This is because the kernel, between now and
  * then, will 0 out the private data pointer.
-- 
1.3.3

[PATCH 5/10] Remove unnecessary #ifndef's

[Mike Halcrow]

Sorry these got in there; they are really only useful for the
userspace tools.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/ecryptfs_kernel.h |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

28ea0fb87bb7dafb8dbf327f86359b9e80c40bb5
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index e8b8691..8e35dbd 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -31,16 +31,9 @@ #include <linux/fs.h>
 #include <linux/scatterlist.h>
 
 /* Version verification for shared data structures w/ userspace */
-#ifndef ECRYPTFS_VERSION_MAJOR
 #define ECRYPTFS_VERSION_MAJOR 0x00
-#endif
-#ifndef ECRYPTFS_VERSION_MINOR
 #define ECRYPTFS_VERSION_MINOR 0x01
-#endif
-
-#ifndef ECRYPTFS_SUPPORTED_FILE_VERSION
 #define ECRYPTFS_SUPPORTED_FILE_VERSION 0x01
-#endif
 
 #define ECRYPTFS_MAX_PASSWORD_LENGTH 64
 #define ECRYPTFS_MAX_PASSPHRASE_BYTES ECRYPTFS_MAX_PASSWORD_LENGTH
@@ -168,9 +161,7 @@ #define ECRYPTFS_DEFAULT_KEY_BYTES 16
 #define ECRYPTFS_DEFAULT_CHAINING_MODE CRYPTO_TFM_MODE_CBC
 #define ECRYPTFS_TAG_3_PACKET_TYPE 0x8C
 #define ECRYPTFS_TAG_11_PACKET_TYPE 0xED
-#ifndef MD5_DIGEST_SIZE
 #define MD5_DIGEST_SIZE 16
-#endif
 
 /**
  * This is the primary struct associated with each encrypted file.
-- 
1.3.3

[PATCH 10/10] Overhaul file locking

[Mike Halcrow]

Use exported posix_lock_file_wait() rather than re-implement it. Note
that ecryptfs_getlk() and ecryptfs_setlk() still need work; we need to
figure out how to best integrate with the existing fcntl_setlk() and
fcntl_getlk() functions in fs/locks.c.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/file.c |   71 +++++++++++++---------------------------------------
 1 files changed, 18 insertions(+), 53 deletions(-)

494162519c1eee8de78405ac35302f488a6b8ef0
diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index c984ea6..c4ea9f0 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -336,40 +336,12 @@ ecryptfs_fsync(struct file *file, struct
 	return rc;
 }
 
-static void locks_delete_block(struct file_lock *waiter)
-{
-	lock_kernel();
-	list_del_init(&waiter->fl_block);
-	list_del_init(&waiter->fl_link);
-	waiter->fl_next = NULL;
-	unlock_kernel();
-}
-
-static int ecryptfs_posix_lock(struct file *file, struct file_lock *fl, int cmd)
-{
-	int rc;
-
-lock_file:
-	rc = posix_lock_file(file, fl);
-	if ((rc != -EAGAIN) || (cmd == F_SETLK))
-		goto out;
-	rc = wait_event_interruptible(fl->fl_wait, !fl->fl_next);
-	if (!rc)
-		goto lock_file;
-	locks_delete_block(fl);
-out:
-	return rc;
-}
-
 static int ecryptfs_setlk(struct file *file, int cmd, struct file_lock *fl)
 {
-	int rc = -EINVAL;
-	struct inode *inode, *lower_inode;
-	struct file *lower_file = NULL;
+	struct file *lower_file = ecryptfs_file_to_lower(file);
+	struct inode *lower_inode = lower_file->f_dentry->d_inode;
+	int rc;
 
-	lower_file = ecryptfs_file_to_lower(file);
-	inode = file->f_dentry->d_inode;
-	lower_inode = lower_file->f_dentry->d_inode;
 	/* Don't allow mandatory locks on files that may be memory mapped
 	 * and shared. */
 	if (IS_MANDLOCK(lower_inode) &&
@@ -378,7 +350,7 @@ static int ecryptfs_setlk(struct file *f
 		rc = -EAGAIN;
 		goto out;
 	}
-	if (cmd == F_SETLKW)
+	if (IS_SETLKW(cmd))
 		fl->fl_flags |= FL_SLEEP;
 	rc = -EBADF;
 	switch (fl->fl_type) {
@@ -406,29 +378,29 @@ static int ecryptfs_setlk(struct file *f
 			goto out;
 		goto upper_lock;
 	}
-	rc = ecryptfs_posix_lock(lower_file, fl, cmd);
+	rc = posix_lock_file_wait(lower_file, fl);
 	if (rc)
 		goto out;
 upper_lock:
 	fl->fl_file = file;
-	rc = ecryptfs_posix_lock(file, fl, cmd);
+	rc = posix_lock_file_wait(lower_file, fl);
 	if (rc) {
 		fl->fl_type = F_UNLCK;
 		fl->fl_file = lower_file;
-		ecryptfs_posix_lock(lower_file, fl, cmd);
+		rc = posix_lock_file_wait(lower_file, fl);
 	}
 out:
 	return rc;
 }
 
-static int ecryptfs_getlk(struct file *file, struct file_lock *fl)
+static int ecryptfs_getlk(struct file *file, int cmd, struct file_lock *fl)
 {
 	struct file_lock cfl;
 	struct file_lock *tempfl = NULL;
 	int rc = 0;
 
 	if (file->f_op && file->f_op->lock) {
-		rc = file->f_op->lock(file, F_GETLK, fl);
+		rc = file->f_op->lock(file, cmd, fl);
 		if (rc < 0)
 			goto out;
 	} else
@@ -454,27 +426,20 @@ static int ecryptfs_fasync(int fd, struc
 
 static int ecryptfs_lock(struct file *file, int cmd, struct file_lock *fl)
 {
-	int rc = 0;
-	struct file *lower_file = NULL;
+	struct file *lower_file = ecryptfs_file_to_lower(file);
+	int rc = -EINVAL;
 
-	lower_file = ecryptfs_file_to_lower(file);
-	rc = -EINVAL;
 	if (!fl)
 		goto out;
-	fl->fl_file = lower_file;
-	switch (cmd) {
-	case F_GETLK:
-		rc = ecryptfs_getlk(lower_file, fl);
-		break;
-	case F_SETLK:
-	case F_SETLKW:
+	if (IS_GETLK(cmd)) {
+		fl->fl_file = lower_file;
+		rc = ecryptfs_getlk(lower_file, cmd, fl);
+		fl->fl_file = file;
+	} else if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
 		fl->fl_file = file;
 		rc = ecryptfs_setlk(file, cmd, fl);
-		break;
-	default:
-		rc = -EINVAL;
-	}
-	fl->fl_file = file;
+	} else
+		fl->fl_file = file;
 out:
 	return rc;
 }
-- 
1.3.3

[PATCH 4/10] uint16_t -> u16

[Mike Halcrow]

uint16_t replaced with more kernel-appropriate u16.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

46e9182ba5ba7c1b503f23b69f3accce416f1b71
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index a0e9f05..9376482 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -141,7 +141,7 @@ static match_table_t tokens = {
  *
  * Returns zero on good version; non-zero otherwise
  */
-int ecryptfs_verify_version(uint16_t version)
+int ecryptfs_verify_version(u16 version)
 {
 	int rc = 0;
 	unsigned char major;
-- 
1.3.3

[PATCH 2/10] Clean up #include's

[Mike Halcrow]

Clean up a couple of #include's in the header file. Looks like there
may be a few more places where the #include's could be tweaked; we'll
address that in a future patch set.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/ecryptfs_kernel.h |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

41e02f240e66ee9c74871eaf0cadd3581cbb1980
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index b58e515..48fd0a1 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -32,8 +32,7 @@ #endif
 
 #include <keys/user-type.h>
 #include <linux/fs.h>
-#include <asm/semaphore.h>
-#include <asm/scatterlist.h>
+#include <linux/scatterlist.h>
 
 /* Version verification for shared data structures w/ userspace */
 #ifndef ECRYPTFS_VERSION_MAJOR
-- 
1.3.3

[PATCH 7/10] Remove extraneous read of inode size from header

[Mike Halcrow]

Christoph Hellwig pointed out that the inode is not live at this point
in ecryptfs_open(), and so this entire function is pointless.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/file.c |   47 +----------------------------------------------
 1 files changed, 1 insertions(+), 46 deletions(-)

35776954e1116224272c4e191aed9a85b63914f7
diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index 0284e16..b8fcd0a 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -189,49 +189,6 @@ retry:
 	return rc;
 }
 
-/**
- * read_inode_size_from_header
- * @lower_file: The lower file struct
- * @lower_inode: The lower inode
- * @inode: The ecryptfs inode
- *
- * Returns zero on success; non-zero otherwise
- */
-static int
-read_inode_size_from_header(struct file *lower_file,
-			    struct inode *lower_inode, struct inode *inode)
-{
-	int rc;
-	struct page *header_page;
-	unsigned char *header_virt;
-	u64 data_size;
-
-	header_page = grab_cache_page(lower_inode->i_mapping, 0);
-	if (!header_page) {
-		rc = -EINVAL;
-		ecryptfs_printk(KERN_ERR, "grab_cache_page for header page "
-				"failed\n");
-		goto out;
-	}
-	header_virt = kmap(header_page);
-	rc = lower_inode->i_mapping->a_ops->readpage(lower_file, header_page);
-	if (rc) {
-		ecryptfs_printk(KERN_ERR, "Error reading header page\n");
-		goto out_unmap;
-	}
-	memcpy(&data_size, header_virt, sizeof(data_size));
-	data_size = be64_to_cpu(data_size);
-	i_size_write(inode, (loff_t)data_size);
-	ecryptfs_printk(KERN_DEBUG, "inode w/ addr = [0x%p], i_ino = [0x%.16x] "
-			"size: [0x%.16x]\n", inode, inode->i_ino,
-			i_size_read(inode));
-out_unmap:
-	kunmap(header_page);
-	page_cache_release(header_page);
-out:
-	return rc;
-}
-
 struct kmem_cache *ecryptfs_file_info_cache;
 
 /**
@@ -320,9 +277,7 @@ static int ecryptfs_open(struct inode *i
 			 * going to default to -EIO. */
 			rc = -EIO;
 			goto out_puts;
-		} else
-			read_inode_size_from_header(lower_file, lower_inode,
-						    inode);
+		}
 	}
 	ecryptfs_printk(KERN_DEBUG, "inode w/ addr = [0x%p], i_ino = [0x%.16x] "
 			"size: [0x%.16x]\n", inode, inode->i_ino,
-- 
1.3.3

[PATCH 6/10] Remove ``NULL =='' syntax

[Mike Halcrow]

Remove ``NULL =='' syntax; easier on the eyes.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/file.c |    2 +-
 fs/ecryptfs/main.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

c6a4cc09586f339bd97330efcd388c92100a6780
diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index ef7d7fa..0284e16 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -371,7 +371,7 @@ ecryptfs_fsync(struct file *file, struct
 	struct file *lower_file = NULL;
 	struct dentry *lower_dentry;
 
-	if (NULL == file) {
+	if (!file) {
 		lower_dentry = ecryptfs_dentry_to_lower(dentry);
 		if (lower_dentry->d_inode->i_fop
 		    && lower_dentry->d_inode->i_fop->fsync) {
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index 9376482..5cbc948 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -98,7 +98,7 @@ int ecryptfs_interpose(struct dentry *lo
 		rc = -ENOMEM;
 		goto out;
 	}
-	if (NULL == ecryptfs_inode_to_lower(inode))
+	if (!ecryptfs_inode_to_lower(inode))
 		ecryptfs_set_inode_lower(inode, igrab(lower_inode));
 	if (S_ISLNK(lower_inode->i_mode))
 		inode->i_op = &ecryptfs_symlink_iops;
-- 
1.3.3

[PATCH 9/10] Rewrite ecryptfs_fsync()

[Mike Halcrow]

Rewrite ecryptfs_fsync() to be more concise. Note that i_fop is always
valid.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/file.c |   29 ++++++++---------------------
 1 files changed, 8 insertions(+), 21 deletions(-)

fb9079aa454161fdbdc53021b987eec7513fec06
diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index 3a9cd15..c984ea6 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -322,29 +322,16 @@ static int ecryptfs_release(struct inode
 static int
 ecryptfs_fsync(struct file *file, struct dentry *dentry, int datasync)
 {
+	struct file *lower_file = ecryptfs_file_to_lower(file);
+	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
+	struct inode *lower_inode = lower_dentry->d_inode;
 	int rc = -EINVAL;
-	struct file *lower_file = NULL;
-	struct dentry *lower_dentry;
 
-	if (!file) {
-		lower_dentry = ecryptfs_dentry_to_lower(dentry);
-		if (lower_dentry->d_inode->i_fop
-		    && lower_dentry->d_inode->i_fop->fsync) {
-			mutex_lock(&lower_dentry->d_inode->i_mutex);
-			rc = lower_dentry->d_inode->i_fop->fsync(lower_file,
-								 lower_dentry,
-								 datasync);
-			mutex_unlock(&lower_dentry->d_inode->i_mutex);
-		}
-	} else {
-		lower_file = ecryptfs_file_to_lower(file);
-		lower_dentry = ecryptfs_dentry_to_lower(dentry);
-		if (lower_file->f_op && lower_file->f_op->fsync) {
-			mutex_lock(&lower_dentry->d_inode->i_mutex);
-			rc = lower_file->f_op->fsync(lower_file, lower_dentry,
-						     datasync);
-			mutex_unlock(&lower_dentry->d_inode->i_mutex);
-		}
+	if (lower_inode->i_fop->fsync) {
+		mutex_lock(&lower_inode->i_mutex);
+		rc = lower_inode->i_fop->fsync(lower_file, lower_dentry,
+					       datasync);
+		mutex_unlock(&lower_inode->i_mutex);
 	}
 	return rc;
 }
-- 
1.3.3

[PATCH 3/10] Convert signed data types to unsigned data types

[Mike Halcrow]

Convert data types in structures in the header to be unsigned if they
are supposed to ever only contain unsigned values.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/ecryptfs_kernel.h |   46 +++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 23 deletions(-)

704eeca89309cfaed08992f03e100d465b458b56
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index 48fd0a1..6164161 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -80,18 +80,18 @@ #define ECRYPTFS_USERSPACE_SHOULD_TRY_TO
 #define ECRYPTFS_USERSPACE_SHOULD_TRY_TO_ENCRYPT 0x00000002
 #define ECRYPTFS_CONTAINS_DECRYPTED_KEY 0x00000004
 #define ECRYPTFS_CONTAINS_ENCRYPTED_KEY 0x00000008
-	s32 flags;
-	s32 encrypted_key_size;
-	s32 decrypted_key_size;
+	u32 flags;
+	u32 encrypted_key_size;
+	u32 decrypted_key_size;
 	u8 encrypted_key[ECRYPTFS_MAX_ENCRYPTED_KEY_BYTES];
 	u8 decrypted_key[ECRYPTFS_MAX_KEY_BYTES];
 };
 
 struct ecryptfs_password {
-	s32 password_bytes;
+	u32 password_bytes;
 	s32 hash_algo;
-	s32 hash_iterations;
-	s32 session_key_encryption_key_bytes;
+	u32 hash_iterations;
+	u32 session_key_encryption_key_bytes;
 #define ECRYPTFS_PERSISTENT_PASSWORD 0x01
 #define ECRYPTFS_SESSION_KEY_ENCRYPTION_KEY_SET 0x02
 	u32 flags;
@@ -104,15 +104,15 @@ #define ECRYPTFS_SESSION_KEY_ENCRYPTION_
 
 /* May be a password or a private key */
 struct ecryptfs_auth_tok {
-	uint16_t version; /* 8-bit major and 8-bit minor */
+	u16 version; /* 8-bit major and 8-bit minor */
 #define ECRYPTFS_PASSWORD         0x00000001
 #define ECRYPTFS_PRIVATE_KEY      0x00000002
 #define ECRYPTFS_CONTAINS_SECRET  0x00000004
 #define ECRYPTFS_EXPIRED          0x00000008
 	u32 flags;
 	uid_t uid;
-	s64 creation_time;
-	s64 expiration_time;
+	u64 creation_time;
+	u64 expiration_time;
 	union {
 		struct ecryptfs_password password;
 		/* Private key is in future eCryptfs releases */
@@ -143,7 +143,7 @@ struct ecryptfs_page_crypt_context {
 	struct page *page;
 #define ECRYPTFS_PREPARE_COMMIT_MODE 0
 #define ECRYPTFS_WRITEPAGE_MODE      1
-	int mode;
+	unsigned int mode;
 	union {
 		struct file *lower_file;
 		struct writeback_control *wbc;
@@ -191,23 +191,22 @@ #define ECRYPTFS_ENABLE_HMAC        0x00
 #define ECRYPTFS_ENCRYPT_IV_PAGES   0x00000040
 #define ECRYPTFS_KEY_VALID          0x00000080
 	u32 flags;
-	int file_version;
-	int iv_bytes;
-	int num_keysigs;
-	int header_extent_size;
-	int num_header_extents_at_front; /* Number of header extents
-					  * at the front of the file */
-	int extent_size; /* Data extent size; default is 4096 */
-	int key_size_bits;
+	unsigned int file_version;
+	unsigned int iv_bytes;
+	unsigned int num_keysigs;
+	unsigned int header_extent_size;
+	unsigned int num_header_extents_at_front;
+	unsigned int extent_size; /* Data extent size; default is 4096 */
+	unsigned int key_size_bits;
 	unsigned int extent_shift;
 	unsigned int extent_mask;
 	struct crypto_tfm *tfm;
 	struct crypto_tfm *md5_tfm; /* Crypto context for generating
 				     * the initialization vectors */
-	char cipher[ECRYPTFS_MAX_CIPHER_NAME_SIZE];
+	unsigned char cipher[ECRYPTFS_MAX_CIPHER_NAME_SIZE];
 	unsigned char key[ECRYPTFS_MAX_KEY_BYTES];
 	unsigned char root_iv[ECRYPTFS_MAX_IV_BYTES];
-	char keysigs[ECRYPTFS_MAX_NUM_KEYSIGS][ECRYPTFS_SIG_SIZE_HEX];
+	unsigned char keysigs[ECRYPTFS_MAX_NUM_KEYSIGS][ECRYPTFS_SIG_SIZE_HEX];
 	struct mutex cs_mutex;
 };
 
@@ -234,8 +233,9 @@ struct ecryptfs_mount_crypt_stat {
 	/* Pointers to memory we do not own, do not free these */
 	struct ecryptfs_auth_tok *global_auth_tok;
 	struct key *global_auth_tok_key;
-	char global_default_cipher_name[ECRYPTFS_MAX_CIPHER_NAME_SIZE + 1];
-	char global_auth_tok_sig[ECRYPTFS_SIG_SIZE_HEX + 1];
+	unsigned char global_default_cipher_name[ECRYPTFS_MAX_CIPHER_NAME_SIZE
+						 + 1];
+	unsigned char global_auth_tok_sig[ECRYPTFS_SIG_SIZE_HEX + 1];
 };
 
 /* superblock private data. */
@@ -253,7 +253,7 @@ struct ecryptfs_file_info {
 
 /* auth_tok <=> encrypted_session_key mappings */
 struct ecryptfs_auth_tok_list_item {
-	char encrypted_session_key[ECRYPTFS_MAX_KEY_BYTES];
+	unsigned char encrypted_session_key[ECRYPTFS_MAX_KEY_BYTES];
 	struct list_head list;
 	struct ecryptfs_auth_tok auth_tok;
 };
-- 
1.3.3

[PATCH 1/10] Convert ASSERT to BUG_ON

[Mike Halcrow]

Use the kernel BUG_ON() macro rather than the eCryptfs ASSERT(). Note
that this temporarily renders the CONFIG_ECRYPT_DEBUG build option
unused. We certainly plan on using it in the future; for now, is it
okay to leave it in fs/Kconfig, or would you like to remove it?

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/ecryptfs/crypto.c          |   16 ++++++++--------
 fs/ecryptfs/ecryptfs_kernel.h |   17 -----------------
 fs/ecryptfs/file.c            |    4 ++--
 fs/ecryptfs/inode.c           |   18 +++++++++---------
 fs/ecryptfs/keystore.c        |    2 +-
 fs/ecryptfs/mmap.c            |    8 ++++----
 6 files changed, 24 insertions(+), 41 deletions(-)

acfbd08111d5f3d1f0aff5064791f86260899aeb
diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index 809d9f5..49b7eb3 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -245,9 +245,9 @@ static int encrypt_scatterlist(struct ec
 {
 	int rc = 0;
 
-	ASSERT(crypt_stat && crypt_stat->tfm
-	       && ECRYPTFS_CHECK_FLAG(crypt_stat->flags,
-				      ECRYPTFS_STRUCT_INITIALIZED));
+	BUG_ON(!crypt_stat || !crypt_stat->tfm
+	       || !ECRYPTFS_CHECK_FLAG(crypt_stat->flags,
+				       ECRYPTFS_STRUCT_INITIALIZED));
 	if (unlikely(ecryptfs_verbosity > 0)) {
 		ecryptfs_printk(KERN_DEBUG, "Key size [%d]; key:\n",
 				crypt_stat->key_size_bits / 8);
@@ -467,8 +467,8 @@ #define ECRYPTFS_PAGE_STATE_WRITTEN   3
 			prior_lower_page_idx = lower_page_idx;
 			page_state = ECRYPTFS_PAGE_STATE_READ;
 		}
-		ASSERT(page_state == ECRYPTFS_PAGE_STATE_MODIFIED
-		       || page_state == ECRYPTFS_PAGE_STATE_READ);
+		BUG_ON(!(page_state == ECRYPTFS_PAGE_STATE_MODIFIED
+			 || page_state == ECRYPTFS_PAGE_STATE_READ));
 		rc = ecryptfs_derive_iv(extent_iv, crypt_stat,
 					(base_extent + extent_offset));
 		if (rc) {
@@ -505,7 +505,7 @@ #define ECRYPTFS_PAGE_STATE_WRITTEN   3
 		page_state = ECRYPTFS_PAGE_STATE_MODIFIED;
 		extent_offset++;
 	}
-	ASSERT(orig_byte_offset == 0);
+	BUG_ON(orig_byte_offset != 0);
 	rc = ecryptfs_write_out_page(ctx, lower_page, lower_inode, 0,
 				     (lower_byte_offset
 				      + crypt_stat->extent_size));
@@ -792,8 +792,8 @@ int ecryptfs_compute_root_iv(struct ecry
 	int rc = 0;
 	char dst[MD5_DIGEST_SIZE];
 
-	ASSERT(crypt_stat->iv_bytes <= MD5_DIGEST_SIZE);
-	ASSERT(crypt_stat->iv_bytes > 0);
+	BUG_ON(crypt_stat->iv_bytes > MD5_DIGEST_SIZE);
+	BUG_ON(crypt_stat->iv_bytes <= 0);
 	if (!ECRYPTFS_CHECK_FLAG(crypt_stat->flags, ECRYPTFS_KEY_VALID)) {
 		rc = -EINVAL;
 		ecryptfs_printk(KERN_WARNING, "Session key not valid; "
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index b58e515..14b3f99 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -26,10 +26,6 @@
 #ifndef ECRYPTFS_KERNEL_H
 #define ECRYPTFS_KERNEL_H
 
-#ifdef CONFIG_ECRYPT_DEBUG
-#define OBSERVE_ASSERTS 1
-#endif
-
 #include <keys/user-type.h>
 #include <linux/fs.h>
 #include <asm/semaphore.h>
@@ -259,19 +255,6 @@ struct ecryptfs_auth_tok_list_item {
 	struct ecryptfs_auth_tok auth_tok;
 };
 
-#ifdef OBSERVE_ASSERTS
-#define ASSERT(EX)	                                                      \
-do {	                                                                      \
-        if (unlikely(!(EX))) {                                                \
-	        printk(KERN_CRIT "ASSERTION FAILED: %s at %s:%d (%s)\n", #EX, \
-	               __FILE__, __LINE__, __FUNCTION__);	              \
-                BUG();                                                        \
-        }	                                                              \
-} while (0)
-#else
-#define ASSERT(EX) do { /* nothing */ } while (0)
-#endif /* OBSERVE_ASSERTS */
-
 static inline struct ecryptfs_file_info *
 ecryptfs_file_to_private(struct file *file)
 {
diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index cb03aee..ef7d7fa 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -530,7 +530,7 @@ static int ecryptfs_lock(struct file *fi
 
 	if (ecryptfs_file_to_private(file))
 		lower_file = ecryptfs_file_to_lower(file);
-	ASSERT(lower_file);
+	BUG_ON(!lower_file);
 	rc = -EINVAL;
 	if (!fl)
 		goto out;
@@ -560,7 +560,7 @@ static ssize_t ecryptfs_sendfile(struct 
 
 	if (ecryptfs_file_to_private(file))
 		lower_file = ecryptfs_file_to_lower(file);
-	ASSERT(lower_file);
+	BUG_ON(!lower_file);
 	if (lower_file->f_op && lower_file->f_op->sendfile)
 		rc = lower_file->f_op->sendfile(lower_file, ppos, count,
 						actor, target);
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 3505dd7..c24b043 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -61,15 +61,15 @@ void ecryptfs_copy_inode_size(struct ino
 
 void ecryptfs_copy_attr_atime(struct inode *dest, const struct inode *src)
 {
-	ASSERT(dest != NULL);
-	ASSERT(src != NULL);
+	BUG_ON(!dest);
+	BUG_ON(!src);
 	dest->i_atime = src->i_atime;
 }
 
 void ecryptfs_copy_attr_times(struct inode *dest, const struct inode *src)
 {
-	ASSERT(dest != NULL);
-	ASSERT(src != NULL);
+	BUG_ON(!dest);
+	BUG_ON(!src);
 	dest->i_atime = src->i_atime;
 	dest->i_mtime = src->i_mtime;
 	dest->i_ctime = src->i_ctime;
@@ -78,8 +78,8 @@ void ecryptfs_copy_attr_times(struct ino
 static void ecryptfs_copy_attr_timesizes(struct inode *dest,
 					 const struct inode *src)
 {
-	ASSERT(dest != NULL);
-	ASSERT(src != NULL);
+	BUG_ON(!dest);
+	BUG_ON(!src);
 	dest->i_atime = src->i_atime;
 	dest->i_mtime = src->i_mtime;
 	dest->i_ctime = src->i_ctime;
@@ -88,8 +88,8 @@ static void ecryptfs_copy_attr_timesizes
 
 void ecryptfs_copy_attr_all(struct inode *dest, const struct inode *src)
 {
-	ASSERT(dest != NULL);
-	ASSERT(src != NULL);
+	BUG_ON(!dest);
+	BUG_ON(!src);
 	dest->i_mode = src->i_mode;
 	dest->i_nlink = src->i_nlink;
 	dest->i_uid = src->i_uid;
@@ -392,7 +392,7 @@ static struct dentry *ecryptfs_lookup(st
 		lower_dentry->d_name.name);
 	lower_inode = lower_dentry->d_inode;
 	ecryptfs_copy_attr_atime(dir, lower_dir_dentry->d_inode);
-	ASSERT(atomic_read(&lower_dentry->d_count));
+	BUG_ON(!atomic_read(&lower_dentry->d_count));
 	ecryptfs_set_dentry_private(dentry,
 				    kmem_cache_alloc(ecryptfs_dentry_info_cache,
 						     SLAB_KERNEL));
diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
index 19ba826..7c5ac0d 100644
--- a/fs/ecryptfs/keystore.c
+++ b/fs/ecryptfs/keystore.c
@@ -506,7 +506,7 @@ static int decrypt_session_key(struct ec
 	       auth_tok->session_key.encrypted_key_size);
 	src_sg[0].page = virt_to_page(encrypted_session_key);
 	src_sg[0].offset = 0;
-	ASSERT(auth_tok->session_key.encrypted_key_size < PAGE_CACHE_SIZE);
+	BUG_ON(auth_tok->session_key.encrypted_key_size > PAGE_CACHE_SIZE);
 	src_sg[0].length = auth_tok->session_key.encrypted_key_size;
 	dst_sg[0].page = virt_to_page(session_key);
 	dst_sg[0].offset = 0;
diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index 58f5e12..0c0411a 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -280,7 +280,7 @@ static int ecryptfs_readpage(struct file
 	int rc = 0;
 	struct ecryptfs_crypt_stat *crypt_stat;
 
-	ASSERT(file && file->f_dentry && file->f_dentry->d_inode);
+	BUG_ON(!(file && file->f_dentry && file->f_dentry->d_inode));
 	crypt_stat =
 		&ecryptfs_inode_to_private(file->f_dentry->d_inode)->crypt_stat;
 	if (!crypt_stat
@@ -553,7 +553,7 @@ process_new_file(struct ecryptfs_crypt_s
 	header_pages = ((crypt_stat->header_extent_size
 			 * crypt_stat->num_header_extents_at_front)
 			/ PAGE_CACHE_SIZE);
-	ASSERT(header_pages >= 1);
+	BUG_ON(header_pages < 1);
 	while (current_header_page < header_pages) {
 		rc = ecryptfs_grab_and_map_lower_page(&header_page,
 						      &header_virt,
@@ -646,8 +646,8 @@ static int ecryptfs_commit_write(struct 
 	mutex_lock(&lower_inode->i_mutex);
 	crypt_stat =
 		&ecryptfs_inode_to_private(file->f_dentry->d_inode)->crypt_stat;
-	ASSERT(crypt_stat);
-	ASSERT(lower_file);
+	BUG_ON(!crypt_stat);
+	BUG_ON(!lower_file);
 	if (ECRYPTFS_CHECK_FLAG(crypt_stat->flags, ECRYPTFS_NEW_FILE)) {
 		ecryptfs_printk(KERN_DEBUG, "ECRYPTFS_NEW_FILE flag set in "
 			"crypt_stat at memory location [%p]\n", crypt_stat);
-- 
1.3.3

Re: [PATCH 1/10] Convert ASSERT to BUG_ON

[Adrian Bunk]

On Fri, May 26, 2006 at 09:21:48AM -0500, Mike Halcrow wrote:

> Use the kernel BUG_ON() macro rather than the eCryptfs ASSERT(). Note
> that this temporarily renders the CONFIG_ECRYPT_DEBUG build option
> unused. We certainly plan on using it in the future; for now, is it
> okay to leave it in fs/Kconfig, or would you like to remove it?
>...

Remove it as long as it deos nothing - re-adding it when it's actually 
used will be trivial.

> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -506,7 +506,7 @@ static int decrypt_session_key(struct ec
>  	       auth_tok->session_key.encrypted_key_size);
>  	src_sg[0].page = virt_to_page(encrypted_session_key);
>  	src_sg[0].offset = 0;
> -	ASSERT(auth_tok->session_key.encrypted_key_size < PAGE_CACHE_SIZE);
> +	BUG_ON(auth_tok->session_key.encrypted_key_size > PAGE_CACHE_SIZE);
>...

What's with (auth_tok->session_key.encrypted_key_size == PAGE_CACHE_SIZE) ?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1/10] Convert ASSERT to BUG_ON

[Michael Halcrow]

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

On Fri, May 26, 2006 at 05:24:01PM +0200, Adrian Bunk wrote:
> On Fri, May 26, 2006 at 09:21:48AM -0500, Mike Halcrow wrote:
> > -	ASSERT(auth_tok->session_key.encrypted_key_size < PAGE_CACHE_SIZE);
> > +	BUG_ON(auth_tok->session_key.encrypted_key_size > PAGE_CACHE_SIZE);
> >...
> 
> What's with (auth_tok->session_key.encrypted_key_size ==
> PAGE_CACHE_SIZE) ?

That should not be a problem.

Mike

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

Re: [PATCH 1/10] Convert ASSERT to BUG_ON

[Adrian Bunk]

On Fri, May 26, 2006 at 11:14:54AM -0500, Michael Halcrow wrote:
> On Fri, May 26, 2006 at 05:24:01PM +0200, Adrian Bunk wrote:
> > On Fri, May 26, 2006 at 09:21:48AM -0500, Mike Halcrow wrote:
> > > -	ASSERT(auth_tok->session_key.encrypted_key_size < PAGE_CACHE_SIZE);
> > > +	BUG_ON(auth_tok->session_key.encrypted_key_size > PAGE_CACHE_SIZE);
> > >...
> > 
> > What's with (auth_tok->session_key.encrypted_key_size ==
> > PAGE_CACHE_SIZE) ?
> 
> That should not be a problem.

IOW, the ASSERT was wrong?

My point is simply that this is not a semantically equivalent 
transformation, so if this fixes a case where the ASSERT() might have 
mistakenly triggered it should be noted in the changelog.

> Mike

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

[PATCH] Remove ECRYPT_DEBUG from fs/Kconfig

[Michael Halcrow]

On Fri, May 26, 2006 at 05:24:01PM +0200, Adrian Bunk wrote:
> On Fri, May 26, 2006 at 09:21:48AM -0500, Mike Halcrow wrote:
> > Use the kernel BUG_ON() macro rather than the eCryptfs
> > ASSERT(). Note that this temporarily renders the
> > CONFIG_ECRYPT_DEBUG build option unused. We certainly plan on
> > using it in the future; for now, is it okay to leave it in
> > fs/Kconfig, or would you like to remove it?
> 
> Remove it as long as it deos nothing - re-adding it when it's
> actually used will be trivial.

This patch removes ECRYPT_DEBUG from fs/Kconfig; apply after the patch
1 from this 10-part patch set.

Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>

---

 fs/Kconfig |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

15ef83ff75ab0b601a2648175c7f60af16ddb31c
diff --git a/fs/Kconfig b/fs/Kconfig
index 67d5568..b68e46e 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -955,14 +955,6 @@ config ECRYPT_FS
 	  To compile this file system support as a module, choose M here: the
 	  module will be called ecryptfs.
 
-config ECRYPT_DEBUG
-	bool "Enable eCryptfs debug mode"
-	depends on ECRYPT_FS
-	help
-	  Turn on debugging code in eCryptfs.
-
-	  If unsure, say N.
-
 config HFS_FS
 	tristate "Apple Macintosh file system support (EXPERIMENTAL)"
 	depends on EXPERIMENTAL