Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()
Hi Zlatko, On Fri, Dec 28, 2012 at 02:29:11PM +0100, Zlatko Calusic wrote: > On 28.12.2012 03:49, Minchan Kim wrote: > >Hello Zlatko, > > > >On Fri, Dec 28, 2012 at 03:16:38AM +0100, Zlatko Calusic wrote: > >>From: Zlatko Calusic > >> > >>The unintended consequence of commit 4ae0a48b is that > >>wait_iff_congested() can now be called with NULL struct zone* > >>producing kernel oops like this: > > > >For good description, it would be better to write simple pseudo code > >flow to show how NULL-zone pass into wait_iff_congested because > >kswapd code flow is too complex. > > > >As I see the code, we have following line above wait_iff_congested. > > > >if (!unbalanced_zone || blah blah) > > break; > > > >How can NULL unbalanced_zone reach wait_iff_congested? > > > > Hello Minchan, and thanks for the comment. > > That line was there before commit 4ae0a48b got in, and you're right, Argh, I didn't see 4ae0a48b in 3.8-rc1. > it's what was protecting wait_iff_congested() from being called with > NULL zone*. But then all that logic got colapsed to a simple > pgdat_balanced() call and that's when I introduced the bug, I lost > the protection. > > What I _think_ is happening (pseudo code following...) is that after > scanning the zone in the dma->highmem direction, and concluding that > all zones are balanced (unbalanced_zone remains NULL!), > wake_up(>pfmemalloc_wait) wakes up a lot of memory hungry > processes (especially true in various aggressive test/benchmarks) > that immediately drain and unbalance one or more zones. Then > pgdat_balanced() call which immediately follows will be false, but > we still have unbalanced_zone = NULL, rememeber? Oops... > > But, all that is a speculation that I can't prove atm. Of course, if > anybody thinks that's a credible explanation, I could add it as a > commit comment, or even as a code comment, but I didn't want to be > overly imaginative. The fix itself is simple and real. Never mind. My confusing is caused my missing 4ae0a48b in lasest tree. Thanks, Zlatko. > > Regards, > -- > Zlatko > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majord...@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: mailto:"d...@kvack.org;> em...@kvack.org -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()
Hi Zlatko, On Fri, Dec 28, 2012 at 02:29:11PM +0100, Zlatko Calusic wrote: On 28.12.2012 03:49, Minchan Kim wrote: Hello Zlatko, On Fri, Dec 28, 2012 at 03:16:38AM +0100, Zlatko Calusic wrote: From: Zlatko Calusic zlatko.calu...@iskon.hr The unintended consequence of commit 4ae0a48b is that wait_iff_congested() can now be called with NULL struct zone* producing kernel oops like this: For good description, it would be better to write simple pseudo code flow to show how NULL-zone pass into wait_iff_congested because kswapd code flow is too complex. As I see the code, we have following line above wait_iff_congested. if (!unbalanced_zone || blah blah) break; How can NULL unbalanced_zone reach wait_iff_congested? Hello Minchan, and thanks for the comment. That line was there before commit 4ae0a48b got in, and you're right, Argh, I didn't see 4ae0a48b in 3.8-rc1. it's what was protecting wait_iff_congested() from being called with NULL zone*. But then all that logic got colapsed to a simple pgdat_balanced() call and that's when I introduced the bug, I lost the protection. What I _think_ is happening (pseudo code following...) is that after scanning the zone in the dma-highmem direction, and concluding that all zones are balanced (unbalanced_zone remains NULL!), wake_up(pgdat-pfmemalloc_wait) wakes up a lot of memory hungry processes (especially true in various aggressive test/benchmarks) that immediately drain and unbalance one or more zones. Then pgdat_balanced() call which immediately follows will be false, but we still have unbalanced_zone = NULL, rememeber? Oops... But, all that is a speculation that I can't prove atm. Of course, if anybody thinks that's a credible explanation, I could add it as a commit comment, or even as a code comment, but I didn't want to be overly imaginative. The fix itself is simple and real. Never mind. My confusing is caused my missing 4ae0a48b in lasest tree. Thanks, Zlatko. Regards, -- Zlatko -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majord...@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: a href=mailto:d...@kvack.org; em...@kvack.org /a -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()
Just FYI:
This patch landed upstream [1].
Thanks for all involved people.
- Sedat -
[1]
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=ecccd1248d6e6986130ffcc3b0d003cb46a485c0
On Fri, Dec 28, 2012 at 3:16 AM, Zlatko Calusic wrote:
> From: Zlatko Calusic
>
> The unintended consequence of commit 4ae0a48b is that
> wait_iff_congested() can now be called with NULL struct zone*
> producing kernel oops like this:
>
> BUG: unable to handle kernel NULL pointer dereference
> IP: [] wait_iff_congested+0x59/0x140
>
> This trivial patch fixes it.
>
> Reported-by: Zhouping Liu
> Reported-and-tested-by: Sedat Dilek
> Cc: Andrew Morton
> Cc: Mel Gorman
> Cc: Hugh Dickins
> Signed-off-by: Zlatko Calusic
> ---
> mm/vmscan.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 02bcfa3..e55ce55 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2782,7 +2782,7 @@ loop_again:
> if (total_scanned && (sc.priority < DEF_PRIORITY - 2)) {
> if (has_under_min_watermark_zone)
> count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
> - else
> + else if (unbalanced_zone)
> wait_iff_congested(unbalanced_zone,
> BLK_RW_ASYNC, HZ/10);
> }
>
> --
> 1.8.1.rc3
>
> --
> Zlatko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()
Just FYI:
This patch landed upstream [1].
Thanks for all involved people.
- Sedat -
[1]
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=ecccd1248d6e6986130ffcc3b0d003cb46a485c0
On Fri, Dec 28, 2012 at 3:16 AM, Zlatko Calusic zlatko.calu...@iskon.hr wrote:
From: Zlatko Calusic zlatko.calu...@iskon.hr
The unintended consequence of commit 4ae0a48b is that
wait_iff_congested() can now be called with NULL struct zone*
producing kernel oops like this:
BUG: unable to handle kernel NULL pointer dereference
IP: [811542d9] wait_iff_congested+0x59/0x140
This trivial patch fixes it.
Reported-by: Zhouping Liu z...@redhat.com
Reported-and-tested-by: Sedat Dilek sedat.di...@gmail.com
Cc: Andrew Morton a...@linux-foundation.org
Cc: Mel Gorman mgor...@suse.de
Cc: Hugh Dickins hu...@google.com
Signed-off-by: Zlatko Calusic zlatko.calu...@iskon.hr
---
mm/vmscan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 02bcfa3..e55ce55 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2782,7 +2782,7 @@ loop_again:
if (total_scanned (sc.priority DEF_PRIORITY - 2)) {
if (has_under_min_watermark_zone)
count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
- else
+ else if (unbalanced_zone)
wait_iff_congested(unbalanced_zone,
BLK_RW_ASYNC, HZ/10);
}
--
1.8.1.rc3
--
Zlatko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()
On 28.12.2012 03:49, Minchan Kim wrote: Hello Zlatko, On Fri, Dec 28, 2012 at 03:16:38AM +0100, Zlatko Calusic wrote: From: Zlatko Calusic The unintended consequence of commit 4ae0a48b is that wait_iff_congested() can now be called with NULL struct zone* producing kernel oops like this: For good description, it would be better to write simple pseudo code flow to show how NULL-zone pass into wait_iff_congested because kswapd code flow is too complex. As I see the code, we have following line above wait_iff_congested. if (!unbalanced_zone || blah blah) break; How can NULL unbalanced_zone reach wait_iff_congested? Hello Minchan, and thanks for the comment. That line was there before commit 4ae0a48b got in, and you're right, it's what was protecting wait_iff_congested() from being called with NULL zone*. But then all that logic got colapsed to a simple pgdat_balanced() call and that's when I introduced the bug, I lost the protection. What I _think_ is happening (pseudo code following...) is that after scanning the zone in the dma->highmem direction, and concluding that all zones are balanced (unbalanced_zone remains NULL!), wake_up(>pfmemalloc_wait) wakes up a lot of memory hungry processes (especially true in various aggressive test/benchmarks) that immediately drain and unbalance one or more zones. Then pgdat_balanced() call which immediately follows will be false, but we still have unbalanced_zone = NULL, rememeber? Oops... But, all that is a speculation that I can't prove atm. Of course, if anybody thinks that's a credible explanation, I could add it as a commit comment, or even as a code comment, but I didn't want to be overly imaginative. The fix itself is simple and real. Regards, -- Zlatko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()
On 28.12.2012 03:49, Minchan Kim wrote: Hello Zlatko, On Fri, Dec 28, 2012 at 03:16:38AM +0100, Zlatko Calusic wrote: From: Zlatko Calusic zlatko.calu...@iskon.hr The unintended consequence of commit 4ae0a48b is that wait_iff_congested() can now be called with NULL struct zone* producing kernel oops like this: For good description, it would be better to write simple pseudo code flow to show how NULL-zone pass into wait_iff_congested because kswapd code flow is too complex. As I see the code, we have following line above wait_iff_congested. if (!unbalanced_zone || blah blah) break; How can NULL unbalanced_zone reach wait_iff_congested? Hello Minchan, and thanks for the comment. That line was there before commit 4ae0a48b got in, and you're right, it's what was protecting wait_iff_congested() from being called with NULL zone*. But then all that logic got colapsed to a simple pgdat_balanced() call and that's when I introduced the bug, I lost the protection. What I _think_ is happening (pseudo code following...) is that after scanning the zone in the dma-highmem direction, and concluding that all zones are balanced (unbalanced_zone remains NULL!), wake_up(pgdat-pfmemalloc_wait) wakes up a lot of memory hungry processes (especially true in various aggressive test/benchmarks) that immediately drain and unbalance one or more zones. Then pgdat_balanced() call which immediately follows will be false, but we still have unbalanced_zone = NULL, rememeber? Oops... But, all that is a speculation that I can't prove atm. Of course, if anybody thinks that's a credible explanation, I could add it as a commit comment, or even as a code comment, but I didn't want to be overly imaginative. The fix itself is simple and real. Regards, -- Zlatko -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()
Hello Zlatko,
On Fri, Dec 28, 2012 at 03:16:38AM +0100, Zlatko Calusic wrote:
> From: Zlatko Calusic
>
> The unintended consequence of commit 4ae0a48b is that
> wait_iff_congested() can now be called with NULL struct zone*
> producing kernel oops like this:
For good description, it would be better to write simple pseudo code
flow to show how NULL-zone pass into wait_iff_congested because
kswapd code flow is too complex.
As I see the code, we have following line above wait_iff_congested.
if (!unbalanced_zone || blah blah)
break;
How can NULL unbalanced_zone reach wait_iff_congested?
>
> BUG: unable to handle kernel NULL pointer dereference
> IP: [] wait_iff_congested+0x59/0x140
>
> This trivial patch fixes it.
>
> Reported-by: Zhouping Liu
> Reported-and-tested-by: Sedat Dilek
> Cc: Andrew Morton
> Cc: Mel Gorman
> Cc: Hugh Dickins
> Signed-off-by: Zlatko Calusic
> ---
> mm/vmscan.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 02bcfa3..e55ce55 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2782,7 +2782,7 @@ loop_again:
> if (total_scanned && (sc.priority < DEF_PRIORITY - 2)) {
> if (has_under_min_watermark_zone)
> count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
> - else
> + else if (unbalanced_zone)
> wait_iff_congested(unbalanced_zone,
> BLK_RW_ASYNC, HZ/10);
> }
>
> --
> 1.8.1.rc3
>
> --
> Zlatko
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mm: fix null pointer dereference in wait_iff_congested()
From: Zlatko Calusic
The unintended consequence of commit 4ae0a48b is that
wait_iff_congested() can now be called with NULL struct zone*
producing kernel oops like this:
BUG: unable to handle kernel NULL pointer dereference
IP: [] wait_iff_congested+0x59/0x140
This trivial patch fixes it.
Reported-by: Zhouping Liu
Reported-and-tested-by: Sedat Dilek
Cc: Andrew Morton
Cc: Mel Gorman
Cc: Hugh Dickins
Signed-off-by: Zlatko Calusic
---
mm/vmscan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 02bcfa3..e55ce55 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2782,7 +2782,7 @@ loop_again:
if (total_scanned && (sc.priority < DEF_PRIORITY - 2)) {
if (has_under_min_watermark_zone)
count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
- else
+ else if (unbalanced_zone)
wait_iff_congested(unbalanced_zone,
BLK_RW_ASYNC, HZ/10);
}
--
1.8.1.rc3
--
Zlatko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mm: fix null pointer dereference in wait_iff_congested()
From: Zlatko Calusic zlatko.calu...@iskon.hr
The unintended consequence of commit 4ae0a48b is that
wait_iff_congested() can now be called with NULL struct zone*
producing kernel oops like this:
BUG: unable to handle kernel NULL pointer dereference
IP: [811542d9] wait_iff_congested+0x59/0x140
This trivial patch fixes it.
Reported-by: Zhouping Liu z...@redhat.com
Reported-and-tested-by: Sedat Dilek sedat.di...@gmail.com
Cc: Andrew Morton a...@linux-foundation.org
Cc: Mel Gorman mgor...@suse.de
Cc: Hugh Dickins hu...@google.com
Signed-off-by: Zlatko Calusic zlatko.calu...@iskon.hr
---
mm/vmscan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 02bcfa3..e55ce55 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2782,7 +2782,7 @@ loop_again:
if (total_scanned (sc.priority DEF_PRIORITY - 2)) {
if (has_under_min_watermark_zone)
count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
- else
+ else if (unbalanced_zone)
wait_iff_congested(unbalanced_zone,
BLK_RW_ASYNC, HZ/10);
}
--
1.8.1.rc3
--
Zlatko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mm: fix null pointer dereference in wait_iff_congested()
Hello Zlatko,
On Fri, Dec 28, 2012 at 03:16:38AM +0100, Zlatko Calusic wrote:
From: Zlatko Calusic zlatko.calu...@iskon.hr
The unintended consequence of commit 4ae0a48b is that
wait_iff_congested() can now be called with NULL struct zone*
producing kernel oops like this:
For good description, it would be better to write simple pseudo code
flow to show how NULL-zone pass into wait_iff_congested because
kswapd code flow is too complex.
As I see the code, we have following line above wait_iff_congested.
if (!unbalanced_zone || blah blah)
break;
How can NULL unbalanced_zone reach wait_iff_congested?
BUG: unable to handle kernel NULL pointer dereference
IP: [811542d9] wait_iff_congested+0x59/0x140
This trivial patch fixes it.
Reported-by: Zhouping Liu z...@redhat.com
Reported-and-tested-by: Sedat Dilek sedat.di...@gmail.com
Cc: Andrew Morton a...@linux-foundation.org
Cc: Mel Gorman mgor...@suse.de
Cc: Hugh Dickins hu...@google.com
Signed-off-by: Zlatko Calusic zlatko.calu...@iskon.hr
---
mm/vmscan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 02bcfa3..e55ce55 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2782,7 +2782,7 @@ loop_again:
if (total_scanned (sc.priority DEF_PRIORITY - 2)) {
if (has_under_min_watermark_zone)
count_vm_event(KSWAPD_SKIP_CONGESTION_WAIT);
- else
+ else if (unbalanced_zone)
wait_iff_congested(unbalanced_zone,
BLK_RW_ASYNC, HZ/10);
}
--
1.8.1.rc3
--
Zlatko
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majord...@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: a href=mailto:d...@kvack.org; em...@kvack.org /a
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
