Re: hwpoison madvise code

Re: hwpoison madvise code

[Nick Piggin]

On Tue, Dec 08, 2009 at 12:26:23PM +0100, Andi Kleen wrote:
> On Tue, Dec 08, 2009 at 12:24:12PM +0100, Nick Piggin wrote:
> > Hi,
> > 
> > Seems like the madvise hwpoison code is ugly and buggy, not to
> > put too fine a point on it :)
> > 
> > Ugly: it should have just followed the same pattern as the other
> > transient advices.
> 
> That wouldn't work.

Of course it will. You may have no need to be given the actual
vma, but that's no big deal. Much better than making up your own
way of doing things.

 
> > Buggy: it doesn't take mmap_sem. If it followed the pattern, it
> > wouldn't have had this bug.
> 
> get_user_pages takes mmap_sem if needed.

On the contrary it is clearly documented as requiring mmap_sem.

Nick

--
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: hwpoison madvise code

[Nick Piggin]

On Tue, Dec 08, 2009 at 01:01:56PM +0100, Andi Kleen wrote:
> > > > Buggy: it doesn't take mmap_sem. If it followed the pattern, it
> > > > wouldn't have had this bug.
> > > 
> > > get_user_pages takes mmap_sem if needed.
> > 
> > On the contrary it is clearly documented as requiring mmap_sem.
> 
> True, I forgot about that. I think at some point I had gup_fast
> in there, I'll just switch back to that. Thanks for the kind note.

My pleasure :) 

> BTW looking over the tree I find at least one other caller that doesn't
> hold it, like futex.c:fault_in_user_writeable. I'll send a separate
> patch for t hat.

Nasty. Good catch.

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

hwpoison madvise code

[Nick Piggin]

Hi,

Seems like the madvise hwpoison code is ugly and buggy, not to
put too fine a point on it :)

Ugly: it should have just followed the same pattern as the other
transient advices.
Buggy: it doesn't take mmap_sem. If it followed the pattern, it
wouldn't have had this bug.

Please fix.

Thanks,
Nick

--
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: hwpoison madvise code

[Andi Kleen]

On Tue, Dec 08, 2009 at 12:24:12PM +0100, Nick Piggin wrote:
> Hi,
> 
> Seems like the madvise hwpoison code is ugly and buggy, not to
> put too fine a point on it :)
> 
> Ugly: it should have just followed the same pattern as the other
> transient advices.

That wouldn't work.

> Buggy: it doesn't take mmap_sem. If it followed the pattern, it
> wouldn't have had this bug.

get_user_pages takes mmap_sem if needed.

If you think that is broken please describe the failure scenario
in detail.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

--
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: hwpoison madvise code

[Andi Kleen]

> > > Buggy: it doesn't take mmap_sem. If it followed the pattern, it
> > > wouldn't have had this bug.
> > 
> > get_user_pages takes mmap_sem if needed.
> 
> On the contrary it is clearly documented as requiring mmap_sem.

True, I forgot about that. I think at some point I had gup_fast
in there, I'll just switch back to that. Thanks for the kind note.

BTW looking over the tree I find at least one other caller that doesn't
hold it, like futex.c:fault_in_user_writeable. I'll send a separate
patch for t hat.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

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