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    [W:0.056 / U:0.988 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site