Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751215AbWEESzG (ORCPT ); Fri, 5 May 2006 14:55:06 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751507AbWEESzG (ORCPT ); Fri, 5 May 2006 14:55:06 -0400 Received: from e31.co.us.ibm.com ([32.97.110.149]:47066 "EHLO e31.co.us.ibm.com") by vger.kernel.org with ESMTP id S1751215AbWEESzD (ORCPT ); Fri, 5 May 2006 14:55:03 -0400 Subject: Re: [PATCH 8/13: eCryptfs] File operations From: "Timothy R. Chavez" To: Phillip Hellewell Cc: Andrew Morton , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, viro@ftp.linux.org.uk, mike@halcrow.us, mhalcrow@us.ibm.com, mcthomps@us.ibm.com, toml@us.ibm.com, yoder1@us.ibm.com, James Morris , "Stephen C. Tweedie" , Erez Zadok , David Howells In-Reply-To: <20060504033949.GG28613@hellewell.homeip.net> References: <20060504031755.GA28257@hellewell.homeip.net> <20060504033949.GG28613@hellewell.homeip.net> Content-Type: text/plain Date: Fri, 05 May 2006 13:55:03 -0500 Message-Id: <1146855304.17346.19.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.6.1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14878 Lines: 441 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 > Signed-off-by: Michael Halcrow > 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 > + * Michael C. Thompson > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#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'. > +} > + - 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/