LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Will Deacon]

Hi Kirill,

Your patch 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")
causes some mmap regressions in LTP, which appears to use a MAP_PRIVATE
mmap of /dev/zero as a way to get anonymous pages in some of its tests
(specifically mmap10 [1]).

Dead simple reproducer below. Is this change in behaviour intentional?

Will

[1]
http://sourceforge.net/p/ltp/git/ci/1eb440c2b5fe43a3e5023015a16aa5d7d3385b1e/tree/testcases/kernel/syscalls/mmap/mmap10.c

--->8

#include <sys/mman.h>
#include <sys/stat.h>

#include <fcntl.h>
#include <stdio.h>

#define MAP_SZ	5*1024*1024

int main(void)
{
	char *foo;
	int fd;

	fd = open("/dev/zero", O_RDWR, 0666);
	if (fd < 0) {
		perror(NULL);
		return fd;
	}

	foo = mmap(NULL, MAP_SZ, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
	if (foo == MAP_FAILED) {
		perror(NULL);
		return -1;
	}

	foo[MAP_SZ >> 1] = 0; // Generates SIGBUS with 4.3-rc1
	return 0;
}

--
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: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Kirill A. Shutemov]

Will Deacon wrote:
> Hi Kirill,
> 
> Your patch 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")
> causes some mmap regressions in LTP, which appears to use a MAP_PRIVATE
> mmap of /dev/zero as a way to get anonymous pages in some of its tests
> (specifically mmap10 [1]).
> 
> Dead simple reproducer below. Is this change in behaviour intentional?

Ouch. Of couse it's a bug.

Fix is below. I don't really like it, but I cannot find any better
solution.

Re: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Will Deacon]

On Mon, Sep 14, 2015 at 12:57:59PM +0100, Kirill A. Shutemov wrote:
> Will Deacon wrote:
> > Your patch 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")
> > causes some mmap regressions in LTP, which appears to use a MAP_PRIVATE
> > mmap of /dev/zero as a way to get anonymous pages in some of its tests
> > (specifically mmap10 [1]).
> > 
> > Dead simple reproducer below. Is this change in behaviour intentional?
> 
> Ouch. Of couse it's a bug.
> 
> Fix is below. I don't really like it, but I cannot find any better
> solution.

Brill, thanks for the quick response! I agree that the fix isn't very
nice. Maybe moving the mmap_zero test into a helper would make it more
palatable?

> From 97be4458fd63758b0c233e528bf952d1cd26428e Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Mon, 14 Sep 2015 14:44:32 +0300
> Subject: [PATCH] mm: fix mmap(MAP_PRIVATE) on /dev/zero
> 
> In attempt to make mm less fragile I've screwed up setting up anonymous
> mappings by mmap() on /dev/zero.
> 
> Here's the fix.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Fixes: 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")
> Reported-by: Will Deacon <will.deacon@arm.com>

FWIW:

  Tested-by: Will Deacon <will.deacon@arm.com>

Cheers,

Will

> ---
>  drivers/char/mem.c | 2 +-
>  include/linux/mm.h | 1 +
>  mm/mmap.c          | 6 ++++--
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 6b1721f978c2..c8fe3af4de29 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -651,7 +651,7 @@ static ssize_t read_iter_zero(struct kiocb *iocb, struct iov_iter *iter)
>  	return written;
>  }
>  
> -static int mmap_zero(struct file *file, struct vm_area_struct *vma)
> +int mmap_zero(struct file *file, struct vm_area_struct *vma)
>  {
>  #ifndef CONFIG_MMU
>  	return -ENOSYS;
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 91c08f6f0dc9..5e152e9588ec 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1066,6 +1066,7 @@ extern void pagefault_out_of_memory(void);
>  extern void show_free_areas(unsigned int flags);
>  extern bool skip_free_areas_node(unsigned int flags, int nid);
>  
> +int mmap_zero(struct file *file, struct vm_area_struct *vma);
>  int shmem_zero_setup(struct vm_area_struct *);
>  #ifdef CONFIG_SHMEM
>  bool shmem_mapping(struct address_space *mapping);
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 971dd2cb77d2..7960fd206a2f 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -612,7 +612,9 @@ static unsigned long count_vma_pages_range(struct mm_struct *mm,
>  void __vma_link_rb(struct mm_struct *mm, struct vm_area_struct *vma,
>  		struct rb_node **rb_link, struct rb_node *rb_parent)
>  {
> -	WARN_ONCE(vma->vm_file && !vma->vm_ops, "missing vma->vm_ops");
> +	WARN_ONCE(vma->vm_file && !vma->vm_ops &&
> +			vma->vm_file->f_op->mmap != mmap_zero,
> +			"missing vma->vm_ops");
>  
>  	/* Update tracking information for the gap following the new vma. */
>  	if (vma->vm_next)
> @@ -1639,7 +1641,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  		WARN_ON_ONCE(addr != vma->vm_start);
>  
>  		/* All file mapping must have ->vm_ops set */
> -		if (!vma->vm_ops) {
> +		if (!vma->vm_ops && file->f_op->mmap != &mmap_zero) {
>  			static const struct vm_operations_struct dummy_ops = {};
>  			vma->vm_ops = &dummy_ops;
>  		}
> -- 
> 2.5.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: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Oleg Nesterov]

On 09/14, Kirill A. Shutemov wrote:
>
> Fix is below. I don't really like it, but I cannot find any better
> solution.

Me too...

But this change "documents" the nasty special "vm_file && !vm_ops" case, and
I am not sure how we can remove it later...

So perhaps we should change vma_is_anonymous() back to check ->fault too,

	 static inline bool vma_is_anonymous(struct vm_area_struct *vma)
	 {
	-	return !vma->vm_ops;
	+	return !vma->vm_ops || !vma->vm_ops->fault;
	 }

and remove the "vma->vm_file && !vma->vm_ops" checks in mmap_region() paths.

I dunno.

Oleg.

--
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: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Oleg Nesterov]

On 09/14, Oleg Nesterov wrote:
>
> On 09/14, Kirill A. Shutemov wrote:
> >
> > Fix is below. I don't really like it, but I cannot find any better
> > solution.
>
> Me too...
>
> But this change "documents" the nasty special "vm_file && !vm_ops" case, and
> I am not sure how we can remove it later...
>
> So perhaps we should change vma_is_anonymous() back to check ->fault too,
>
> 	 static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> 	 {
> 	-	return !vma->vm_ops;
> 	+	return !vma->vm_ops || !vma->vm_ops->fault;
> 	 }
>
> and remove the "vma->vm_file && !vma->vm_ops" checks in mmap_region() paths.
  ^^^

sorry, I actually meant "or remove". But perhaps "and" makes sense too.

Say, if we change vma_is_anonymous() as above, then we can kill
arch_vma_name() on x86. mpx_mmap() can install the dummy vm_ops with
the single ->name() method.

> I dunno.

Yes. Up to you.

Oleg.

--
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: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Kirill A. Shutemov]

On Mon, Sep 14, 2015 at 07:05:47PM +0200, Oleg Nesterov wrote:
> On 09/14, Kirill A. Shutemov wrote:
> >
> > Fix is below. I don't really like it, but I cannot find any better
> > solution.
> 
> Me too...
> 
> But this change "documents" the nasty special "vm_file && !vm_ops" case, and
> I am not sure how we can remove it later...
> 
> So perhaps we should change vma_is_anonymous() back to check ->fault too,
> 
> 	 static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> 	 {
> 	-	return !vma->vm_ops;
> 	+	return !vma->vm_ops || !vma->vm_ops->fault;

No. This would give a lot false positives from drives which setup page
tables upfront and don't use ->fault at all.

> 	 }
> 
> and remove the "vma->vm_file && !vma->vm_ops" checks in mmap_region() paths.
> 
> I dunno.
> 
> Oleg.
> 
> --
> 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>

-- 
 Kirill A. Shutemov

--
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: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Oleg Nesterov]

On 09/14, Kirill A. Shutemov wrote:
>
> On Mon, Sep 14, 2015 at 07:05:47PM +0200, Oleg Nesterov wrote:
> > On 09/14, Kirill A. Shutemov wrote:
> > >
> > > Fix is below. I don't really like it, but I cannot find any better
> > > solution.
> >
> > Me too...
> >
> > But this change "documents" the nasty special "vm_file && !vm_ops" case, and
> > I am not sure how we can remove it later...
> >
> > So perhaps we should change vma_is_anonymous() back to check ->fault too,
> >
> > 	 static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> > 	 {
> > 	-	return !vma->vm_ops;
> > 	+	return !vma->vm_ops || !vma->vm_ops->fault;
>
> No. This would give a lot false positives from drives which setup page
> tables upfront and don't use ->fault at all.

And? I mean, I am not sure I understand what exactly do you dislike.

Firstly, I still think that (in the long term) we should change them
to use .faul = no_fault() which just returns VM_FAULT_SIGBUS.

Until then I do not see why the change above can be really bad. The
VM_SHARED case is fine, do_anonymous_page() will return VM_FAULT_SIGBUS.

So afaics the only problem is that after the change above the private
mapping can silently get an anonymous page after (say) MADV_DONTNEED
instead of the nice SIGBUS from do_fault(). I agree, this is not good,
but see above.

Or I missed something else?

Let me repeat, I am not going to really argue, you understand this all
much better than me. But imho we should try to avoid the special case
added by your change as much as possible, in this sense the change above
looks "obviously better" at least as a short-term fix.


Whether we need to keep the vm_ops/fault check in __vma_link_rb() and
mmap_region() is another issue. But if we keep them, then I think we
should at least turn the !vma->vm_ops check in mmap_region into
WARN_ON() as well.

Oleg.

--
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: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Kirill A. Shutemov]

On Tue, Sep 15, 2015 at 02:12:01PM +0200, Oleg Nesterov wrote:
> On 09/14, Kirill A. Shutemov wrote:
> >
> > On Mon, Sep 14, 2015 at 07:05:47PM +0200, Oleg Nesterov wrote:
> > > On 09/14, Kirill A. Shutemov wrote:
> > > >
> > > > Fix is below. I don't really like it, but I cannot find any better
> > > > solution.
> > >
> > > Me too...
> > >
> > > But this change "documents" the nasty special "vm_file && !vm_ops" case, and
> > > I am not sure how we can remove it later...
> > >
> > > So perhaps we should change vma_is_anonymous() back to check ->fault too,
> > >
> > > 	 static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> > > 	 {
> > > 	-	return !vma->vm_ops;
> > > 	+	return !vma->vm_ops || !vma->vm_ops->fault;
> >
> > No. This would give a lot false positives from drives which setup page
> > tables upfront and don't use ->fault at all.
> 
> And? I mean, I am not sure I understand what exactly do you dislike.
> 
> Firstly, I still think that (in the long term) we should change them
> to use .faul = no_fault() which just returns VM_FAULT_SIGBUS.

I would rather like to see consolidated fault path between file and anon
with ->vm_ops set for both. So vma_is_anonymous() will be trivial
vma->vm_ops == anon_vm_ops.

> Until then I do not see why the change above can be really bad. The
> VM_SHARED case is fine, do_anonymous_page() will return VM_FAULT_SIGBUS.
> 
> So afaics the only problem is that after the change above the private
> mapping can silently get an anonymous page after (say) MADV_DONTNEED
> instead of the nice SIGBUS from do_fault(). I agree, this is not good,
> but see above.

So, what the point to introduce vma_is_anonymous() if it often produces
false result? vma_is_anonymous_or_maybe_not()?

> Or I missed something else?
> 
> Let me repeat, I am not going to really argue, you understand this all
> much better than me. But imho we should try to avoid the special case
> added by your change as much as possible, in this sense the change above
> looks "obviously better" at least as a short-term fix.
> 
> 
> Whether we need to keep the vm_ops/fault check in __vma_link_rb() and
> mmap_region() is another issue. But if we keep them, then I think we
> should at least turn the !vma->vm_ops check in mmap_region into
> WARN_ON() as well.

It would require first fix all known cases where ->f_op->mmap() returns
vma->vm_ops == NULL. Not subject for 4.3, I think.

-- 
 Kirill A. Shutemov

--
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: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Oleg Nesterov]

On 09/15, Kirill A. Shutemov wrote:
>
> On Tue, Sep 15, 2015 at 02:12:01PM +0200, Oleg Nesterov wrote:
> > On 09/14, Kirill A. Shutemov wrote:
> > >
> > > On Mon, Sep 14, 2015 at 07:05:47PM +0200, Oleg Nesterov wrote:
> > > > On 09/14, Kirill A. Shutemov wrote:
> > > > >
> > > > > Fix is below. I don't really like it, but I cannot find any better
> > > > > solution.
> > > >
> > > > Me too...
> > > >
> > > > But this change "documents" the nasty special "vm_file && !vm_ops" case, and
> > > > I am not sure how we can remove it later...
> > > >
> > > > So perhaps we should change vma_is_anonymous() back to check ->fault too,
> > > >
> > > > 	 static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> > > > 	 {
> > > > 	-	return !vma->vm_ops;
> > > > 	+	return !vma->vm_ops || !vma->vm_ops->fault;
> > >
> > > No. This would give a lot false positives from drives which setup page
> > > tables upfront and don't use ->fault at all.
> >
> > And? I mean, I am not sure I understand what exactly do you dislike.
> >
> > Firstly, I still think that (in the long term) we should change them
> > to use .faul = no_fault() which just returns VM_FAULT_SIGBUS.
>
> I would rather like to see consolidated fault path between file and anon
> with ->vm_ops set for both. So vma_is_anonymous() will be trivial
> vma->vm_ops == anon_vm_ops.

I too thought about this. Perhaps but I guess this needs another
discussion.

In particular I am not sure we should just rely on vm_ops == anon_vm_ops.
Again, it is not that I think that the VM_MPX check in arch_vma_name() is
that bad. Still I think it would be better if mpx_mmap() could install
vma->vm_ops = mpx_vm_ops with ->name(). So perhaps ->anon_fault() makes
more sense. But lets not discuss this right now.

>
> > Until then I do not see why the change above can be really bad. The
> > VM_SHARED case is fine, do_anonymous_page() will return VM_FAULT_SIGBUS.
> >
> > So afaics the only problem is that after the change above the private
> > mapping can silently get an anonymous page after (say) MADV_DONTNEED
> > instead of the nice SIGBUS from do_fault(). I agree, this is not good,
> > but see above.
>
> So, what the point to introduce vma_is_anonymous() if it often produces
> false result? vma_is_anonymous_or_maybe_not()?

Heh.

Then what the point to demand that "All file mapping must have ->vm_ops set"
if mmap(MAP_PRIVATE, "/dev/zero") has ->vm_ops == NULL ? Because this is
not actually the file mapping, yes. And this is why we want vma_is_anonymous()
to return T in this case.

vma_is_anonymous() just says that a page fault will use do_anonymous_page().
I agree, it would be nice to ensure vma_is_anonymous() can only be true
if this vma can only have the anon pages. Let me repeat that I suggested
this change as a short-term fix (at least without other changes like we
discuss above). Because the mmap_zero() hack looks worse to me. Damn, even
the ugly hack below looks better to me.

> > Whether we need to keep the vm_ops/fault check in __vma_link_rb() and
> > mmap_region() is another issue. But if we keep them, then I think we
> > should at least turn the !vma->vm_ops check in mmap_region into
> > WARN_ON() as well.
>
> It would require first fix all known cases where ->f_op->mmap() returns
> vma->vm_ops == NULL. Not subject for 4.3, I think.

Kirill, I even sent you the private email to clarify that - of course! -
I only meant "in the longer term" ;)

Oleg.

--- x/include/linux/mm.h
+++ x/include/linux/mm.h
@@ -1289,9 +1289,11 @@ static inline int vma_growsdown(struct v
 	return vma && (vma->vm_end == addr) && (vma->vm_flags & VM_GROWSDOWN);
 }
 
+#define xxx_fault	((void*)1)
+
 static inline bool vma_is_anonymous(struct vm_area_struct *vma)
 {
-	return !vma->vm_ops;
+	return !vma->vm_ops || vma->vm_ops->fault == xxx_fault;
 }
 
 static inline int stack_guard_page_start(struct vm_area_struct *vma,
--- x/drivers/char/mem.c
+++ x/drivers/char/mem.c
@@ -653,11 +653,17 @@ static ssize_t read_iter_zero(struct kio
 
 static int mmap_zero(struct file *file, struct vm_area_struct *vma)
 {
+	static const struct vm_operations_struct xxx_ops = {
+		.fault = xxx_fault,
+	};
+		}
 #ifndef CONFIG_MMU
 	return -ENOSYS;
 #endif
 	if (vma->vm_flags & VM_SHARED)
 		return shmem_zero_setup(vma);
+
+	vma->vm_ops = &xxx_ops;
 	return 0;
 }
 

--
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: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Andrew Morton]

On Tue, 15 Sep 2015 16:42:16 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> I would rather like to see consolidated fault path between file and anon
> with ->vm_ops set for both. So vma_is_anonymous() will be trivial
> vma->vm_ops == anon_vm_ops.

People are noticing: https://bugzilla.kernel.org/show_bug.cgi?id=104691

How about I send Linus a revert of 6dc296e7df4c while we work out what
to do?

--
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: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Kirill A. Shutemov]

On Wed, Sep 16, 2015 at 02:28:18PM -0700, Andrew Morton wrote:
> On Tue, 15 Sep 2015 16:42:16 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > I would rather like to see consolidated fault path between file and anon
> > with ->vm_ops set for both. So vma_is_anonymous() will be trivial
> > vma->vm_ops == anon_vm_ops.
> 
> People are noticing: https://bugzilla.kernel.org/show_bug.cgi?id=104691
> 
> How about I send Linus a revert of 6dc296e7df4c while we work out what
> to do?

I think it's the best step for now. Although, I'm not sure when I will get
time on reworking fault path :-/

-- 
 Kirill A. Shutemov

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