index 7c3cd24..09ef5d6 100644
@@ -944,14 +944,17 @@ static ssize_t btrfs_file_write(struct file *file, const char __user *buf,
 	if (count == 0)
 		goto out_nolock;
 
+	pages = kmalloc(nrptrs * sizeof(struct page *), GFP_KERNEL);
+	if (!pages)
+		goto out_nolock;
+
+	mutex_lock(&inode->i_mutex);
+
 	err = file_remove_suid(file);
 	if (err)
-		goto out_nolock;
+		goto out;
 	file_update_time(file);
 
-	pages = kmalloc(nrptrs * sizeof(struct page *), GFP_KERNEL);
-
-	mutex_lock(&inode->i_mutex);
 	BTRFS_I(inode)->sequence++;
 	first_index = pos >> PAGE_CACHE_SHIFT;
 	last_index = (pos + count) >> PAGE_CACHE_SHIFT;

Tao Ma

2009-07-06 07:42 UTC #1 lore raw

Hi Amerigo,

Quoted 22 lines · Amerigo Wang

V1 -> V2:
Move kmalloc() before mutex_lock(), suggested by Arjan.
file_remove_suid() should be called with i_mutex held, file_update_time() too. So move them after mutex_lock().
Plus, check the return value of kmalloc().
Signed-off-by WANG Cong <[email protected]>
Cc Arjan <[email protected]>
Cc Chris Mason <[email protected]>
Cc Yan Zheng <[email protected]>
Cc Sven Wegener <[email protected]>
Cc Josef Bacik <[email protected]>
Cc Jeff Mahoney <[email protected]>
---
fs/btrfs/file.c +0 -0
index 7c3cd24..09ef5d6 100644

fs/btrfs/file.c static ssize_t btrfs_file_write(struct file *file, const char __user *buf, · line 949

Quoted 6 lines

fs/btrfs/file.c +3 -0

@@ -944,14 +944,17 @@ static ssize_t btrfs_file_write(struct file *file, const char __user *buf,
 	if (count == 0)
 		goto out_nolock;
 
+	pages = kmalloc(nrptrs * sizeof(struct page *), GFP_KERNEL);
+	if (!pages)
+		goto out_nolock;

I guess you need to set err to -ENOMEM here so that the caller knows what's wrong. With your patch, this function just return 0(since num_written and err are both 0) with no error, and I guess it is worse than kernel BUG out when the NULL pages is used later.

Regards,
Tao

Amerigo Wang

author

2009-07-06 09:10 UTC #2 lore raw

Quoted 29 lines · Tao Ma

Hi Amerigo,
Quoted 26 lines · Amerigo Wang
V1 -> V2:
Move kmalloc() before mutex_lock(), suggested by Arjan.
file_remove_suid() should be called with i_mutex held, file_update_time() too. So move them after mutex_lock().
Plus, check the return value of kmalloc().
Signed-off-by WANG Cong <[email protected]>
Cc Arjan <[email protected]>
Cc Chris Mason <[email protected]>
Cc Yan Zheng <[email protected]>
Cc Sven Wegener <[email protected]>
Cc Josef Bacik <[email protected]>
Cc Jeff Mahoney <[email protected]>
---
fs/btrfs/file.c +0 -0
index 7c3cd24..09ef5d6 100644
@@ -944,14 +944,17 @@ static ssize_t btrfs_file_write(struct file 
 *file, const char __user *buf,
     if (count == 0)
         goto out_nolock;
 

Quoted 4 lines

fs/btrfs/file.c static ssize_t btrfs_file_write(struct file *file, const char __user *buf, · line 949
Quoted 3 lines
fs/btrfs/file.c +3 -0
+    pages = kmalloc(nrptrs * sizeof(struct page *), GFP_KERNEL);
+    if (!pages)
+        goto out_nolock;
I guess you need to set err to -ENOMEM here so that the caller knows what's wrong. With your patch, this function just return 0(since num_written and err are both 0) with no error, and I guess it is worse than kernel BUG out when the NULL pages is used later.

Agree. Thanks, I will update it.