Re: [Ecryptfs-devel] [PATCH 3/11] eCryptfs: read_write.c routines
From: Michael Halcrow
Date: Fri Sep 21 2007 - 17:51:51 EST
On Wed, Sep 19, 2007 at 10:38:50PM -0700, Andrew Morton wrote:
> > + virt = kmap(page_for_lower);
> > + rc = ecryptfs_write_lower(ecryptfs_inode, virt, offset, size);
> > + kunmap(page_for_lower);
> > + return rc;
> > +}
>
> argh, kmap. http://lkml.org/lkml/2007/9/15/55
Here is a patch that moves to kmap_atomic(), adding an intermediate
copy. Although I would really like to find a way to avoid having to do
this extra copy.
---
Replace kmap() with kmap_atomic() for read_write.c routines
kmap() can lead to deadlock when multiple tasks attempt to take more
than one simultaneously:
http://lkml.org/lkml/2007/9/15/55
In order to avoid this possibility, eCryptfs must allocate an
intermediate block of memory to use with vfs_read() and vfs_write(),
copying the data through this memory region, since kmap_atomic()
cannot be held during calls which may block.
Signed-off-by: Michael Halcrow <mhalcrow@xxxxxxxxxx>
---
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index bb92b74..ce7a5d4 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -648,6 +648,6 @@ int ecryptfs_read_lower_page_segment(struct page *page_for_ecryptfs,
struct inode *ecryptfs_inode);
int ecryptfs_read(char *data, loff_t offset, size_t size,
struct file *ecryptfs_file);
-struct page *ecryptfs_get1page(struct file *file, loff_t index);
+struct page *ecryptfs_get_locked_page(struct file *file, loff_t index);
#endif /* #ifndef ECRYPTFS_KERNEL_H */
diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c
index c6a8a33..6abf805 100644
--- a/fs/ecryptfs/mmap.c
+++ b/fs/ecryptfs/mmap.c
@@ -37,23 +37,30 @@
struct kmem_cache *ecryptfs_lower_page_cache;
/**
- * ecryptfs_get1page
+ * ecryptfs_get_locked_page
*
* Get one page from cache or lower f/s, return error otherwise.
*
- * Returns unlocked and up-to-date page (if ok), with increased
+ * Returns a locked and up-to-date page (if ok), with increased
* refcnt.
*/
-struct page *ecryptfs_get1page(struct file *file, loff_t index)
+struct page *ecryptfs_get_locked_page(struct file *file, loff_t index)
{
struct dentry *dentry;
struct inode *inode;
struct address_space *mapping;
+ struct page *page;
dentry = file->f_path.dentry;
inode = dentry->d_inode;
mapping = inode->i_mapping;
- return read_mapping_page(mapping, index, (void *)file);
+ page = read_mapping_page(mapping, index, (void *)file);
+ if (!IS_ERR(page))
+ lock_page(page);
+ else
+ printk(KERN_ERR "%s: Error from read_mapping_page()\n",
+ __FUNCTION__);
+ return page;
}
/**
diff --git a/fs/ecryptfs/read_write.c b/fs/ecryptfs/read_write.c
index ccd2599..6732a4c 100644
--- a/fs/ecryptfs/read_write.c
+++ b/fs/ecryptfs/read_write.c
@@ -83,14 +83,24 @@ int ecryptfs_write_lower_page_segment(struct inode *ecryptfs_inode,
struct page *page_for_lower,
size_t offset_in_page, size_t size)
{
- char *virt;
+ char *page_for_lower_virt;
+ char *tmp_virt;
loff_t offset;
int rc;
- offset = (page_for_lower->index << PAGE_CACHE_SHIFT) + offset_in_page;
- virt = kmap(page_for_lower);
- rc = ecryptfs_write_lower(ecryptfs_inode, virt, offset, size);
- kunmap(page_for_lower);
+ tmp_virt = kmalloc(PAGE_CACHE_SIZE, GFP_KERNEL);
+ if (!tmp_virt) {
+ rc = -ENOMEM;
+ goto out;
+ }
+ offset = ((((loff_t)page_for_lower->index) << PAGE_CACHE_SHIFT)
+ + offset_in_page);
+ page_for_lower_virt = kmap_atomic(page_for_lower, KM_USER0);
+ memcpy(tmp_virt, page_for_lower_virt, PAGE_CACHE_SIZE);
+ kunmap_atomic(page_for_lower_virt, KM_USER0);
+ rc = ecryptfs_write_lower(ecryptfs_inode, tmp_virt, offset, size);
+ kfree(tmp_virt);
+out:
return rc;
}
@@ -140,8 +150,8 @@ int ecryptfs_write(struct file *ecryptfs_file, char *data, loff_t offset,
if (num_bytes > total_remaining_zeros)
num_bytes = total_remaining_zeros;
}
- ecryptfs_page = ecryptfs_get1page(ecryptfs_file,
- ecryptfs_page_idx);
+ ecryptfs_page = ecryptfs_get_locked_page(ecryptfs_file,
+ ecryptfs_page_idx);
if (IS_ERR(ecryptfs_page)) {
rc = PTR_ERR(ecryptfs_page);
printk(KERN_ERR "%s: Error getting page at "
@@ -159,6 +169,7 @@ int ecryptfs_write(struct file *ecryptfs_file, char *data, loff_t offset,
printk(KERN_ERR "%s: Error decrypting "
"page; rc = [%d]\n",
__FUNCTION__, rc);
+ unlock_page(ecryptfs_page);
page_cache_release(ecryptfs_page);
goto out;
}
@@ -182,9 +193,11 @@ int ecryptfs_write(struct file *ecryptfs_file, char *data, loff_t offset,
if (rc) {
printk(KERN_ERR "%s: Error encrypting "
"page; rc = [%d]\n", __FUNCTION__, rc);
+ unlock_page(ecryptfs_page);
page_cache_release(ecryptfs_page);
goto out;
}
+ unlock_page(ecryptfs_page);
page_cache_release(ecryptfs_page);
pos += num_bytes;
}
@@ -245,10 +258,6 @@ int ecryptfs_read_lower(char *data, loff_t offset, size_t size,
data_page = virt_to_page(data + i);
flush_dcache_page(data_page);
- if (rc)
- ClearPageUptodate(data_page);
- else
- SetPageUptodate(data_page);
}
return rc;
}
@@ -273,14 +282,34 @@ int ecryptfs_read_lower_page_segment(struct page *page_for_ecryptfs,
size_t offset_in_page, size_t size,
struct inode *ecryptfs_inode)
{
- char *virt;
+ char *page_for_ecryptfs_virt;
+ char *tmp_virt;
loff_t offset;
int rc;
- offset = ((page_index << PAGE_CACHE_SHIFT) + offset_in_page);
- virt = kmap(page_for_ecryptfs);
- rc = ecryptfs_read_lower(virt, offset, size, ecryptfs_inode);
- kunmap(page_for_ecryptfs);
+ tmp_virt = kmalloc(PAGE_CACHE_SIZE, GFP_KERNEL);
+ if (!tmp_virt) {
+ rc = -ENOMEM;
+ goto out;
+ }
+ offset = ((((loff_t)page_index) << PAGE_CACHE_SHIFT) + offset_in_page);
+ rc = ecryptfs_read_lower(tmp_virt, offset, size, ecryptfs_inode);
+ if (rc) {
+ printk(KERN_ERR "%s: Error reading lower; rc = [%d]\n",
+ __FUNCTION__, rc);
+ goto out_free;
+ }
+ page_for_ecryptfs_virt = kmap_atomic(page_for_ecryptfs, KM_USER0);
+ memcpy(page_for_ecryptfs_virt, tmp_virt, PAGE_CACHE_SIZE);
+ kunmap_atomic(page_for_ecryptfs_virt, KM_USER0);
+ flush_dcache_page(page_for_ecryptfs);
+out_free:
+ kfree(tmp_virt);
+out:
+ if (rc)
+ ClearPageUptodate(page_for_ecryptfs);
+ else
+ SetPageUptodate(page_for_ecryptfs);
return rc;
}
@@ -328,8 +357,8 @@ int ecryptfs_read(char *data, loff_t offset, size_t size,
if (num_bytes > total_remaining_bytes)
num_bytes = total_remaining_bytes;
- ecryptfs_page = ecryptfs_get1page(ecryptfs_file,
- ecryptfs_page_idx);
+ ecryptfs_page = ecryptfs_get_locked_page(ecryptfs_file,
+ ecryptfs_page_idx);
if (IS_ERR(ecryptfs_page)) {
rc = PTR_ERR(ecryptfs_page);
printk(KERN_ERR "%s: Error getting page at "
@@ -342,6 +371,7 @@ int ecryptfs_read(char *data, loff_t offset, size_t size,
if (rc) {
printk(KERN_ERR "%s: Error decrypting "
"page; rc = [%d]\n", __FUNCTION__, rc);
+ unlock_page(ecryptfs_page);
page_cache_release(ecryptfs_page);
goto out;
}
@@ -350,6 +380,7 @@ int ecryptfs_read(char *data, loff_t offset, size_t size,
((char *)ecryptfs_page_virt + start_offset_in_page),
num_bytes);
kunmap_atomic(ecryptfs_page_virt, KM_USER0);
+ unlock_page(ecryptfs_page);
page_cache_release(ecryptfs_page);
pos += num_bytes;
data_offset += num_bytes;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/