* [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
@ 2019-09-19 9:18 Xiaoming Ni
2019-09-19 9:29 ` Greg KH
0 siblings, 1 reply; 10+ messages in thread
From: Xiaoming Ni @ 2019-09-19 9:18 UTC (permalink / raw)
To: penberg, gregkh, jslaby
Cc: nico, textshell, sam, daniel.vetter, mpatocka, ghalat,
linux-kernel, yangyingliang, yuehaibing, zengweilin
Using kzalloc() to allocate memory in function con_init(), but not
checking the return value, there is a risk of null pointer references
oops.
Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
drivers/tty/vt/vt.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 34aa39d..db83e52 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3357,15 +3357,33 @@ static int __init con_init(void)
for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
+ if (unlikely(!vc)) {
+ pr_warn("%s:failed to allocate memory for the %u vc\n",
+ __func__, currcons);
+ break;
+ }
INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
tty_port_init(&vc->port);
visual_init(vc, currcons, 1);
vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
+ if (unlikely(!vc->vc_screenbuf)) {
+ pr_warn("%s:failed to allocate memory for the %u vc_screenbuf\n",
+ __func__, currcons);
+ visual_deinit(vc);
+ tty_port_destroy(&vc->port);
+ kfree(vc);
+ vc_cons[currcons].d = NULL;
+ break;
+ }
vc_init(vc, vc->vc_rows, vc->vc_cols,
currcons || !vc->vc_sw->con_save_screen);
}
currcons = fg_console = 0;
master_display_fg = vc = vc_cons[currcons].d;
+ if (unlikely(!vc)) {
+ console_unlock();
+ return 0;
+ }
set_origin(vc);
save_screen(vc);
gotoxy(vc, vc->vc_x, vc->vc_y);
--
1.8.5.6
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
2019-09-19 9:18 [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops Xiaoming Ni
@ 2019-09-19 9:29 ` Greg KH
2019-09-19 15:16 ` Mikulas Patocka
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Greg KH @ 2019-09-19 9:29 UTC (permalink / raw)
To: Xiaoming Ni
Cc: penberg, jslaby, nico, textshell, sam, daniel.vetter, mpatocka,
ghalat, linux-kernel, yangyingliang, yuehaibing, zengweilin
On Thu, Sep 19, 2019 at 05:18:15PM +0800, Xiaoming Ni wrote:
> Using kzalloc() to allocate memory in function con_init(), but not
> checking the return value, there is a risk of null pointer references
> oops.
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
We keep having this be "reported" :(
> ---
> drivers/tty/vt/vt.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 34aa39d..db83e52 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3357,15 +3357,33 @@ static int __init con_init(void)
>
> for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> + if (unlikely(!vc)) {
> + pr_warn("%s:failed to allocate memory for the %u vc\n",
> + __func__, currcons);
> + break;
> + }
At init, this really can not happen. Have you see it ever happen?
> INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> tty_port_init(&vc->port);
> visual_init(vc, currcons, 1);
> vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> + if (unlikely(!vc->vc_screenbuf)) {
Never use likely/unlikely unless you can actually measure the speed
difference. For something like this, the compiler will always get it
right without you having to do anything.
And again, how can this fail? Have you seen it fail?
> + pr_warn("%s:failed to allocate memory for the %u vc_screenbuf\n",
> + __func__, currcons);
> + visual_deinit(vc);
> + tty_port_destroy(&vc->port);
> + kfree(vc);
> + vc_cons[currcons].d = NULL;
> + break;
> + }
> vc_init(vc, vc->vc_rows, vc->vc_cols,
> currcons || !vc->vc_sw->con_save_screen);
> }
> currcons = fg_console = 0;
> master_display_fg = vc = vc_cons[currcons].d;
> + if (unlikely(!vc)) {
Again, never use likely/unlikely unless you can measure it.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
2019-09-19 9:29 ` Greg KH
@ 2019-09-19 15:16 ` Mikulas Patocka
2019-09-20 2:29 ` Nixiaoming
2019-09-20 2:56 ` Nicolas Pitre
2 siblings, 0 replies; 10+ messages in thread
From: Mikulas Patocka @ 2019-09-19 15:16 UTC (permalink / raw)
To: Greg KH
Cc: Xiaoming Ni, penberg, jslaby, nico, textshell, sam,
daniel.vetter, ghalat, linux-kernel, yangyingliang, yuehaibing,
zengweilin
On Thu, 19 Sep 2019, Greg KH wrote:
> On Thu, Sep 19, 2019 at 05:18:15PM +0800, Xiaoming Ni wrote:
> > Using kzalloc() to allocate memory in function con_init(), but not
> > checking the return value, there is a risk of null pointer references
> > oops.
> >
> > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>
> We keep having this be "reported" :(
>
> > ---
> > drivers/tty/vt/vt.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 34aa39d..db83e52 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -3357,15 +3357,33 @@ static int __init con_init(void)
> >
> > for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > + if (unlikely(!vc)) {
> > + pr_warn("%s:failed to allocate memory for the %u vc\n",
> > + __func__, currcons);
> > + break;
> > + }
>
> At init, this really can not happen. Have you see it ever happen?
>
> > INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> > tty_port_init(&vc->port);
> > visual_init(vc, currcons, 1);
> > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > + if (unlikely(!vc->vc_screenbuf)) {
>
> Never use likely/unlikely unless you can actually measure the speed
> difference. For something like this, the compiler will always get it
> right without you having to do anything.
>
> And again, how can this fail? Have you seen it fail?
>
> > + pr_warn("%s:failed to allocate memory for the %u vc_screenbuf\n",
> > + __func__, currcons);
> > + visual_deinit(vc);
> > + tty_port_destroy(&vc->port);
> > + kfree(vc);
> > + vc_cons[currcons].d = NULL;
> > + break;
> > + }
> > vc_init(vc, vc->vc_rows, vc->vc_cols,
> > currcons || !vc->vc_sw->con_save_screen);
> > }
> > currcons = fg_console = 0;
> > master_display_fg = vc = vc_cons[currcons].d;
> > + if (unlikely(!vc)) {
>
> Again, never use likely/unlikely unless you can measure it.
>
> thanks,
>
> greg k-h
Why does it use GFP_NOWAIT and not GFP_KERNEL? Is there some problem with
GFP_KERNEL during initialization?
Mikulas
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
2019-09-19 9:29 ` Greg KH
2019-09-19 15:16 ` Mikulas Patocka
@ 2019-09-20 2:29 ` Nixiaoming
2019-09-20 6:04 ` Greg KH
2019-09-20 2:56 ` Nicolas Pitre
2 siblings, 1 reply; 10+ messages in thread
From: Nixiaoming @ 2019-09-20 2:29 UTC (permalink / raw)
To: Greg KH
Cc: penberg, jslaby, nico, textshell, sam, daniel.vetter, mpatocka,
ghalat, linux-kernel, Yangyingliang, yuehaibing, Zengweilin
On 2019/9/19 17:30, Greg KH wrote:
> On Thu, Sep 19, 2019 at 05:18:15PM +0800, Xiaoming Ni wrote:
>> Using kzalloc() to allocate memory in function con_init(), but not
>> checking the return value, there is a risk of null pointer references
>> oops.
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>
> We keep having this be "reported"
>
>> ---
>> drivers/tty/vt/vt.c | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
>> index 34aa39d..db83e52 100644
>> --- a/drivers/tty/vt/vt.c
>> +++ b/drivers/tty/vt/vt.c
>> @@ -3357,15 +3357,33 @@ static int __init con_init(void)
>>
>> for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
>> vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
>> + if (unlikely(!vc)) {
>> + pr_warn("%s:failed to allocate memory for the %u vc\n",
>> + __func__, currcons);
>> + break;
>> + }
>
> At init, this really can not happen. Have you see it ever happen?
I did not actually observe the null pointer here.
I am confused when I see the code allocated here without check the return value.
Small memory allocation failures are difficult to occur during system initialization
But is it not safe enough if the code is not judged?
Also if the memory allocation failure is not allowed here, is it better to add the __GFP_NOFAIL flags?
>> INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
>> tty_port_init(&vc->port);
>> visual_init(vc, currcons, 1);
>> vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
>> + if (unlikely(!vc->vc_screenbuf)) {
>
> Never use likely/unlikely unless you can actually measure the speed
> difference. For something like this, the compiler will always get it
> right without you having to do anything.
>
Thank you for your guidance.
I misuse it.
> And again, how can this fail? Have you seen it fail?
>
>> + pr_warn("%s:failed to allocate memory for the %u vc_screenbuf\n",
>> + __func__, currcons);
>> + visual_deinit(vc);
>> + tty_port_destroy(&vc->port);
>> + kfree(vc);
>> + vc_cons[currcons].d = NULL;
>> + break;
>> + }
>> vc_init(vc, vc->vc_rows, vc->vc_cols,
>> currcons || !vc->vc_sw->con_save_screen);
>> }
>> currcons = fg_console = 0;
>> master_display_fg = vc = vc_cons[currcons].d;
>> + if (unlikely(!vc)) {
>
> Again, never use likely/unlikely unless you can measure it.
>
> thanks,
>
> greg k-h
>
> .
>
thanks,
Xiaoming Ni
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
2019-09-19 9:29 ` Greg KH
2019-09-19 15:16 ` Mikulas Patocka
2019-09-20 2:29 ` Nixiaoming
@ 2019-09-20 2:56 ` Nicolas Pitre
2019-09-20 6:04 ` Greg KH
2 siblings, 1 reply; 10+ messages in thread
From: Nicolas Pitre @ 2019-09-20 2:56 UTC (permalink / raw)
To: Greg KH
Cc: Xiaoming Ni, penberg, jslaby, textshell, sam, daniel.vetter,
mpatocka, ghalat, linux-kernel, yangyingliang, yuehaibing,
zengweilin
On Thu, 19 Sep 2019, Greg KH wrote:
> On Thu, Sep 19, 2019 at 05:18:15PM +0800, Xiaoming Ni wrote:
> > Using kzalloc() to allocate memory in function con_init(), but not
> > checking the return value, there is a risk of null pointer references
> > oops.
> >
> > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>
> We keep having this be "reported" :(
Something probably needs to be "communicated" about that.
> > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > + if (unlikely(!vc)) {
> > + pr_warn("%s:failed to allocate memory for the %u vc\n",
> > + __func__, currcons);
> > + break;
> > + }
>
> At init, this really can not happen. Have you see it ever happen?
This is maybe too subtle a fact. The "communication" could be done with
some GFP_WONTFAIL flag, and have the allocator simply pannic() if it
ever fails.
Nicolas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
2019-09-20 2:56 ` Nicolas Pitre
@ 2019-09-20 6:04 ` Greg KH
2019-09-21 7:26 ` Xiaoming Ni
0 siblings, 1 reply; 10+ messages in thread
From: Greg KH @ 2019-09-20 6:04 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Xiaoming Ni, penberg, jslaby, textshell, sam, daniel.vetter,
mpatocka, ghalat, linux-kernel, yangyingliang, yuehaibing,
zengweilin
On Thu, Sep 19, 2019 at 10:56:15PM -0400, Nicolas Pitre wrote:
> On Thu, 19 Sep 2019, Greg KH wrote:
>
> > On Thu, Sep 19, 2019 at 05:18:15PM +0800, Xiaoming Ni wrote:
> > > Using kzalloc() to allocate memory in function con_init(), but not
> > > checking the return value, there is a risk of null pointer references
> > > oops.
> > >
> > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> >
> > We keep having this be "reported" :(
>
> Something probably needs to be "communicated" about that.
I know, but it's also kind of fun to see what these "automated" checkers
find, sometimes the resulting patches almost work properly :)
This one is really close, I think if the likely/unlikely gets cleaned
up, it is viable.
> > > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > > + if (unlikely(!vc)) {
> > > + pr_warn("%s:failed to allocate memory for the %u vc\n",
> > > + __func__, currcons);
> > > + break;
> > > + }
> >
> > At init, this really can not happen. Have you see it ever happen?
>
> This is maybe too subtle a fact. The "communication" could be done with
> some GFP_WONTFAIL flag, and have the allocator simply pannic() if it
> ever fails.
That's a good idea to do as well.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
2019-09-20 2:29 ` Nixiaoming
@ 2019-09-20 6:04 ` Greg KH
0 siblings, 0 replies; 10+ messages in thread
From: Greg KH @ 2019-09-20 6:04 UTC (permalink / raw)
To: Nixiaoming
Cc: penberg, jslaby, nico, textshell, sam, daniel.vetter, mpatocka,
ghalat, linux-kernel, Yangyingliang, yuehaibing, Zengweilin
On Fri, Sep 20, 2019 at 02:29:49AM +0000, Nixiaoming wrote:
> On 2019/9/19 17:30, Greg KH wrote:
> > On Thu, Sep 19, 2019 at 05:18:15PM +0800, Xiaoming Ni wrote:
> >> Using kzalloc() to allocate memory in function con_init(), but not
> >> checking the return value, there is a risk of null pointer references
> >> oops.
> >>
> >> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> >
> > We keep having this be "reported"
> >
> >> ---
> >> drivers/tty/vt/vt.c | 18 ++++++++++++++++++
> >> 1 file changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> >> index 34aa39d..db83e52 100644
> >> --- a/drivers/tty/vt/vt.c
> >> +++ b/drivers/tty/vt/vt.c
> >> @@ -3357,15 +3357,33 @@ static int __init con_init(void)
> >>
> >> for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> >> vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> >> + if (unlikely(!vc)) {
> >> + pr_warn("%s:failed to allocate memory for the %u vc\n",
> >> + __func__, currcons);
> >> + break;
> >> + }
> >
> > At init, this really can not happen. Have you see it ever happen?
>
> I did not actually observe the null pointer here.
> I am confused when I see the code allocated here without check the return value.
> Small memory allocation failures are difficult to occur during system initialization
> But is it not safe enough if the code is not judged?
> Also if the memory allocation failure is not allowed here, is it better to add the __GFP_NOFAIL flags?
See my response to Nicolas, but yes, that would be a good way to handle
this.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
2019-09-20 6:04 ` Greg KH
@ 2019-09-21 7:26 ` Xiaoming Ni
2019-09-23 3:50 ` Nicolas Pitre
0 siblings, 1 reply; 10+ messages in thread
From: Xiaoming Ni @ 2019-09-21 7:26 UTC (permalink / raw)
To: Greg KH, Nicolas Pitre
Cc: penberg, jslaby, textshell, sam, daniel.vetter, mpatocka, ghalat,
linux-kernel, yangyingliang, yuehaibing, zengweilin
On 2019/9/20 14:04, Greg KH wrote:
> On Thu, Sep 19, 2019 at 10:56:15PM -0400, Nicolas Pitre wrote:
>> On Thu, 19 Sep 2019, Greg KH wrote:
>>
>>> On Thu, Sep 19, 2019 at 05:18:15PM +0800, Xiaoming Ni wrote:
>>>> Using kzalloc() to allocate memory in function con_init(), but not
>>>> checking the return value, there is a risk of null pointer references
>>>> oops.
>>>>
>>>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>>>
>>> We keep having this be "reported" :(
>>
>> Something probably needs to be "communicated" about that.
>
> I know, but it's also kind of fun to see what these "automated" checkers
> find, sometimes the resulting patches almost work properly :)
>
> This one is really close, I think if the likely/unlikely gets cleaned
> up, it is viable.
>
>>>> vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
>>>> + if (unlikely(!vc)) {
>>>> + pr_warn("%s:failed to allocate memory for the %u vc\n",
>>>> + __func__, currcons);
>>>> + break;
>>>> + }
>>>
>>> At init, this really can not happen. Have you see it ever happen?
>>
>> This is maybe too subtle a fact. The "communication" could be done with
>> some GFP_WONTFAIL flag, and have the allocator simply pannic() if it
>> ever fails.
>
> That's a good idea to do as well.
>
> thanks,
>
> greg k-h
>
> .
>
Thank you for your advice.
@ Nicolas Pitre
Can I make a v2 patch based on your advice ?
Or you will submit a patch for "GFP_WONTFAIL" yourself ?
thanks
Xiaoming Ni
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
2019-09-21 7:26 ` Xiaoming Ni
@ 2019-09-23 3:50 ` Nicolas Pitre
2019-09-25 8:37 ` Xiaoming Ni
0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Pitre @ 2019-09-23 3:50 UTC (permalink / raw)
To: Xiaoming Ni
Cc: Greg KH, penberg, jslaby, textshell, sam, daniel.vetter,
mpatocka, ghalat, linux-kernel, yangyingliang, yuehaibing,
zengweilin
On Sat, 21 Sep 2019, Xiaoming Ni wrote:
> @ Nicolas Pitre
> Can I make a v2 patch based on your advice ?
> Or you will submit a patch for "GFP_WONTFAIL" yourself ?
Here's a patch implementing what I had in mind. This is compile tested
only.
----- >8
Subject: [PATCH] mm: add __GFP_WONTFAIL and GFP_ONBOOT
Some memory allocations are very unlikely to fail during system boot.
Because of that, the code often doesn't bother to check for allocation
failure, but this gets reported anyway.
As an alternative to adding code to check for NULL that has almost no
chance of ever being exercised, let's use a GFP flag to identify those
cases and panic the kernel if allocation failure ever occurs.
Conversion of one such instance is also included.
Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 34aa39d1ae..bd0a0e4807 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3356,7 +3356,7 @@ static int __init con_init(void)
}
for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
- vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
+ vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_ONBOOT);
INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
tty_port_init(&vc->port);
visual_init(vc, currcons, 1);
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index f33881688f..6f33575cd6 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -39,8 +39,9 @@ struct vm_area_struct;
#define ___GFP_HARDWALL 0x100000u
#define ___GFP_THISNODE 0x200000u
#define ___GFP_ACCOUNT 0x400000u
+#define ___GFP_WONTFAIL 0x800000u
#ifdef CONFIG_LOCKDEP
-#define ___GFP_NOLOCKDEP 0x800000u
+#define ___GFP_NOLOCKDEP 0x1000000u
#else
#define ___GFP_NOLOCKDEP 0
#endif
@@ -187,6 +188,12 @@ struct vm_area_struct;
* definitely preferable to use the flag rather than opencode endless
* loop around allocator.
* Using this flag for costly allocations is _highly_ discouraged.
+ *
+ * %__GFP_WONTFAIL: No allocation error is expected what so ever. The
+ * caller presumes allocation will always succeed (e.g. the system is still
+ * booting, the allocation size is relatively small and memory should be
+ * plentiful) so testing for failure is skipped. If allocation ever fails
+ * then the kernel will simply panic.
*/
#define __GFP_IO ((__force gfp_t)___GFP_IO)
#define __GFP_FS ((__force gfp_t)___GFP_FS)
@@ -196,6 +203,7 @@ struct vm_area_struct;
#define __GFP_RETRY_MAYFAIL ((__force gfp_t)___GFP_RETRY_MAYFAIL)
#define __GFP_NOFAIL ((__force gfp_t)___GFP_NOFAIL)
#define __GFP_NORETRY ((__force gfp_t)___GFP_NORETRY)
+#define __GFP_WONTFAIL ((__force gfp_t)___GFP_WONTFAIL)
/**
* DOC: Action modifiers
@@ -217,7 +225,7 @@ struct vm_area_struct;
#define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
/* Room for N __GFP_FOO bits */
-#define __GFP_BITS_SHIFT (23 + IS_ENABLED(CONFIG_LOCKDEP))
+#define __GFP_BITS_SHIFT (24 + IS_ENABLED(CONFIG_LOCKDEP))
#define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
/**
@@ -285,6 +293,9 @@ struct vm_area_struct;
* available and will not wake kswapd/kcompactd on failure. The _LIGHT
* version does not attempt reclaim/compaction at all and is by default used
* in page fault path, while the non-light is used by khugepaged.
+ *
+ * %GFP_ONBOOT is for relatively small allocations that are not expected
+ * to fail while the system is booting.
*/
#define GFP_ATOMIC (__GFP_HIGH|__GFP_ATOMIC|__GFP_KSWAPD_RECLAIM)
#define GFP_KERNEL (__GFP_RECLAIM | __GFP_IO | __GFP_FS)
@@ -300,6 +311,7 @@ struct vm_area_struct;
#define GFP_TRANSHUGE_LIGHT ((GFP_HIGHUSER_MOVABLE | __GFP_COMP | \
__GFP_NOMEMALLOC | __GFP_NOWARN) & ~__GFP_RECLAIM)
#define GFP_TRANSHUGE (GFP_TRANSHUGE_LIGHT | __GFP_DIRECT_RECLAIM)
+#define GFP_ONBOOT (GFP_NOWAIT | __GFP_WONTFAIL)
/* Convert GFP flags to their corresponding migrate type */
#define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ff5484fdbd..36dee09f7f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4625,6 +4625,14 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
fail:
warn_alloc(gfp_mask, ac->nodemask,
"page allocation failure: order:%u", order);
+ if (gfp_mask & __GFP_WONTFAIL) {
+ /*
+ * The assumption was wrong. This is never supposed to happen.
+ * Caller most likely won't check for a returned NULL either.
+ * So the only reasonable thing to do is to pannic.
+ */
+ panic("Failed to allocate memory despite GFP_WONTFAIL\n");
+ }
got_pg:
return page;
}
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops
2019-09-23 3:50 ` Nicolas Pitre
@ 2019-09-25 8:37 ` Xiaoming Ni
0 siblings, 0 replies; 10+ messages in thread
From: Xiaoming Ni @ 2019-09-25 8:37 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Greg KH, penberg, jslaby, textshell, sam, daniel.vetter,
mpatocka, ghalat, linux-kernel, yangyingliang, yuehaibing,
zengweilin
On 2019/9/23 11:50, Nicolas Pitre wrote:
> On Sat, 21 Sep 2019, Xiaoming Ni wrote:
>
>> @ Nicolas Pitre
>> Can I make a v2 patch based on your advice ?
>> Or you will submit a patch for "GFP_WONTFAIL" yourself ?
>
> Here's a patch implementing what I had in mind. This is compile tested
> only.
>
> ----- >8
>
> Subject: [PATCH] mm: add __GFP_WONTFAIL and GFP_ONBOOT
>
> Some memory allocations are very unlikely to fail during system boot.
> Because of that, the code often doesn't bother to check for allocation
> failure, but this gets reported anyway.
>
> As an alternative to adding code to check for NULL that has almost no
> chance of ever being exercised, let's use a GFP flag to identify those
> cases and panic the kernel if allocation failure ever occurs.
>
> Conversion of one such instance is also included.
>
> Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
>
.....
....
> /**
> @@ -285,6 +293,9 @@ struct vm_area_struct;
> * available and will not wake kswapd/kcompactd on failure. The _LIGHT
> * version does not attempt reclaim/compaction at all and is by default used
> * in page fault path, while the non-light is used by khugepaged.
> + *
> + * %GFP_ONBOOT is for relatively small allocations that are not expected
> + * to fail while the system is booting.
> */
> #define GFP_ATOMIC (__GFP_HIGH|__GFP_ATOMIC|__GFP_KSWAPD_RECLAIM)
> #define GFP_KERNEL (__GFP_RECLAIM | __GFP_IO | __GFP_FS)
> @@ -300,6 +311,7 @@ struct vm_area_struct;
> #define GFP_TRANSHUGE_LIGHT ((GFP_HIGHUSER_MOVABLE | __GFP_COMP | \
> __GFP_NOMEMALLOC | __GFP_NOWARN) & ~__GFP_RECLAIM)
> #define GFP_TRANSHUGE (GFP_TRANSHUGE_LIGHT | __GFP_DIRECT_RECLAIM)
> +#define GFP_ONBOOT (GFP_NOWAIT | __GFP_WONTFAIL)
>
Isn't it better to bind GFP_ONBOOT and GFP_NOWAIT?
Can be not GFP_NOWAIT when applying for memory at boot time
> /* Convert GFP flags to their corresponding migrate type */
> #define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ff5484fdbd..36dee09f7f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4625,6 +4625,14 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> fail:
> warn_alloc(gfp_mask, ac->nodemask,
> "page allocation failure: order:%u", order);
> + if (gfp_mask & __GFP_WONTFAIL) {
Is it more intuitive to use __GFP_DIE_IF_FAIL as the flag name?
> + /*
> + * The assumption was wrong. This is never supposed to happen.
> + * Caller most likely won't check for a returned NULL either.
> + * So the only reasonable thing to do is to pannic.
> + */
> + panic("Failed to allocate memory despite GFP_WONTFAIL\n");
> + }
> got_pg:
> return page;
> }
>
> .
>
thanks
Niaoming Ni
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-09-25 8:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-19 9:18 [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops Xiaoming Ni
2019-09-19 9:29 ` Greg KH
2019-09-19 15:16 ` Mikulas Patocka
2019-09-20 2:29 ` Nixiaoming
2019-09-20 6:04 ` Greg KH
2019-09-20 2:56 ` Nicolas Pitre
2019-09-20 6:04 ` Greg KH
2019-09-21 7:26 ` Xiaoming Ni
2019-09-23 3:50 ` Nicolas Pitre
2019-09-25 8:37 ` Xiaoming Ni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.