hwpoison madvise code

DISCUSSION Nick Piggin 2 participants 5 messages Dec 8, 2009 Memory management

lore

Share & tools

lore

Nick Piggin

author

2009-12-08 11:24 UTC #0 lore raw

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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andi Kleen

2009-12-08 11:26 UTC #1 lore raw

Quoted 7 lines · 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.

That wouldn't work.

Quoted 2 lines

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

Signature · 7 lines

[email protected] -- Speaking for myself only.

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

Nick Piggin

author

2009-12-08 11:50 UTC #2 lore raw

Quoted 10 lines · Andi Kleen

Quoted 7 lines · 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.
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.

Quoted 4 lines

Quoted 2 lines
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

Andi Kleen

2009-12-08 12:02 UTC #3 lore raw

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
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

Signature · 7 lines

[email protected] -- Speaking for myself only.

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

Nick Piggin

author

2009-12-08 12:07 UTC #4 lore raw

Quoted 9 lines · Andi Kleen

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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 :)

Quoted 3 lines

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.