lkml.org 
[lkml]   [2006]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 8/13: eCryptfs] File operations
From
Date
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>

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-05-05 20:57    [from the cache]
©2003-2011 Jasper Spaans