Re: [PATCH 8/13: eCryptfs] File operations

["Timothy R. Chavez" <tinytim@us.ibm.com>]

On Wed, 2006-05-03 at 21:39 -0600, Phillip Hellewell wrote: 
> This is the 8th patch in a series of 13 constituting the kernel
> components of the eCryptfs cryptographic filesystem.
> 
> eCryptfs file operations. Includes code to read header information
> from the underyling file when needed.
> 
> Signed-off-by: Phillip Hellewell <phillip@hellewell.homeip.net>
> Signed-off-by: Michael Halcrow <mhalcrow@us.ibm.com>
> 

Hello,

Just a few more little comments.

-tim

> ---
> 
>  file.c |  642 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 642 insertions(+)
> 
> Index: linux-2.6.17-rc3-mm1-ecryptfs/fs/ecryptfs/file.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.17-rc3-mm1-ecryptfs/fs/ecryptfs/file.c	2006-05-02 19:36:01.000000000 -0600
> @@ -0,0 +1,642 @@
> +/**
> + * eCryptfs: Linux filesystem encryption layer
> + *
> + * Copyright (C) 1997-2004 Erez Zadok
> + * Copyright (C) 2001-2004 Stony Brook University
> + * Copyright (C) 2004-2006 International Business Machines Corp.
> + *   Author(s): Michael A. Halcrow <mhalcrow@us.ibm.com>
> + *   		Michael C. Thompson <mcthomps@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; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
> + * 02111-1307, USA.
> + */
> +
> +#include <linux/file.h>
> +#include <linux/poll.h>
> +#include <linux/mount.h>
> +#include <linux/pagemap.h>
> +#include <linux/security.h>
> +#include <linux/smp_lock.h>
> +#include <linux/compat.h>
> +#include "ecryptfs_kernel.h"
> +
> +/**
> + * @param file File we are seeking in
> + * @param offset The offset to seek to
> + * @param origin 2: offset from i_size; 1: offset from f_pos
> + * @return The position we have seeked to, or negative on error
> + */
> +static loff_t ecryptfs_llseek(struct file *file, loff_t offset, int origin)
> +{
> +	loff_t rv;
> +	loff_t new_end_pos;
> +	int rc;
> +	int expanding_file = 0;
> +	struct inode *inode = file->f_mapping->host;
> +
> +	ecryptfs_printk(KERN_DEBUG, "Enter; offset = [0x%.16x]\n", offset);
> +	ecryptfs_printk(KERN_DEBUG, "origin = [%d]\n", origin);
> +	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));
> +	/* If our offset is past the end of our file, we're going to
> +	 * need to grow it so we have a valid length of 0's */
> +	new_end_pos = offset;
> +	switch (origin) {
> +	case 2:
> +		new_end_pos += i_size_read(inode);
> +		expanding_file = 1;
> +		break;
> +	case 1:
> +		new_end_pos += file->f_pos;
> +		if (new_end_pos > i_size_read(inode)) {
> +			ecryptfs_printk(KERN_DEBUG, "new_end_pos(=[0x%.16x]) "
> +					"> i_size_read(inode)(=[0x%.16x])\n",
> +					new_end_pos, i_size_read(inode));
> +			expanding_file = 1;
> +		}
> +		break;
> +	default:
> +		if (new_end_pos > i_size_read(inode)) {
> +			ecryptfs_printk(KERN_DEBUG, "new_end_pos(=[0x%.16x]) "
> +					"> i_size_read(inode)(=[0x%.16x])\n",
> +					new_end_pos, i_size_read(inode));
> +			expanding_file = 1;
> +		}
> +	}
> +	ecryptfs_printk(KERN_DEBUG, "new_end_pos = [0x%.16x]\n", new_end_pos);
> +	if (expanding_file) {
> +		rc = ecryptfs_truncate(file->f_dentry, new_end_pos);
> +		if (rc) {
> +			rv = rc;
> +			ecryptfs_printk(KERN_ERR, "Error on attempt to "
> +					"truncate to (higher) offset [0x%.16x];"
> +					" rc = [%d]\n", rc, new_end_pos);
> +			goto out;
> +		}
> +	}
> +	rv = generic_file_llseek(file, offset, origin);
> +out:
> +	ecryptfs_printk(KERN_DEBUG, "Exit; rv = [0x%.16x]\n", rv);
> +	return rv;
> +}

Maybe get rid of 'rc' and rename 'rv' to 'rc'?  Makes things look a bit
more consistent... 

[..] 
> +
> +/**
> + * generic_file_read updates the atime of upper layer inode.  But, it
> + * doesn't give us a chance to update the atime of the lower layer
> + * inode.  This function is a wrapper to generic_file_read.  It
> + * updates the atime of the lower level inode if generic_file_read
> + * returns without any errors. This is to be used only for file reads.
> + * The function to be used for directory reads is ecryptfs_read.
> + */
> +static ssize_t ecryptfs_read_update_atime(struct file *file, char __user * buf,
> +					  size_t count, loff_t * ppos)
> +{
> +	int rc = 0;

Initialization not needed.

[..]
> +	struct dentry *lower_dentry;
> +	struct vfsmount *lower_vfsmount;
> +
> +	ecryptfs_printk(KERN_DEBUG, "Enter\n");
> +	rc = generic_file_read(file, buf, count, ppos);
> +	if (rc >= 0) {
> +		lower_dentry = ECRYPTFS_DENTRY_TO_LOWER(file->f_dentry);
> +		lower_vfsmount = ECRYPTFS_SUPERBLOCK_TO_PRIVATE(
> +			file->f_dentry->d_inode->i_sb)->lower_mnt;
> +		touch_atime(lower_vfsmount, lower_dentry);
> +	}
> +	ecryptfs_printk(KERN_DEBUG, "Exit\n");
> +	return rc;
> +}
> +
> +struct ecryptfs_getdents_callback {
> +	void *dirent;
> +	struct dentry *dentry;
> +	filldir_t filldir;
> +	int err;
> +	int filldir_called;
> +	int entries_written;
> +};
> +
> +/* Inspired by generic filldir in fs/readir.c */
> +static int
> +ecryptfs_filldir(void *dirent, const char *name, int namelen, loff_t offset,
> +		 ino_t ino, unsigned int d_type)
> +{
> +	struct ecryptfs_crypt_stat *crypt_stat;
> +	struct ecryptfs_getdents_callback *buf =
> +	    (struct ecryptfs_getdents_callback *)dirent;
> +	int rc;
> +	char *decoded_name;
> +	int decoded_length;
> +
> +	ecryptfs_printk(KERN_DEBUG, "Enter w/ name = [%.*s]\n", namelen,
> +			name);
> +	crypt_stat = ECRYPTFS_DENTRY_TO_PRIVATE(buf->dentry)->crypt_stat;
> +	buf->filldir_called++;
> +	decoded_length = ecryptfs_decode_filename(crypt_stat, name, namelen,
> +						  &decoded_name);
> +	if (decoded_length < 0)
> +		return 0;

Is it wise to potentially toss away the -ENOMEM being returned from
ecryptfs_decode_filename... Might consider propagating that error??

[..]
> 
> +	rc = buf->filldir(buf->dirent, decoded_name, decoded_length, offset,
> +			  ino, d_type);
> +	kfree(decoded_name);
> +	if (rc >= 0)
> +		buf->entries_written++;
> +	ecryptfs_printk(KERN_DEBUG, "Exit; rc = [%d]\n", rc);
> +	return rc;
> +}
> +
> +/**
> + * @param file The ecryptfs file struct
> + * @param filldir The filldir callback function
> + */
> +static int ecryptfs_readdir(struct file *file, void *dirent, filldir_t filldir)
> +{
> +	int rc = -ENOTDIR;
> +	struct file *lower_file = NULL;

Neither initialization not needed.

[..]
> +	struct inode *inode;
> +	struct ecryptfs_getdents_callback buf;
> +
> +	ecryptfs_printk(KERN_DEBUG, "Enter; file = [%p]\n", file);
> +	lower_file = ECRYPTFS_FILE_TO_LOWER(file);
> +	inode = file->f_dentry->d_inode;
> +	memset(&buf, 0, sizeof(buf));
> +	buf.dirent = dirent;
> +	buf.dentry = file->f_dentry;
> +	buf.filldir = filldir;
> +retry:
> +	buf.filldir_called = 0;
> +	buf.entries_written = 0;
> +	buf.err = 0;
> +	rc = vfs_readdir(lower_file, ecryptfs_filldir, (void *)&buf);
> +	if (buf.err)
> +		rc = buf.err;
> +	if (buf.filldir_called && !buf.entries_written)
> +		goto retry;
> +	file->f_pos = lower_file->f_pos;
> +	if (rc >= 0)
> +		ecryptfs_copy_attr_atime(inode, lower_file->f_dentry->d_inode);
> +	ecryptfs_printk(KERN_DEBUG, "Exit; rc = [%d]\n", rc);
> +	return rc;
> +}
> +
> +/**
> + * @return 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 = 0;
> +	struct page *header_page;
> +	unsigned char *header_virt;
> +	u64 data_size;
> +
> +	ecryptfs_printk(KERN_DEBUG, "Enter w/ lower_inode = [%p]; inode = "
> +			"[%p]\n", lower_inode, inode);
> +	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:
> +	ecryptfs_printk(KERN_DEBUG, "Exit; rc = [%d]\n", rc);
> +	return rc;
> +}
> +
> +kmem_cache_t *ecryptfs_file_info_cache;

Static?

[..]
> +
> +/**
> + * Opens the file specified by inode.
> + *
> + * @param inode	inode speciying file to open
> + * @param file	Structure to return filled in
> + * @return Zero on success; non-zero otherwise
> + */
> +static int ecryptfs_open(struct inode *inode, struct file *file)
> +{
> +	int rc = 0;
> +	struct ecryptfs_crypt_stat *crypt_stat = NULL;
> +	struct dentry *ecryptfs_dentry = file->f_dentry;
> +	struct dentry *lower_dentry = ECRYPTFS_DENTRY_TO_LOWER(ecryptfs_dentry);
> +	struct inode *lower_inode = NULL;
> +	struct file *lower_file = NULL;
> +	struct vfsmount *lower_mnt;
> +	int lower_flags;
> +
> +	ecryptfs_printk(KERN_DEBUG, "Enter\n");
> +	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));
> +	ecryptfs_printk(KERN_DEBUG, "file->f_dentry = [%p], "
> +			"file->f_dentry->d_name.name = [%s], "
> +			"file->f_dentry->d_name.len = [%d]\n",
> +			ecryptfs_dentry, ecryptfs_dentry->d_name.name,
> +			ecryptfs_dentry->d_name.len);
> +	/* ECRYPTFS_DENTRY_TO_PRIVATE(ecryptfs_dentry) Allocated in
> +	 * ecryptfs_lookup() */
> +	/* Released in ecryptfs_release or end of function if failure */
> +	ECRYPTFS_FILE_TO_PRIVATE_SM(file) =
> +		kmem_cache_alloc(ecryptfs_file_info_cache, SLAB_KERNEL);
> +	if (!ECRYPTFS_FILE_TO_PRIVATE_SM(file)) {
> +		ecryptfs_printk(KERN_ERR,
> +				"Error attempting to allocate memory\n");
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +	lower_dentry = ECRYPTFS_DENTRY_TO_LOWER(ecryptfs_dentry);
> +	crypt_stat = &(ECRYPTFS_INODE_TO_PRIVATE(inode)->crypt_stat);
> +	if (!ECRYPTFS_CHECK_FLAG(crypt_stat->flags, ECRYPTFS_POLICY_APPLIED)) {
> +		ecryptfs_printk(KERN_DEBUG, "Setting flags for stat...\n");
> +		/* Policy code enabled in future release */
> +		ECRYPTFS_SET_FLAG(crypt_stat->flags, ECRYPTFS_POLICY_APPLIED);
> +		ECRYPTFS_SET_FLAG(crypt_stat->flags, ECRYPTFS_ENCRYPTED);
> +	}
> +	/* This mntget & dget is undone via fput when the file is released */
> +	dget(lower_dentry);
> +	lower_flags = file->f_flags;
> +	if ((lower_flags & O_ACCMODE) == O_WRONLY)
> +		lower_flags = (lower_flags & O_ACCMODE) | O_RDWR;
> +	if (file->f_flags & O_APPEND)
> +		lower_flags &= ~O_APPEND;
> +	lower_mnt = ECRYPTFS_SUPERBLOCK_TO_PRIVATE(inode->i_sb)->lower_mnt;
> +	mntget(lower_mnt);
> +	/* Corresponding fput() in ecryptfs_release() */
> +	lower_file = dentry_open(lower_dentry, lower_mnt, lower_flags);
> +	if (IS_ERR(lower_file)) {
> +		rc = PTR_ERR(lower_file);
> +		ecryptfs_printk(KERN_ERR, "Error opening lower file\n");
> +		goto out_puts;
> +	}
> +	ECRYPTFS_FILE_TO_LOWER(file) = lower_file;
> +	/* Isn't this check the same as the one in lookup? */
> +	lower_inode = lower_dentry->d_inode;
> +	if (S_ISDIR(ecryptfs_dentry->d_inode->i_mode)) {
> +		ecryptfs_printk(KERN_DEBUG, "This is a directory\n");
> +		ECRYPTFS_CLEAR_FLAG(crypt_stat->flags, ECRYPTFS_ENCRYPTED);
> +		rc = 0;
> +		goto out;
> +	}
> +	if (i_size_read(lower_inode) == 0) {
> +		ecryptfs_printk(KERN_EMERG, "Zero-length lower file; "
> +				"ecryptfs_create() had a problem?\n");
> +		rc = -ENOENT;
> +		goto out_puts;
> +	} else if (!ECRYPTFS_CHECK_FLAG(crypt_stat->flags,
> +					ECRYPTFS_POLICY_APPLIED)
> +		   || !ECRYPTFS_CHECK_FLAG(crypt_stat->flags,
> +					   ECRYPTFS_KEY_VALID)) {
> +		rc = ecryptfs_read_headers(ecryptfs_dentry, lower_file);
> +		if (rc) {
> +			ecryptfs_printk(KERN_DEBUG,
> +					"Valid headers not found\n");
> +			ECRYPTFS_CLEAR_FLAG(crypt_stat->flags,
> +					    ECRYPTFS_ENCRYPTED);
> +			/* At this point, we could just move on and
> +			 * have the encrypted data passed through
> +			 * as-is to userspace. For release 0.1, we are
> +			 * 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,
> +			i_size_read(inode));
> +	ECRYPTFS_FILE_TO_LOWER(file) = lower_file;
> +	goto out;
> +out_puts:
> +	mntput(lower_mnt);
> +	dput(lower_dentry);
> +	kmem_cache_free(ecryptfs_file_info_cache,
> +			ECRYPTFS_FILE_TO_PRIVATE(file));
> +out:
> +	ecryptfs_printk(KERN_DEBUG, "Exit; rc = [%d]\n", rc);
> +	return rc;
> +}
> +
> +static int ecryptfs_flush(struct file *file, fl_owner_t td)
> +{
> +	int rc = 0;
> +	struct file *lower_file = NULL;
> +
> +	ecryptfs_printk(KERN_DEBUG, "Enter; file = [%p]\n", file);
> +	lower_file = ECRYPTFS_FILE_TO_LOWER(file);
> +	if (lower_file->f_op && lower_file->f_op->flush)
> +		rc = lower_file->f_op->flush(lower_file, td);
> +	ecryptfs_printk(KERN_DEBUG, "Exit; rc = [%d]\n", rc);
> +	return rc;
> +}
> +
> +static int ecryptfs_release(struct inode *ecryptfs_inode, struct file *file)
> +{
> +	int rc = 0;
> +	struct file *lower_file = NULL;
> +	struct inode *lower_inode;
> +
> +	ecryptfs_printk(KERN_DEBUG,
> +			"Enter; ecryptfs_inode->i_count = [%d]\n",
> +			ecryptfs_inode->i_count);
> +	lower_file = ECRYPTFS_FILE_TO_LOWER(file);
> +	kmem_cache_free(ecryptfs_file_info_cache,
> +			ECRYPTFS_FILE_TO_PRIVATE(file));
> +	lower_inode = ECRYPTFS_INODE_TO_LOWER(ecryptfs_inode);
> +	fput(lower_file);
> +	ecryptfs_inode->i_blocks = lower_inode->i_blocks;
> +	ecryptfs_printk(KERN_DEBUG, "Exit; rc = [%d]\n", rc);
> +	return rc;

What about just "return 0;" here.  There's no need for 'rc'.

> +}
> +

<snip>