Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid oops

[Mikulas Patocka]

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