Skip to contentSkip to conversation

btrfs: use file_remove_suid() after i_mutex is held

PATCH Superseded Amerigo Wang 2 participants 3 messages Jul 6, 2009 Btrfs file system

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Amerigo Wang

author
#0 lore raw

PATCH 1/1 btrfs: use file_remove_suid() after i_mutex is held

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]>

---

fs/btrfs/file.c +5 -2
index 7c3cd24..cd36301 100644
@@ -944,14 +944,17 @@ static ssize_t btrfs_file_write(struct file *file, const char __user *buf,
 	if (count == 0)
 		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);
+	if (!pages)
+		goto out;
 
-	mutex_lock(&inode->i_mutex);
 	BTRFS_I(inode)->sequence++;
 	first_index = pos >> PAGE_CACHE_SHIFT;
 	last_index = (pos + count) >> PAGE_CACHE_SHIFT;
#1 lore raw
Quoted 19 lines · On Mon, 6 Jul 2009 01:29:14 -0400 Amerigo Wang <[email protected]> wrote:

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]>

---

fs/btrfs/file.c +2 -0
index 7c3cd24..cd36301 100644
@@ -944,14 +944,17 @@ static ssize_t btrfs_file_write(struct file
 *file, const char __user *buf, if (count == 0)
 		goto out_nolock;
 
+	mutex_lock(&inode->i_mutex);
+

fs/btrfs/file.c static ssize_t btrfs_file_write(struct file · line 957

Quoted 10 lines
fs/btrfs/file.c +3 -1
 	err = file_remove_suid(file);
 	if (err)
-		goto out_nolock;
+		goto out;
 	file_update_time(file);
 
 	pages = kmalloc(nrptrs * sizeof(struct page *), GFP_KERNEL);
+	if (!pages)
+		goto out;

Hi,

I don't think you can keep this at GFP_KERNEL once you hold i_mutex.... very likely this needs to now turn into GFP_NOFS!

Signature · 3 lines
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
author
#2 lore raw
Quoted 35 lines · Arjan van de Ven
Quoted 19 lines · On Mon, 6 Jul 2009 01:29:14 -0400 Amerigo Wang <[email protected]> wrote:

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]>

---

fs/btrfs/file.c +2 -0
index 7c3cd24..cd36301 100644
@@ -944,14 +944,17 @@ static ssize_t btrfs_file_write(struct file
 *file, const char __user *buf, if (count == 0)
 		goto out_nolock;
 
+	mutex_lock(&inode->i_mutex);
+
 	err = file_remove_suid(file);

fs/btrfs/file.c static ssize_t btrfs_file_write(struct file · line 958

Quoted 10 lines
fs/btrfs/file.c +3 -1
 	if (err)
-		goto out_nolock;
+		goto out;
 	file_update_time(file);
 
 	pages = kmalloc(nrptrs * sizeof(struct page *), GFP_KERNEL);
+	if (!pages)
+		goto out;

Hi,

Quoted 3 lines

I don't think you can keep this at GFP_KERNEL once you hold i_mutex.... very likely this needs to now turn into GFP_NOFS!

Good point!
Hmm, GFP_KERNEL adds __GFP_FS while GFP_NOFS not...
Just moving that kmalloc() up, before mutex_lock(), I think, can solve this.

I will update this patch now...

Thanks!