diff mbox series

mm: add GFP_ATOMIC flag after local_lock_irqsave

Message ID 1625493432-9945-1-git-send-email-wangqing@vivo.com (mailing list archive)
State New
Headers show
Series mm: add GFP_ATOMIC flag after local_lock_irqsave | expand

Commit Message

王擎 July 5, 2021, 1:57 p.m. UTC 
Use GFP_ATOMIC when local_lock_irqsave in __alloc_pages_bulk

Reported-by: syzbot+e45919db2eab5e837646@syzkaller.appspotmail.com
Signed-off-by: Wang Qing <wangqing@vivo.com>
---
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Muchun Song July 6, 2021, 2:32 a.m. UTC | #1 
On Mon, Jul 5, 2021 at 9:57 PM Wang Qing <wangqing@vivo.com> wrote:
>
> Use GFP_ATOMIC when local_lock_irqsave in __alloc_pages_bulk
>
> Reported-by: syzbot+e45919db2eab5e837646@syzkaller.appspotmail.com
> Signed-off-by: Wang Qing <wangqing@vivo.com>
> ---
>  mm/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d6e94cc..3016ba5
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5309,7 +5309,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
>                 }
>                 nr_account++;
>
> -               prep_new_page(page, 0, gfp, 0);
> +               prep_new_page(page, 0, gfp | GFP_ATOMIC, 0);

Hi Wang Qing,

I didn't get the point here. IIUC, prep_new_page() will not allocate
memory. So why do we need GFP_ATOMIC? What I missed here?

Thanks.

>                 if (page_list)
>                         list_add(&page->lru, page_list);
>                 else
> --
> 2.7.4
>
王擎 July 6, 2021, 2:41 a.m. UTC | #2 
>On Mon, Jul 5, 2021 at 9:57 PM Wang Qing <wangqing@vivo.com> wrote:
>>
>> Use GFP_ATOMIC when local_lock_irqsave in __alloc_pages_bulk
>>
>> Reported-by: syzbot+e45919db2eab5e837646@syzkaller.appspotmail.com
>> Signed-off-by: Wang Qing <wangqing@vivo.com>
>> ---
>>  mm/page_alloc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index d6e94cc..3016ba5
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5309,7 +5309,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
>>                 }
>>                 nr_account++;
>>
>> -               prep_new_page(page, 0, gfp, 0);
>> +               prep_new_page(page, 0, gfp | GFP_ATOMIC, 0);
>
>Hi Wang Qing,
>
>I didn't get the point here. IIUC, prep_new_page() will not allocate
>memory. So why do we need GFP_ATOMIC? What I missed here?
>
>Thanks.

prep_new_page() will allocate memory in some scenarios. For details, 
you can check the bugs detected by syzkaller:
https://syzkaller.appspot.com/bug?id=91c2030241ada0e5d21877f8f2f44c98cffc04bb

Call Trace:
 __dump_stack lib/dump_stack.c:79 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:96
 ___might_sleep.cold+0x1f1/0x237 kernel/sched/core.c:9153
 prepare_alloc_pages+0x3da/0x580 mm/page_alloc.c:5179
 __alloc_pages+0x12f/0x500 mm/page_alloc.c:5375
 alloc_pages+0x18c/0x2a0 mm/mempolicy.c:2272
 stack_depot_save+0x39d/0x4e0 lib/stackdepot.c:303
 save_stack+0x15e/0x1e0 mm/page_owner.c:120
 __set_page_owner+0x50/0x290 mm/page_owner.c:181
 prep_new_page mm/page_alloc.c:2445 [inline]
 __alloc_pages_bulk+0x8b9/0x1870 mm/page_alloc.c:5313

Thanks.

Qing

>
>>                 if (page_list)
>>                         list_add(&page->lru, page_list);
>>                 else
>> --
>> 2.7.4
>>
Muchun Song July 6, 2021, 3:05 a.m. UTC | #3 
On Tue, Jul 6, 2021 at 10:41 AM 王擎 <wangqing@vivo.com> wrote:
>
>
> >On Mon, Jul 5, 2021 at 9:57 PM Wang Qing <wangqing@vivo.com> wrote:
> >>
> >> Use GFP_ATOMIC when local_lock_irqsave in __alloc_pages_bulk
> >>
> >> Reported-by: syzbot+e45919db2eab5e837646@syzkaller.appspotmail.com
> >> Signed-off-by: Wang Qing <wangqing@vivo.com>
> >> ---
> >>  mm/page_alloc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >> index d6e94cc..3016ba5
> >> --- a/mm/page_alloc.c
> >> +++ b/mm/page_alloc.c
> >> @@ -5309,7 +5309,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
> >>                 }
> >>                 nr_account++;
> >>
> >> -               prep_new_page(page, 0, gfp, 0);
> >> +               prep_new_page(page, 0, gfp | GFP_ATOMIC, 0);
> >
> >Hi Wang Qing,
> >
> >I didn't get the point here. IIUC, prep_new_page() will not allocate
> >memory. So why do we need GFP_ATOMIC? What I missed here?
> >
> >Thanks.
>
> prep_new_page() will allocate memory in some scenarios. For details,
> you can check the bugs detected by syzkaller:
> https://syzkaller.appspot.com/bug?id=91c2030241ada0e5d21877f8f2f44c98cffc04bb
>
> Call Trace:
>  __dump_stack lib/dump_stack.c:79 [inline]
>  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:96
>  ___might_sleep.cold+0x1f1/0x237 kernel/sched/core.c:9153
>  prepare_alloc_pages+0x3da/0x580 mm/page_alloc.c:5179
>  __alloc_pages+0x12f/0x500 mm/page_alloc.c:5375
>  alloc_pages+0x18c/0x2a0 mm/mempolicy.c:2272
>  stack_depot_save+0x39d/0x4e0 lib/stackdepot.c:303
>  save_stack+0x15e/0x1e0 mm/page_owner.c:120
>  __set_page_owner+0x50/0x290 mm/page_owner.c:181
>  prep_new_page mm/page_alloc.c:2445 [inline]
>  __alloc_pages_bulk+0x8b9/0x1870 mm/page_alloc.c:5313

Got it. But I don't think the fix you mentioned above was
appropriate. What if GFP_KERNEL | GFP_ATOMIC?

Thanks.

>
> Thanks.
>
> Qing
>
> >
> >>                 if (page_list)
> >>                         list_add(&page->lru, page_list);
> >>                 else
> >> --
> >> 2.7.4
> >>
>
>
王擎 July 6, 2021, 3:13 a.m. UTC | #4 
>On Tue, Jul 6, 2021 at 10:41 AM 王擎 <wangqing@vivo.com> wrote:
>>
>>
>> >On Mon, Jul 5, 2021 at 9:57 PM Wang Qing <wangqing@vivo.com> wrote:
>> >>
>> >> Use GFP_ATOMIC when local_lock_irqsave in __alloc_pages_bulk
>> >>
>> >> Reported-by: syzbot+e45919db2eab5e837646@syzkaller.appspotmail.com
>> >> Signed-off-by: Wang Qing <wangqing@vivo.com>
>> >> ---
>> >>  mm/page_alloc.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> >> index d6e94cc..3016ba5
>> >> --- a/mm/page_alloc.c
>> >> +++ b/mm/page_alloc.c
>> >> @@ -5309,7 +5309,7 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
>> >>                 }
>> >>                 nr_account++;
>> >>
>> >> -               prep_new_page(page, 0, gfp, 0);
>> >> +               prep_new_page(page, 0, gfp | GFP_ATOMIC, 0);
>> >
>> >Hi Wang Qing,
>> >
>> >I didn't get the point here. IIUC, prep_new_page() will not allocate
>> >memory. So why do we need GFP_ATOMIC? What I missed here?
>> >
>> >Thanks.
>>
>> prep_new_page() will allocate memory in some scenarios. For details,
>> you can check the bugs detected by syzkaller:
>> https://syzkaller.appspot.com/bug?id=91c2030241ada0e5d21877f8f2f44c98cffc04bb
>>
>> Call Trace:
>>  __dump_stack lib/dump_stack.c:79 [inline]
>>  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:96
>>  ___might_sleep.cold+0x1f1/0x237 kernel/sched/core.c:9153
>>  prepare_alloc_pages+0x3da/0x580 mm/page_alloc.c:5179
>>  __alloc_pages+0x12f/0x500 mm/page_alloc.c:5375
>>  alloc_pages+0x18c/0x2a0 mm/mempolicy.c:2272
>>  stack_depot_save+0x39d/0x4e0 lib/stackdepot.c:303
>>  save_stack+0x15e/0x1e0 mm/page_owner.c:120
>>  __set_page_owner+0x50/0x290 mm/page_owner.c:181
>>  prep_new_page mm/page_alloc.c:2445 [inline]
>>  __alloc_pages_bulk+0x8b9/0x1870 mm/page_alloc.c:5313
>
>Got it. But I don't think the fix you mentioned above was
>appropriate. What if GFP_KERNEL | GFP_ATOMIC?

Yes agree, but I haven't figured out what will happen this way, 
the test has been passed in syzkaller.
Or how about gfp | GFP_ATOMIC & ~GFP_KERNEL ?

Thanks,

Qing
>
>Thanks.
>
>>
>> Thanks.
>>
>> Qing
>>
>> >
>> >>                 if (page_list)
>> >>                         list_add(&page->lru, page_list);
>> >>                 else
>> >> --
>> >> 2.7.4
>> >>
>>
>>
diff mbox series

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d6e94cc..3016ba5
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5309,7 +5309,7 @@  unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
 		}
 		nr_account++;
 
-		prep_new_page(page, 0, gfp, 0);
+		prep_new_page(page, 0, gfp | GFP_ATOMIC, 0);
 		if (page_list)
 			list_add(&page->lru, page_list);
 		else