xip: use i_mutex for xip_file_fault

RFC PATCH In review Marco Stornelli 2 participants 6 messages Sep 10, 2011 – Sep 11, 2011 VFS

lore

Share & tools

lore

Marco Stornelli

author

2011-09-10 15:37 UTC #0 lore raw

RFC PATCH 1/1 xip: use i_mutex for xip_file_fault

From: Marco Stornelli <[email protected]>

There aren't sufficient sync points for a fs for xip operations. In particular for the mmap case. It can be not sufficient to lock/unlock to do some operation inside get_xip_mem callback. For xip_file_read it's really easy to write a fs specific wrapper, xip_file_write hold i_mutex so no problem. With this patch we can avoid concurrent truncate operation and xip mmap.

Signed-off-by Marco Stornelli <[email protected]>

---

filemap_xip.c renamed from filemap_xip.c.orig +12 -3

@@ -230,6 +230,7 @@ static int xip_file_fault(struct vm_area
 	int error;
 
 	/* XXX: are VM_FAULT_ codes OK? */
+	mutex_lock(&inode->i_mutex);
 again:
 	size = (i_size_read(inode) + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
 	if (vmf->pgoff >= size)

@@ -239,8 +240,10 @@ again:
 						&xip_mem, &xip_pfn);
 	if (likely(!error))
 		goto found;
-	if (error != -ENODATA)
+	if (error != -ENODATA) {
+		mutex_unlock(&inode->i_mutex);
 		return VM_FAULT_OOM;
+	}
 
 	/* sparse block */
 	if ((vma->vm_flags & (VM_WRITE | VM_MAYWRITE)) &&

@@ -253,17 +256,22 @@ again:
 		error = mapping->a_ops->get_xip_mem(mapping, vmf->pgoff, 1,
 							&xip_mem, &xip_pfn);
 		mutex_unlock(&xip_sparse_mutex);
-		if (error)
+		if (error) {
+			mutex_unlock(&inode->i_mutex);
 			return VM_FAULT_SIGBUS;
+		}
 		/* unmap sparse mappings at pgoff from all other vmas */
 		__xip_unmap(mapping, vmf->pgoff);
 
 found:
 		err = vm_insert_mixed(vma, (unsigned long)vmf->virtual_address,
 							xip_pfn);
-		if (err == -ENOMEM)
+		if (err == -ENOMEM) {
+			mutex_unlock(&inode->i_mutex);
 			return VM_FAULT_OOM;
+		}
 		BUG_ON(err);
+		mutex_unlock(&inode->i_mutex);
 		return VM_FAULT_NOPAGE;
 	} else {
 		int err, ret = VM_FAULT_OOM;

@@ -292,6 +300,7 @@ found:
 out:
 		write_seqcount_end(&xip_sparse_seq);
 		mutex_unlock(&xip_sparse_mutex);
+		mutex_unlock(&inode->i_mutex);
 
 		return ret;
 	}

Al Viro

maintainer

2011-09-10 15:56 UTC #1 lore raw

Quoted 8 lines · Marco Stornelli

From: Marco Stornelli <[email protected]>
There aren't sufficient sync points for a fs for xip operations. In particular for the mmap case. It can be not sufficient to lock/unlock to do some operation inside get_xip_mem callback. For xip_file_read it's really easy to write a fs specific wrapper, xip_file_write hold i_mutex so no problem. With this patch we can avoid concurrent truncate operation and xip mmap.

Umm... I really don't like that; what's going to happen if you have a file mmapped and do write() to that file from address in that mapping?

Marco Stornelli

author

2011-09-11 08:31 UTC #2 lore raw

Il 10/09/2011 17:56, Al Viro ha scritto:

Quoted 10 lines · Marco Stornelli

Quoted 8 lines
From: Marco Stornelli<[email protected]>
There aren't sufficient sync points for a fs for xip operations. In particular for the mmap case. It can be not sufficient to lock/unlock to do some operation inside get_xip_mem callback. For xip_file_read it's really easy to write a fs specific wrapper, xip_file_write hold i_mutex so no problem. With this patch we can avoid concurrent truncate operation and xip mmap.

Quoted 3 lines

Umm... I really don't like that; what's going to happen if you have a file mmapped and do write() to that file from address in that mapping?

Nothing strange. There is a serialization of the operations. Maybe I don't understand the point here.

Marco

Al Viro

maintainer

2011-09-11 11:15 UTC #3 lore raw

Quoted 11 lines · Marco Stornelli

Il 10/09/2011 17:56, Al Viro ha scritto:
Quoted 10 lines · Marco Stornelli
Quoted 8 lines
From: Marco Stornelli<[email protected]>
There aren't sufficient sync points for a fs for xip operations. In particular for the mmap case. It can be not sufficient to lock/unlock to do some operation inside get_xip_mem callback. For xip_file_read it's really easy to write a fs specific wrapper, xip_file_write hold i_mutex so no problem. With this patch we can avoid concurrent truncate operation and xip mmap.

Quoted 6 lines

Quoted 3 lines
Umm... I really don't like that; what's going to happen if you have a file mmapped and do write() to that file from address in that mapping?
Nothing strange. There is a serialization of the operations. Maybe I don't understand the point here.

write() grabs ->i_mutex on the file it's going to write to. It uses copy_from_user() while holding ->i_mutex; that can end up calling ->fault(). If your data comes from the same file mmapped in your address space, you have xip_write_fault() called while you are in xip_file_write() and *already* are holding ->i_mutex on the same inode. With your patch it will, AFAICS, cheerfully deadlock.

Al Viro

maintainer

2011-09-11 11:25 UTC #4 lore raw

Quoted 6 lines · Al Viro

write() grabs ->i_mutex on the file it's going to write to. It uses copy_from_user() while holding ->i_mutex; that can end up calling ->fault(). If your data comes from the same file mmapped in your address space, you have xip_write_fault() called while you are in xip_file_write() and *already* are holding ->i_mutex on the same inode. With your patch it will, AFAICS, cheerfully deadlock.

Oh, wait... You are only doing that to write side of pagefault? That's better, but not much:

thread 1: mmap the file, modify mapping
thread 2: write() to file

The former will do xip_write_fault() while holding ->mmap_sem. The latter will do copy_from_user() from xip_file_write(), getting pagefaults while holding ->i_mutex.

Note that we are grabbing ->mmap_sem and ->i_mutex in opposite orders. I.e. that will deadlock on you - all you need is threads sharing the address space.

Marco Stornelli

author

2011-09-11 16:03 UTC #5 lore raw

Il 11/09/2011 13:25, Al Viro ha scritto:

Quoted 15 lines · Al Viro

Quoted 7 lines
write() grabs ->i_mutex on the file it's going to write to. It uses copy_from_user() while holding ->i_mutex; that can end up calling ->fault(). If your data comes from the same file mmapped in your address space, you have xip_write_fault() called while you are in xip_file_write() and *already* are holding ->i_mutex on the same inode. With your patch it will, AFAICS, cheerfully deadlock.
Oh, wait... You are only doing that to write side of pagefault? That's better, but not much:
thread 1: mmap the file, modify mapping
thread 2: write() to file

Quoted 8 lines

The former will do xip_write_fault() while holding ->mmap_sem. The latter will do copy_from_user() from xip_file_write(), getting pagefaults while holding ->i_mutex.
Note that we are grabbing ->mmap_sem and ->i_mutex in opposite orders. I.e. that will deadlock on you - all you need is threads sharing the address space.

Ok, thank you very much for the on-line debug :) So i_mutex is not a good lock to use in this situation. It was a common sync point, but it has some collateral effect on the write path that we must avoid. At this point, what can be a good strategy? Any opinion is welcome.

Marco