Re: [PATCH] mm: mmu_notifier: re-fix freed page still mapped in secondary MMU

[Robin Holt <holt@sgi.com>]

On Thu, Apr 18, 2013 at 02:41:31AM +0800, Xiao Guangrong wrote:
> On 04/17/2013 10:10 PM, Robin Holt wrote:
> > On Wed, Apr 17, 2013 at 10:55:26AM +0800, Xiao Guangrong wrote:
> >> On 04/17/2013 02:08 AM, Robin Holt wrote:
> >>> On Tue, Apr 16, 2013 at 09:07:20PM +0800, Xiao Guangrong wrote:
> >>>> On 04/16/2013 07:43 PM, Robin Holt wrote:
> >>>>> Argh.  Taking a step back helped clear my head.
> >>>>>
> >>>>> For the -stable releases, I agree we should just go with your
> >>>>> revert-plus-hlist_del_init_rcu patch.  I will give it a test
> >>>>> when I am in the office.
> >>>>
> >>>> Okay. Wait for your test report. Thank you in advance.
> >>>>
> >>>>>
> >>>>> For the v3.10 release, we should work on making this more
> >>>>> correct and completely documented.
> >>>>
> >>>> Better document is always welcomed.
> >>>>
> >>>> Double call ->release is not bad, like i mentioned it in the changelog:
> >>>>
> >>>> it is really rare (e.g, can not happen on kvm since mmu-notify is unregistered
> >>>> after exit_mmap()) and the later call of multiple ->release should be
> >>>> fast since all the pages have already been released by the first call.
> >>>>
> >>>> But, of course, it's great if you have a _light_ way to avoid this.
> >>>
> >>> Getting my test environment set back up took longer than I would have liked.
> >>>
> >>> Your patch passed.  I got no NULL-pointer derefs.
> >>
> >> Thanks for your test again.
> >>
> >>>
> >>> How would you feel about adding the following to your patch?
> >>
> >> I prefer to make these changes as a separate patch, this change is the
> >> improvement, please do not mix it with bugfix.
> > 
> > I think your "improvement" classification is a bit deceiving.  My previous
> > patch fixed the bug in calling release multiple times.  Your patch without
> > this will reintroduce that buggy behavior.  Just because the bug is already
> > worked around by KVM does not mean it is not a bug.
> 
> As your tested, calling ->release() multiple times can work, but just make your
> testcase more _slower_. So your changes is trying to speed it up - it is a
> improvement.
> 
> Well, _if_ it is really a bug, could you please do not fix two bugs in one patch?

The code, as is, does not call ->release() multiple times.  Your code
changes the behavior to call it multiple times.  You are introducing the
bug by your code changes.  Why not fix the bug you create in the patch
which creates it?

Robin

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