[PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine

[PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine

[Sasha Levin]

From: Austin Kim <austindh.kim@gmail.com>

[ 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 <austindh.kim@gmail.com>
Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com>
Cc: Hedi Berriche <hedi.berriche@hpe.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mike Travis <mike.travis@hpe.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Russ Anderson <russ.anderson@hpe.com>
Cc: Steve Wahl <steve.wahl@hpe.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: allison@lohutok.net
Cc: andy@infradead.org
Cc: armijn@tjaldur.nl
Cc: bp@alien8.de
Cc: dvhart@infradead.org
Cc: gregkh@linuxfoundation.org
Cc: hpa@zytor.com
Cc: kjlu@umn.edu
Cc: platform-driver-x86@vger.kernel.org
Link: https://lkml.kernel.org/r/20190905232951.GA28779@LGEARND20B15
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 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);
 
-- 
2.20.1

Re: [PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine

[Greg KH]

On Sun, Sep 22, 2019 at 02:43:15PM -0400, Sasha Levin wrote:
> From: Austin Kim <austindh.kim@gmail.com>
> 
> [ 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 <austindh.kim@gmail.com>
> Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com>
> Cc: Hedi Berriche <hedi.berriche@hpe.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Mike Travis <mike.travis@hpe.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Russ Anderson <russ.anderson@hpe.com>
> Cc: Steve Wahl <steve.wahl@hpe.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: allison@lohutok.net
> Cc: andy@infradead.org
> Cc: armijn@tjaldur.nl
> Cc: bp@alien8.de
> Cc: dvhart@infradead.org
> Cc: gregkh@linuxfoundation.org
> Cc: hpa@zytor.com
> Cc: kjlu@umn.edu
> Cc: platform-driver-x86@vger.kernel.org
> Link: https://lkml.kernel.org/r/20190905232951.GA28779@LGEARND20B15
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  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 :(

Please drop this.

thanks,

greg k-h

Re: [PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine

[Sasha Levin]

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 <austindh.kim@gmail.com>
>>
>> [ 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 <austindh.kim@gmail.com>
>> Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com>
>> Cc: Hedi Berriche <hedi.berriche@hpe.com>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Mike Travis <mike.travis@hpe.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Russ Anderson <russ.anderson@hpe.com>
>> Cc: Steve Wahl <steve.wahl@hpe.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: allison@lohutok.net
>> Cc: andy@infradead.org
>> Cc: armijn@tjaldur.nl
>> Cc: bp@alien8.de
>> Cc: dvhart@infradead.org
>> Cc: gregkh@linuxfoundation.org
>> Cc: hpa@zytor.com
>> Cc: kjlu@umn.edu
>> Cc: platform-driver-x86@vger.kernel.org
>> Link: https://lkml.kernel.org/r/20190905232951.GA28779@LGEARND20B15
>> Signed-off-by: Ingo Molnar <mingo@kernel.org>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> ---
>>  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 :(
>
>Please drop this.

Heh, I've dropped it.

RE: [PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine

[David Laight]

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 <austindh.kim@gmail.com>
> >>
> >> [ 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 <austindh.kim@gmail.com>
...
> >> ---
> >>  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)

Re: [PATCH AUTOSEL 5.3 169/203] x86/platform/uv: Fix kmalloc() NULL check routine

[Mike Travis]

On 10/2/2019 1:34 AM, David Laight wrote:
> 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 <austindh.kim@gmail.com>
>>>>
>>>> [ 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 <austindh.kim@gmail.com>
> ...
>>>> ---
>>>>   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)
> 

Thanks, I will put all of these comments in my notes.  This whole 
function is slated to move to a specialized UV APIC driver since it uses 
a unique scaling feature available in the UV hardware.  (The original 
author has long retired.)

-Mike

[PATCH AUTOSEL 5.3 171/203] platform/x86: intel_pmc_core: Do not ioremap RAM

[Sasha Levin]

From: "M. Vefa Bicakci" <m.v.b@runbox.com>

[ Upstream commit 7d505758b1e556cdf65a5e451744fe0ae8063d17 ]

On a Xen-based PVH virtual machine with more than 4 GiB of RAM,
intel_pmc_core fails initialization with the following warning message
from the kernel, indicating that the driver is attempting to ioremap
RAM:

  ioremap on RAM at 0x00000000fe000000 - 0x00000000fe001fff
  WARNING: CPU: 1 PID: 434 at arch/x86/mm/ioremap.c:186 __ioremap_caller.constprop.0+0x2aa/0x2c0
...
  Call Trace:
   ? pmc_core_probe+0x87/0x2d0 [intel_pmc_core]
   pmc_core_probe+0x87/0x2d0 [intel_pmc_core]

This issue appears to manifest itself because of the following fallback
mechanism in the driver:

	if (lpit_read_residency_count_address(&slp_s0_addr))
		pmcdev->base_addr = PMC_BASE_ADDR_DEFAULT;

The validity of address PMC_BASE_ADDR_DEFAULT (i.e., 0xFE000000) is not
verified by the driver, which is what this patch introduces. With this
patch, if address PMC_BASE_ADDR_DEFAULT is in RAM, then the driver will
not attempt to ioremap the aforementioned address.

Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/platform/x86/intel_pmc_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index c510d0d724759..3b6b8dcc47678 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -878,10 +878,14 @@ static int pmc_core_probe(struct platform_device *pdev)
 	if (pmcdev->map == &spt_reg_map && !pci_dev_present(pmc_pci_ids))
 		pmcdev->map = &cnp_reg_map;
 
-	if (lpit_read_residency_count_address(&slp_s0_addr))
+	if (lpit_read_residency_count_address(&slp_s0_addr)) {
 		pmcdev->base_addr = PMC_BASE_ADDR_DEFAULT;
-	else
+
+		if (page_is_ram(PHYS_PFN(pmcdev->base_addr)))
+			return -ENODEV;
+	} else {
 		pmcdev->base_addr = slp_s0_addr - pmcdev->map->slp_s0_offset;
+	}
 
 	pmcdev->regbase = ioremap(pmcdev->base_addr,
 				  pmcdev->map->regmap_length);
-- 
2.20.1

[PATCH AUTOSEL 5.3 172/203] platform/x86: intel_pmc_core_pltdrv: Module removal warning fix

[Sasha Levin]

From: "M. Vefa Bicakci" <m.v.b@runbox.com>

[ Upstream commit 0b43e41e93815ecd9616759cf5d64d3a7be8e6fb ]

Prior to this commit, removing the intel_pmc_core_pltdrv module
would cause the following warning:

  Device 'intel_pmc_core.0' does not have a release() function, it is broken and must be fixed. See Documentation/kobject.txt.
  WARNING: CPU: 0 PID: 2202 at drivers/base/core.c:1238 device_release+0x6f/0x80

This commit hence adds an empty release function for the driver.

Signed-off-by: M. Vefa Bicakci <m.v.b@runbox.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/platform/x86/intel_pmc_core_pltdrv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/intel_pmc_core_pltdrv.c b/drivers/platform/x86/intel_pmc_core_pltdrv.c
index a8754a6db1b8b..186540014c480 100644
--- a/drivers/platform/x86/intel_pmc_core_pltdrv.c
+++ b/drivers/platform/x86/intel_pmc_core_pltdrv.c
@@ -18,8 +18,16 @@
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 
+static void intel_pmc_core_release(struct device *dev)
+{
+	/* Nothing to do. */
+}
+
 static struct platform_device pmc_core_device = {
 	.name = "intel_pmc_core",
+	.dev  = {
+		.release = intel_pmc_core_release,
+	},
 };
 
 /*
-- 
2.20.1