Re: [PATCH] Fix OOPS in mmap_region() when merging adjacent VM_LOCKEDfile segments

[Linus Torvalds]

On Thu, 29 Jan 2009, Greg KH wrote:
> 
> Which version was the "non-cleanup" version that should be added to the
> stable trees?

There were two different versions:

	From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
	Subject: Re: possible bug in mmap_region() in linux-2.6.28 kernel
	Message-Id: <20090128134350.034ac6a7.akpm@xxxxxxxxxxxxxxxxxxxx>

	From: Lee Schermerhorn <Lee.Schermerhorn@xxxxxx>
	Subject: [PATCH] Fix OOPS in mmap_region() when merging adjacent VM_LOCKED file segments
	Message-Id: <1233259410.2315.75.camel@lts-notebook>

and I'm actually not at all sure which one should go into stable (or if we 
should just pick the same one that went into mainline). 

> The commit you did make, de33c8db5910cda599899dd431cc30d7c1018cbf,
> seemed pretty "tiny" to me.

Oh, yes, in m any ways it's smaller than the trivial patches, since it's 
really just removing code (well, it adds "file" as a parameter to the 
first vma_merge, it used to always be NULL since we only did that for the 
"!file" case).

The downside with that commit is simply that there is a (pretty damn 
small, imho) possibility that somebody will report a regression with it, 
and we'll have to add some appropriate

	vma->vm_flags |= VM_IO | VM_DONTEXPAND;

to some odd special device driver, to make sure that it cannot trigger the 
"merge early" case because it needs its ->mmap() function called, rather 
than just expand the mapping quietly.

The reason(s) I don't think it's very likely is that

 (a) hopefully drivers already explicitly mark their mappings with one of 
     the VM_SPECIAL bits already. 

     Most such drivers would likely want to use "vm_insert_pfn()" (to 
     insert random IO pages into the mapping), and in order to do that 
     they have to add the VM_PFNMAP to vm_flags - we check. That would 
     disable any merging, because that flag won't be set in the new vma 
     before calling ->mmap.

 (b) even if the drivers don't do that explicit VM_PFNMAP, a driver that 
     does special things at mmap() time - even if it just uses regular 
     pages - probably prepopulates its mmap area with vm_insert_page().

     That in turn, will implicitly set VM_INSERTPAGE, and again: the 
     merge logic that verifies that the old vma->vm_flags == vm_flags (in 
     is_mergeable_vma) would disable merging.

 (c) finally - even if a merge really would be possible (ie vm_flags 
     didn't really get changed by the special ->mmap() function, but the 
     driver still depended on ->mmap() being called for some other 
     reason), I suspect it is really really unlikely that any application 
     would actually request two magic consecutive mmap's to the same 
     special device, with the same permissions and with just the right 
     pgoff values etc. IOW, even if a merge could happen in theory with a 
     flaky driver that doesn't like it, I don't think we'd hit it in 
     practice.

So I feel I had pretty good reasons to say "f*ck it, I'm going to fix this 
properly in mainline with a much prettier patch". 

But none of the above really changes the fact that the patch I committed 
to mainline was really quite fundamentally more invasive than either of 
the "simple" patches. All three patches are small, with mine arguably the 
smallest of the lot, but mine actually changed semantics, while Andrew's 
and Lee's patch literally only fix the invalid pointer use.

I'll leave it to others to decide which one goes into -stable. I 
personally don't really think it matters. I argue above that mine is 
pretty safe and thus perfectly fine even for -stable, but reality has a 
habit of sometimes disagreeing with me. Dang.

				Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/