[PATCH 1/2] mremap: don't allow VM_MIXEDMAP vma expanding

[PATCH 1/2] mremap: don't allow VM_MIXEDMAP vma expanding

[Shaohua Li]

Drivers using vm_insert_page() will set VM_MIXEDMAP, but their .fault
handler are likely not prepared to handle expansion.

Cc: Rik van Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 17fa018..3b886dc 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -354,7 +354,7 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	if (new_len > old_len) {
 		unsigned long pgoff;
 
-		if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP))
+		if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP | VM_MIXEDMAP))
 			goto Efault;
 		pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
 		pgoff += vma->vm_pgoff;
-- 
1.8.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] aio: make aio .mremap handle size changes

[Shaohua Li]

mremap aio ring buffer to another smaller vma is legal. For example,
mremap the ring buffer from the begining, though after the mremap, some
ring buffer pages can't be accessed in userspace because vma size is
shrinked. The problem is ctx->mmap_size isn't changed if the new ring
buffer vma size is changed. Latter io_destroy will zap all vmas within
mmap_size, which might zap unrelated vmas.

Cc: Benjamin LaHaise <bcrl@kvack.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 fs/aio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/aio.c b/fs/aio.c
index 1b7893e..fa354cf 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -306,6 +306,7 @@ static void aio_ring_remap(struct file *file, struct vm_area_struct *vma)
 		ctx = table->table[i];
 		if (ctx && ctx->aio_ring_file == file) {
 			ctx->user_id = ctx->mmap_base = vma->vm_start;
+			ctx->mmap_size = vma->vm_end - vma->vm_start;
 			break;
 		}
 	}
-- 
1.8.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] aio: make aio .mremap handle size changes

[Benjamin LaHaise]

On Tue, Feb 03, 2015 at 11:18:53AM -0800, Shaohua Li wrote:
> mremap aio ring buffer to another smaller vma is legal. For example,
> mremap the ring buffer from the begining, though after the mremap, some
> ring buffer pages can't be accessed in userspace because vma size is
> shrinked. The problem is ctx->mmap_size isn't changed if the new ring
> buffer vma size is changed. Latter io_destroy will zap all vmas within
> mmap_size, which might zap unrelated vmas.

Nak.  Shrinking the aio ring buffer is not a supported operation and will 
cause the application to lose events.  Make the size changing mremap fail, 
as this patch will not make the system do the right thing.

		-ben

> Cc: Benjamin LaHaise <bcrl@kvack.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  fs/aio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 1b7893e..fa354cf 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -306,6 +306,7 @@ static void aio_ring_remap(struct file *file, struct vm_area_struct *vma)
>  		ctx = table->table[i];
>  		if (ctx && ctx->aio_ring_file == file) {
>  			ctx->user_id = ctx->mmap_base = vma->vm_start;
> +			ctx->mmap_size = vma->vm_end - vma->vm_start;
>  			break;
>  		}
>  	}
> -- 
> 1.8.1

-- 
"Thought is the essence of where you are now."

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] aio: make aio .mremap handle size changes

[Shaohua Li]

On Tue, Feb 03, 2015 at 02:23:23PM -0500, Benjamin LaHaise wrote:
> On Tue, Feb 03, 2015 at 11:18:53AM -0800, Shaohua Li wrote:
> > mremap aio ring buffer to another smaller vma is legal. For example,
> > mremap the ring buffer from the begining, though after the mremap, some
> > ring buffer pages can't be accessed in userspace because vma size is
> > shrinked. The problem is ctx->mmap_size isn't changed if the new ring
> > buffer vma size is changed. Latter io_destroy will zap all vmas within
> > mmap_size, which might zap unrelated vmas.
> 
> Nak.  Shrinking the aio ring buffer is not a supported operation and will 
> cause the application to lose events.  Make the size changing mremap fail, 
> as this patch will not make the system do the right thing.

Yes, making the syscall fail (vma ops has .remap) is another option. If
the app uses io_getevents(), looks the app will not lose events, no? On
the other hand, I just want to make sure kernel does the right thing
(not zap unrelated vmas). If app does crazy things, it will break.

Thanks,
Shaohua

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] aio: make aio .mremap handle size changes

[Benjamin LaHaise]

On Tue, Feb 03, 2015 at 11:31:15AM -0800, Shaohua Li wrote:
> On Tue, Feb 03, 2015 at 02:23:23PM -0500, Benjamin LaHaise wrote:
> > On Tue, Feb 03, 2015 at 11:18:53AM -0800, Shaohua Li wrote:
> > > mremap aio ring buffer to another smaller vma is legal. For example,
> > > mremap the ring buffer from the begining, though after the mremap, some
> > > ring buffer pages can't be accessed in userspace because vma size is
> > > shrinked. The problem is ctx->mmap_size isn't changed if the new ring
> > > buffer vma size is changed. Latter io_destroy will zap all vmas within
> > > mmap_size, which might zap unrelated vmas.
> > 
> > Nak.  Shrinking the aio ring buffer is not a supported operation and will 
> > cause the application to lose events.  Make the size changing mremap fail, 
> > as this patch will not make the system do the right thing.
> 
> Yes, making the syscall fail (vma ops has .remap) is another option. If
> the app uses io_getevents(), looks the app will not lose events, no? On
> the other hand, I just want to make sure kernel does the right thing
> (not zap unrelated vmas). If app does crazy things, it will break.

But reading events out of the ring buffer is a supported mode of operation.  
Given that constraint, you should make an mremap changing the size of the 
ring buffer fail.

		-ben

> Thanks,
> Shaohua

-- 
"Thought is the essence of where you are now."

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] aio: make aio .mremap handle size changes

[Shaohua Li]

On Tue, Feb 03, 2015 at 02:48:28PM -0500, Benjamin LaHaise wrote:
> On Tue, Feb 03, 2015 at 11:31:15AM -0800, Shaohua Li wrote:
> > On Tue, Feb 03, 2015 at 02:23:23PM -0500, Benjamin LaHaise wrote:
> > > On Tue, Feb 03, 2015 at 11:18:53AM -0800, Shaohua Li wrote:
> > > > mremap aio ring buffer to another smaller vma is legal. For example,
> > > > mremap the ring buffer from the begining, though after the mremap, some
> > > > ring buffer pages can't be accessed in userspace because vma size is
> > > > shrinked. The problem is ctx->mmap_size isn't changed if the new ring
> > > > buffer vma size is changed. Latter io_destroy will zap all vmas within
> > > > mmap_size, which might zap unrelated vmas.
> > > 
> > > Nak.  Shrinking the aio ring buffer is not a supported operation and will 
> > > cause the application to lose events.  Make the size changing mremap fail, 
> > > as this patch will not make the system do the right thing.
> > 
> > Yes, making the syscall fail (vma ops has .remap) is another option. If
> > the app uses io_getevents(), looks the app will not lose events, no? On
> > the other hand, I just want to make sure kernel does the right thing
> > (not zap unrelated vmas). If app does crazy things, it will break.
> 
> But reading events out of the ring buffer is a supported mode of operation.  
> Given that constraint, you should make an mremap changing the size of the 
> ring buffer fail.

But if app chooses not to read the ring buffer, the app isn't broken.
This makes me hesitate to make the syscall fail.
There is a grey area about what's the correct semantics for mremap. We
currently don't limit any vma shrink for mremap.

Thanks,
Shaohua

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] aio: make aio .mremap handle size changes

[Benjamin LaHaise]

On Tue, Feb 03, 2015 at 01:31:50PM -0800, Shaohua Li wrote:
> On Tue, Feb 03, 2015 at 02:48:28PM -0500, Benjamin LaHaise wrote:
> > On Tue, Feb 03, 2015 at 11:31:15AM -0800, Shaohua Li wrote:
> > > On Tue, Feb 03, 2015 at 02:23:23PM -0500, Benjamin LaHaise wrote:
> > > > On Tue, Feb 03, 2015 at 11:18:53AM -0800, Shaohua Li wrote:
> > > > > mremap aio ring buffer to another smaller vma is legal. For example,
> > > > > mremap the ring buffer from the begining, though after the mremap, some
> > > > > ring buffer pages can't be accessed in userspace because vma size is
> > > > > shrinked. The problem is ctx->mmap_size isn't changed if the new ring
> > > > > buffer vma size is changed. Latter io_destroy will zap all vmas within
> > > > > mmap_size, which might zap unrelated vmas.
> > > > 
> > > > Nak.  Shrinking the aio ring buffer is not a supported operation and will 
> > > > cause the application to lose events.  Make the size changing mremap fail, 
> > > > as this patch will not make the system do the right thing.
> > > 
> > > Yes, making the syscall fail (vma ops has .remap) is another option. If
> > > the app uses io_getevents(), looks the app will not lose events, no? On
> > > the other hand, I just want to make sure kernel does the right thing
> > > (not zap unrelated vmas). If app does crazy things, it will break.
> > 
> > But reading events out of the ring buffer is a supported mode of operation.  
> > Given that constraint, you should make an mremap changing the size of the 
> > ring buffer fail.
> 
> But if app chooses not to read the ring buffer, the app isn't broken.
> This makes me hesitate to make the syscall fail.
> There is a grey area about what's the correct semantics for mremap. We
> currently don't limit any vma shrink for mremap.

mremap() on the aio ringbuffer was not previously supported.  The support was 
added recently was to enable checkpoint/restore to work properly.  In that 
use-case there is no need resize the ring buffer's mmap window.

If you really want to fix this, then make the mremap operation do the right 
thing and actually change the size of the aio ring.  That is going to be 
vastly more complex of a change for no real gain since no applications use 
that functionality.

		-ben

> Thanks,
> Shaohua

-- 
"Thought is the essence of where you are now."

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] aio: make aio .mremap handle size changes

[Shaohua Li]

On Tue, Feb 03, 2015 at 04:47:49PM -0500, Benjamin LaHaise wrote:
> On Tue, Feb 03, 2015 at 01:31:50PM -0800, Shaohua Li wrote:
> > On Tue, Feb 03, 2015 at 02:48:28PM -0500, Benjamin LaHaise wrote:
> > > On Tue, Feb 03, 2015 at 11:31:15AM -0800, Shaohua Li wrote:
> > > > On Tue, Feb 03, 2015 at 02:23:23PM -0500, Benjamin LaHaise wrote:
> > > > > On Tue, Feb 03, 2015 at 11:18:53AM -0800, Shaohua Li wrote:
> > > > > > mremap aio ring buffer to another smaller vma is legal. For example,
> > > > > > mremap the ring buffer from the begining, though after the mremap, some
> > > > > > ring buffer pages can't be accessed in userspace because vma size is
> > > > > > shrinked. The problem is ctx->mmap_size isn't changed if the new ring
> > > > > > buffer vma size is changed. Latter io_destroy will zap all vmas within
> > > > > > mmap_size, which might zap unrelated vmas.
> > > > > 
> > > > > Nak.  Shrinking the aio ring buffer is not a supported operation and will 
> > > > > cause the application to lose events.  Make the size changing mremap fail, 
> > > > > as this patch will not make the system do the right thing.
> > > > 
> > > > Yes, making the syscall fail (vma ops has .remap) is another option. If
> > > > the app uses io_getevents(), looks the app will not lose events, no? On
> > > > the other hand, I just want to make sure kernel does the right thing
> > > > (not zap unrelated vmas). If app does crazy things, it will break.
> > > 
> > > But reading events out of the ring buffer is a supported mode of operation.  
> > > Given that constraint, you should make an mremap changing the size of the 
> > > ring buffer fail.
> > 
> > But if app chooses not to read the ring buffer, the app isn't broken.
> > This makes me hesitate to make the syscall fail.
> > There is a grey area about what's the correct semantics for mremap. We
> > currently don't limit any vma shrink for mremap.
> 
> mremap() on the aio ringbuffer was not previously supported.  The support was 
> added recently was to enable checkpoint/restore to work properly.  In that 
> use-case there is no need resize the ring buffer's mmap window.
> 
> If you really want to fix this, then make the mremap operation do the right 
> thing and actually change the size of the aio ring.  That is going to be 
> vastly more complex of a change for no real gain since no applications use 
> that functionality.

That's too complex. Don't think I'll spend time on the no-usage
functionality. Will leave it be if you don't like the sample workaround.

Thanks,
Shaohua

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] aio: make aio .mremap handle size changes

[Benjamin LaHaise]

On Tue, Feb 03, 2015 at 02:58:45PM -0800, Shaohua Li wrote:
> That's too complex. Don't think I'll spend time on the no-usage
> functionality. Will leave it be if you don't like the sample workaround.

I'll fix the mremap case to disable resizing the ring buffer myself then.

		-ben
-- 
"Thought is the essence of where you are now."

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>