From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E1DB4C352AA for ; Wed, 2 Oct 2019 08:34:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BF2CB2190F for ; Wed, 2 Oct 2019 08:34:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727824AbfJBIeN convert rfc822-to-8bit (ORCPT ); Wed, 2 Oct 2019 04:34:13 -0400 Received: from eu-smtp-delivery-151.mimecast.com ([207.82.80.151]:59409 "EHLO eu-smtp-delivery-151.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726559AbfJBIeM (ORCPT ); Wed, 2 Oct 2019 04:34:12 -0400 Received: from AcuMS.aculab.com (156.67.243.126 [156.67.243.126]) (Using TLS) by relay.mimecast.com with ESMTP id uk-mta-106-A4o3tKQ9OZitIOq_xjIfUQ-1; Wed, 02 Oct 2019 09:34:09 +0100 Received: from AcuMS.Aculab.com (fd9f:af1c:a25b:0:43c:695e:880f:8750) by AcuMS.aculab.com (fd9f:af1c:a25b:0:43c:695e:880f:8750) with Microsoft SMTP Server (TLS) id 15.0.1347.2; Wed, 2 Oct 2019 09:34:08 +0100 Received: from AcuMS.Aculab.com ([fe80::43c:695e:880f:8750]) by AcuMS.aculab.com ([fe80::43c:695e:880f:8750%12]) with mapi id 15.00.1347.000; Wed, 2 Oct 2019 09:34:08 +0100 From: David Laight To: 'Sasha Levin' , Greg KH CC: "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , Austin Kim , Dimitri Sivanich , "Hedi Berriche" , Linus Torvalds , Mike Travis , "Peter Zijlstra" , Russ Anderson , "Steve Wahl" , Thomas Gleixner , "allison@lohutok.net" , "andy@infradead.org" , "armijn@tjaldur.nl" , "bp@alien8.de" , "dvhart@infradead.org" , "hpa@zytor.com" , "kjlu@umn.edu" , "platform-driver-x86@vger.kernel.org" , Ingo Molnar Subject: RE: [PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine Thread-Topic: [PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine Thread-Index: AQHVeHIkSedJbnEQTUevg+MgQTj4iqdHBdUw Date: Wed, 2 Oct 2019 08:34:08 +0000 Message-ID: References: <20190922184350.30563-1-sashal@kernel.org> <20190922184350.30563-169-sashal@kernel.org> <20190922202544.GA2719513@kroah.com> <20191001160601.GX8171@sasha-vm> In-Reply-To: <20191001160601.GX8171@sasha-vm> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.202.205.107] MIME-Version: 1.0 X-MC-Unique: A4o3tKQ9OZitIOq_xjIfUQ-1 X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Sasha Levin > Sent: 01 October 2019 17:06 > Subject: Re: [PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine > > On Sun, Sep 22, 2019 at 10:25:44PM +0200, Greg KH wrote: > >On Sun, Sep 22, 2019 at 02:43:15PM -0400, Sasha Levin wrote: > >> From: Austin Kim > >> > >> [ Upstream commit 864b23f0169d5bff677e8443a7a90dfd6b090afc ] > >> > >> The result of kmalloc() should have been checked ahead of below statement: > >> > >> pqp = (struct bau_pq_entry *)vp; > >> > >> Move BUG_ON(!vp) before above statement. > >> > >> Signed-off-by: Austin Kim ... > >> --- > >> arch/x86/platform/uv/tlb_uv.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/x86/platform/uv/tlb_uv.c b/arch/x86/platform/uv/tlb_uv.c > >> index 20c389a91b803..5f0a96bf27a1f 100644 > >> --- a/arch/x86/platform/uv/tlb_uv.c > >> +++ b/arch/x86/platform/uv/tlb_uv.c > >> @@ -1804,9 +1804,9 @@ static void pq_init(int node, int pnode) > >> > >> plsize = (DEST_Q_SIZE + 1) * sizeof(struct bau_pq_entry); > >> vp = kmalloc_node(plsize, GFP_KERNEL, node); > >> - pqp = (struct bau_pq_entry *)vp; > >> - BUG_ON(!pqp); > >> + BUG_ON(!vp); > >> > >> + pqp = (struct bau_pq_entry *)vp; > >> cp = (char *)pqp + 31; > >> pqp = (struct bau_pq_entry *)(((unsigned long)cp >> 5) << 5); > >> > > > >How did this even get merged in the first place? I thought a number of > >us complained about it. > > > >This isn't any change in code, and the original is just fine, the author > >didn't realize how C works :( Mind you, the code itself if pretty horrid. Looks like it is aligning to 32 bytes, easier done by: pqp = (void *)((unsigned long)vp + 31 & ~31); (and there's a roundup macro to obfuscate it somewhere.) But I'd also expect to see a matching '+ 31' in the size passed to kmalloc(). Not to mention a comment! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Laight Subject: RE: [PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine Date: Wed, 2 Oct 2019 08:34:08 +0000 Message-ID: References: <20190922184350.30563-1-sashal@kernel.org> <20190922184350.30563-169-sashal@kernel.org> <20190922202544.GA2719513@kroah.com> <20191001160601.GX8171@sasha-vm> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <20191001160601.GX8171@sasha-vm> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: 'Sasha Levin' , Greg KH Cc: "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , Austin Kim , Dimitri Sivanich , Hedi Berriche , Linus Torvalds , Mike Travis , Peter Zijlstra , Russ Anderson , Steve Wahl , Thomas Gleixner , "allison@lohutok.net" , "andy@infradead.org" , "armijn@tjaldur.nl" , "bp@alien8.de" , "dvhart@infradead.org" , "hpa@zytor.com" , "kjlu@umn.edu" , platform-driver List-Id: platform-driver-x86.vger.kernel.org From: Sasha Levin > Sent: 01 October 2019 17:06 > Subject: Re: [PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine > > On Sun, Sep 22, 2019 at 10:25:44PM +0200, Greg KH wrote: > >On Sun, Sep 22, 2019 at 02:43:15PM -0400, Sasha Levin wrote: > >> From: Austin Kim > >> > >> [ Upstream commit 864b23f0169d5bff677e8443a7a90dfd6b090afc ] > >> > >> The result of kmalloc() should have been checked ahead of below statement: > >> > >> pqp = (struct bau_pq_entry *)vp; > >> > >> Move BUG_ON(!vp) before above statement. > >> > >> Signed-off-by: Austin Kim ... > >> --- > >> arch/x86/platform/uv/tlb_uv.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/x86/platform/uv/tlb_uv.c b/arch/x86/platform/uv/tlb_uv.c > >> index 20c389a91b803..5f0a96bf27a1f 100644 > >> --- a/arch/x86/platform/uv/tlb_uv.c > >> +++ b/arch/x86/platform/uv/tlb_uv.c > >> @@ -1804,9 +1804,9 @@ static void pq_init(int node, int pnode) > >> > >> plsize = (DEST_Q_SIZE + 1) * sizeof(struct bau_pq_entry); > >> vp = kmalloc_node(plsize, GFP_KERNEL, node); > >> - pqp = (struct bau_pq_entry *)vp; > >> - BUG_ON(!pqp); > >> + BUG_ON(!vp); > >> > >> + pqp = (struct bau_pq_entry *)vp; > >> cp = (char *)pqp + 31; > >> pqp = (struct bau_pq_entry *)(((unsigned long)cp >> 5) << 5); > >> > > > >How did this even get merged in the first place? I thought a number of > >us complained about it. > > > >This isn't any change in code, and the original is just fine, the author > >didn't realize how C works :( Mind you, the code itself if pretty horrid. Looks like it is aligning to 32 bytes, easier done by: pqp = (void *)((unsigned long)vp + 31 & ~31); (and there's a roundup macro to obfuscate it somewhere.) But I'd also expect to see a matching '+ 31' in the size passed to kmalloc(). Not to mention a comment! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)