[PATCH, RFC] xip: use i_mutex for xip_file_fault

[PATCH, RFC] xip: use i_mutex for xip_file_fault

[Marco Stornelli]

From: Marco Stornelli <marco.stornelli@gmail.com>

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 <marco.stornelli@gmail.com>
---

--- filemap_xip.c.orig	2011-09-10 17:16:56.000000000 +0200
+++ filemap_xip.c	2011-09-10 17:19:25.000000000 +0200
@@ -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;
 	}

Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault

[Al Viro]

On Sat, Sep 10, 2011 at 05:31:39PM +0200, Marco Stornelli wrote:
> From: Marco Stornelli <marco.stornelli@gmail.com>
> 
> 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?

Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault

[Marco Stornelli]

Il 10/09/2011 17:56, Al Viro ha scritto:
> On Sat, Sep 10, 2011 at 05:31:39PM +0200, Marco Stornelli wrote:
>> From: Marco Stornelli<marco.stornelli@gmail.com>
>>
>> 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?
>

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

Marco

Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault

[Al Viro]

On Sun, Sep 11, 2011 at 10:25:03AM +0200, Marco Stornelli wrote:
> Il 10/09/2011 17:56, Al Viro ha scritto:
> >On Sat, Sep 10, 2011 at 05:31:39PM +0200, Marco Stornelli wrote:
> >>From: Marco Stornelli<marco.stornelli@gmail.com>
> >>
> >>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?
> >
> 
> 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.

Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault

[Al Viro]

On Sun, Sep 11, 2011 at 12:15:04PM +0100, Al Viro wrote:

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

Re: [PATCH, RFC] xip: use i_mutex for xip_file_fault

[Marco Stornelli]

Il 11/09/2011 13:25, Al Viro ha scritto:
> On Sun, Sep 11, 2011 at 12:15:04PM +0100, Al Viro wrote:
>
>> 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.
>

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