Hi Linus,
On Mon, 9 Jul 2001, Linus Torvalds wrote:
>
> On 9 Jul 2001, Christoph Rohland wrote:
>>
>> No, it does matter. It prevents races against getpage.
>
> No it doesn't.
>
> We have the page locked.
>
> And if somebody does "getpage()" and doesn't check for the page
> lock, then that test _still_ doesn't prevent races, because the
> getpage might happen just _after_ the "atomic_read()".
>
> As it stands now, that atomic_read() does _nothing_. If you think
> something depends on it, then that something is already buggy.
Yep, you are right. This check hides another error: We cannot use
find_get_page for shmem since this is getting the page without the
lock like you described. I removed this optimization. Also
__find_lock_page has to check that mapping and index are still the
ones we looked for.
I append a patch to fix these errors (and the other obvious buglets in
shmem.c I did send to you several times).
Stephen, could you crosscheck? You had the test case which triggered
the count > 2 bug.
Greetings
Christoph
diff -uNr 7-pre6/mm/filemap.c 7-pre6-fix/mm/filemap.c
--- 7-pre6/mm/filemap.c Wed Jul 11 09:59:01 2001
+++ 7-pre6-fix/mm/filemap.c Wed Jul 11 20:49:14 2001
@@ -760,7 +760,7 @@
lock_page(page);
/* Is the page still hashed? Ok, good.. */
- if (page->mapping)
+ if (page->mapping == mapping && page->index == offset)
return page;
/* Nope: we raced. Release and try again.. */
diff -uNr 7-pre6/mm/shmem.c 7-pre6-fix/mm/shmem.c
--- 7-pre6/mm/shmem.c Wed Jul 11 09:59:01 2001
+++ 7-pre6-fix/mm/shmem.c Wed Jul 11 20:44:35 2001
@@ -3,7 +3,8 @@
*
* Copyright (C) 2000 Linus Torvalds.
* 2000 Transmeta Corp.
- * 2000 Christoph Rohland
+ * 2000-2001 Christoph Rohland
+ * 2000-2001 SAP AG
*
* This file is released under the GPL.
*/
@@ -33,7 +34,7 @@
#define TMPFS_MAGIC 0x01021994
#define ENTRIES_PER_PAGE (PAGE_SIZE/sizeof(unsigned long))
-#define NR_SINGLE (ENTRIES_PER_PAGE + SHMEM_NR_DIRECT)
+#define SHMEM_MAX_BLOCKS (SHMEM_NR_DIRECT + ENTRIES_PER_PAGE*ENTRIES_PER_PAGE)
static struct super_operations shmem_ops;
static struct address_space_operations shmem_aops;
@@ -193,7 +194,14 @@
}
out:
- info->max_index = index;
+ /*
+ * We have no chance to give an error, so we limit it to max
+ * size here and the application will fail later
+ */
+ if (index > SHMEM_MAX_BLOCKS)
+ info->max_index = SHMEM_MAX_BLOCKS;
+ else
+ info->max_index = index;
info->swapped -= freed;
shmem_recalc_inode(inode);
spin_unlock (&info->lock);
@@ -311,6 +319,7 @@
return page;
}
+ shmem_recalc_inode(inode);
if (entry->val) {
unsigned long flags;
@@ -390,22 +399,9 @@
static int shmem_getpage(struct inode * inode, unsigned long idx, struct page **ptr)
{
- struct address_space * mapping = inode->i_mapping;
int error;
- *ptr = NOPAGE_SIGBUS;
- if (inode->i_size <= (loff_t) idx * PAGE_CACHE_SIZE)
- return -EFAULT;
-
- *ptr = __find_get_page(mapping, idx, page_hash(mapping, idx));
- if (*ptr) {
- if (Page_Uptodate(*ptr))
- return 0;
- page_cache_release(*ptr);
- }
-
down (&inode->i_sem);
- /* retest we may have slept */
if (inode->i_size < (loff_t) idx * PAGE_CACHE_SIZE)
goto sigbus;
*ptr = shmem_getpage_locked(inode, idx);
@@ -1024,6 +1020,8 @@
unsigned long max_inodes, inodes;
struct shmem_sb_info *info = &sb->u.shmem_sb;
+ max_blocks = info->max_blocks;
+ max_inodes = info->max_inodes;
if (shmem_parse_options (data, NULL, &max_blocks, &max_inodes))
return -EINVAL;
@@ -1071,7 +1069,7 @@
sb->u.shmem_sb.free_blocks = blocks;
sb->u.shmem_sb.max_inodes = inodes;
sb->u.shmem_sb.free_inodes = inodes;
- sb->s_maxbytes = (unsigned long long)(SHMEM_NR_DIRECT + (ENTRIES_PER_PAGE*ENTRIES_PER_PAGE)) << PAGE_CACHE_SHIFT;
+ sb->s_maxbytes = (unsigned long long)SHMEM_MAX_BLOCKS << PAGE_CACHE_SHIFT;
sb->s_blocksize = PAGE_CACHE_SIZE;
sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
sb->s_magic = TMPFS_MAGIC;
@@ -1279,9 +1277,11 @@
struct qstr this;
int vm_enough_memory(long pages);
- error = -ENOMEM;
+ if (size > (unsigned long long) SHMEM_MAX_BLOCKS << PAGE_CACHE_SHIFT)
+ return ERR_PTR(-EINVAL);
+
if (!vm_enough_memory((size) >> PAGE_SHIFT))
- goto out;
+ return ERR_PTR(-ENOMEM);
this.name = name;
this.len = strlen(name);
@@ -1289,7 +1289,7 @@
root = tmpfs_fs_type.kern_mnt->mnt_root;
dentry = d_alloc(root, &this);
if (!dentry)
- goto out;
+ return ERR_PTR(-ENOMEM);
error = -ENFILE;
file = get_empty_filp();
@@ -1315,7 +1315,6 @@
put_filp(file);
put_dentry:
dput (dentry);
-out:
return ERR_PTR(error);
}
/*
-
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/
This archive was generated by hypermail 2b29 : Sun Jul 15 2001 - 21:00:15 EST