From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760711AbXK1KyA (ORCPT ); Wed, 28 Nov 2007 05:54:00 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1757607AbXK1Kxx (ORCPT ); Wed, 28 Nov 2007 05:53:53 -0500 Received: from wa-out-1112.google.com ([209.85.146.180]:13852 "EHLO wa-out-1112.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756404AbXK1Kxw (ORCPT ); Wed, 28 Nov 2007 05:53:52 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=received:message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=YoIRWzHh/DtZbFQLKDUJhRYYCQQr0EVgOnZz1wcnP/h3b+TNijON5kbDgVuC7c7G+9RSSTioHZTax7G3L3h5dfpKMzzIEbkker8ybnQyGwfrvcem69hDOBldxgn/k9LCX7rHN40wjRH7Yu71VuiphuyR7s9TlWFbNMvg4wiMBvc= Message-ID: Date: Wed, 28 Nov 2007 13:53:46 +0300 From: "Cyrill Gorcunov" To: michael@ellerman.id.au Subject: Re: [PATCH] PPC: CELLEB - fix potential NULL pointer dereference Cc: PPCML , "Olof Johansson" , "Paul Mackerras" , LKML , "Ishizaki Kou" In-Reply-To: <1196243333.2109.2.camel@concordia> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <1196243333.2109.2.camel@concordia> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org On 11/28/07, Michael Ellerman wrote: > On Mon, 2007-11-26 at 10:46 +0300, Cyrill Gorcunov wrote: > > This patch adds checking for NULL value returned to prevent possible > > NULL pointer dereference. > > Also two unneeded 'return' are removed. > > > > Signed-off-by: Cyrill Gorcunov > > --- > > Any comments are welcome. > > I guess it's good to be paranoid, but this is a little verbose: > > wi0 = of_get_property(node, "device-id", NULL); > + if (unlikely((!wi0))) { > + printk(KERN_ERR "PCI: device-id not found.\n"); > + goto error; > + } > wi1 = of_get_property(node, "vendor-id", NULL); > + if (unlikely((!wi1))) { > + printk(KERN_ERR "PCI: vendor-id not found.\n"); > + goto error; > + } > wi2 = of_get_property(node, "class-code", NULL); > + if (unlikely((!wi2))) { > + printk(KERN_ERR "PCI: class-code not found.\n"); > + goto error; > + } > wi3 = of_get_property(node, "revision-id", NULL); > + if (unlikely((!wi3))) { > + printk(KERN_ERR "PCI: revision-id not found.\n"); > + goto error; > + } > > Perhaps instead: > > wi0 = of_get_property(node, "device-id", NULL); > wi1 = of_get_property(node, "vendor-id", NULL); > wi2 = of_get_property(node, "class-code", NULL); > wi3 = of_get_property(node, "revision-id", NULL); > > if (!wi0 || !wi1 || !wi2 || !wi3) { > printk(KERN_ERR "PCI: Missing device tree properties.\n"); > goto error; > } Hi Michael, yes that is much better (actually I was doubt about what form of which the checking style to use - your form is much compact but mine does show where *exactly* the problem appeared). So 'case that is the fake driver your form is preferred ;) Ishizaki, could you use Michael's part then? > > > cheers > > -- > Michael Ellerman > OzLabs, IBM Australia Development Lab > > wwweb: http://michael.ellerman.id.au > phone: +61 2 6212 1183 (tie line 70 21183) > > We do not inherit the earth from our ancestors, > we borrow it from our children. - S.M.A.R.T Person > > Cyrill