Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 19 Sep 2019 11:16:17 -0400 (EDT)
From:   Mikulas Patocka <mpatocka@redhat.com>
To:     Greg KH <gregkh@linuxfoundation.org>
cc:     Xiaoming Ni <nixiaoming@huawei.com>, penberg@cs.helsinki.fi,
        jslaby@suse.com, nico@fluxnic.net, textshell@uchuujin.de,
        sam@ravnborg.org, daniel.vetter@ffwll.ch, ghalat@redhat.com,
        linux-kernel@vger.kernel.org, yangyingliang@huawei.com,
        yuehaibing@huawei.com, zengweilin@huawei.com
Subject: Re: [PATCH] tty:vt: Add check the return value of kzalloc to avoid
 oops
In-Reply-To: <20190919092933.GA2684163@kroah.com>
Message-ID: <alpine.LRH.2.02.1909191114340.14850@file01.intranet.prod.int.rdu2.redhat.com>
References: <1568884695-56789-1-git-send-email-nixiaoming@huawei.com> <20190919092933.GA2684163@kroah.com>
User-Agent: Alpine 2.02 (LRH 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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