linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86: eas should not be NULL when it is referenced
@ 2021-06-24  7:04 13145886936
  2021-06-24  7:24 ` Peter Zijlstra
  2021-06-24 19:03 ` Liang, Kan
  0 siblings, 2 replies; 7+ messages in thread
From: 13145886936 @ 2021-06-24  7:04 UTC (permalink / raw)
  To: tglx, bp, x86; +Cc: linux-perf-users, linux-kernel, gushengxian

From: gushengxian <gushengxian@yulong.com>

"eas" should not be NULL when it is referenced.

Signed-off-by: gushengxian <gushengxian@yulong.com>
---
 arch/x86/events/intel/uncore_snbep.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index bb6eb1e5569c..836aa78cd0f3 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -3826,8 +3826,10 @@ pmu_iio_set_mapping(struct intel_uncore_type *type, struct attribute_group *ag)
 
 	return 0;
 err:
-	for (; die >= 0; die--)
-		kfree(eas[die].attr.attr.name);
+	if (eas) {
+		for (; die >= 0; die--)
+			kfree(eas[die].attr.attr.name);
+	}
 	kfree(eas);
 	kfree(attrs);
 	kfree(type->topology);
-- 
2.25.1



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86: eas should not be NULL when it is referenced
  2021-06-24  7:04 [PATCH] x86: eas should not be NULL when it is referenced 13145886936
@ 2021-06-24  7:24 ` Peter Zijlstra
  2021-06-24 19:03 ` Liang, Kan
  1 sibling, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2021-06-24  7:24 UTC (permalink / raw)
  To: 13145886936
  Cc: tglx, bp, x86, linux-perf-users, linux-kernel, gushengxian, Liang, Kan

On Thu, Jun 24, 2021 at 12:04:42AM -0700, 13145886936@163.com wrote:
> From: gushengxian <gushengxian@yulong.com>

Is ^^^ that you? That is, 13145886936@163.com is a bit of a dodgy email
address and i've no way of verifying you are the person responsible for
the patch.

Could you, if at all possible, please send patches from your yulong.com
account such that we have at least a coherent chain of custody?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86: eas should not be NULL when it is referenced
  2021-06-24  7:04 [PATCH] x86: eas should not be NULL when it is referenced 13145886936
  2021-06-24  7:24 ` Peter Zijlstra
@ 2021-06-24 19:03 ` Liang, Kan
  2021-06-24 19:06   ` Liang, Kan
  1 sibling, 1 reply; 7+ messages in thread
From: Liang, Kan @ 2021-06-24 19:03 UTC (permalink / raw)
  To: 13145886936, tglx, bp, x86
  Cc: linux-perf-users, linux-kernel, gushengxian, Antonov, Alexander


Hi Shengxian,

Thanks for the patch.

On 6/24/2021 3:04 AM, 13145886936@163.com wrote:
> From: gushengxian <gushengxian@yulong.com>
> 
> "eas" should not be NULL when it is referenced.
> 

I think the NULL pointer dereference of eas should not happen, because 
die is -1 if eas is NULL. But the whole error handling path looks fragile.

We already fixed one issue caused by it in commit ID f797f05d917f 
("perf/x86/intel/uncore: Fix for iio mapping on Skylake Server")
https://lore.kernel.org/lkml/160149233331.7002.10919231011379055356.tip-bot2@tip-bot2/

Maybe something as below?

 From 3de81ba3b04262ef3346297d82f6c4ffb4af7029 Mon Sep 17 00:00:00 2001
From: Kan Liang <kan.liang@linux.intel.com>
Date: Thu, 24 Jun 2021 11:17:57 -0700
Subject: [PATCH] perf/x86/intel/uncore: Clean up error handling path of 
iio mapping

The error handling path of iio mapping looks fragile. We already fixed
one issue caused by it, commit ID f797f05d917f ("perf/x86/intel/uncore:
Fix for iio mapping on Skylake Server"). Clean up the error handling
path and make the code robust.

Reported-by: gushengxian <gushengxian@yulong.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
  arch/x86/events/intel/uncore_snbep.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/uncore_snbep.c 
b/arch/x86/events/intel/uncore_snbep.c
index 7622762..6d4a5a9 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -3802,11 +3802,11 @@ pmu_iio_set_mapping(struct intel_uncore_type 
*type, struct attribute_group *ag)
  	/* One more for NULL. */
  	attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), GFP_KERNEL);
  	if (!attrs)
-		goto err;
+		goto clear_topology;

  	eas = kcalloc(uncore_max_dies(), sizeof(*eas), GFP_KERNEL);
  	if (!eas)
-		goto err;
+		goto clear_attrs;

  	for (die = 0; die < uncore_max_dies(); die++) {
  		sprintf(buf, "die%ld", die);
@@ -3827,7 +3827,9 @@ pmu_iio_set_mapping(struct intel_uncore_type 
*type, struct attribute_group *ag)
  	for (; die >= 0; die--)
  		kfree(eas[die].attr.attr.name);
  	kfree(eas);
+clear_attrs:
  	kfree(attrs);
+clear_topology:
  	kfree(type->topology);
  clear_attr_update:
  	type->attr_update = NULL;
-- 
2.7.4
Thanks,
Kan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86: eas should not be NULL when it is referenced
  2021-06-24 19:03 ` Liang, Kan
@ 2021-06-24 19:06   ` Liang, Kan
  2021-06-25 13:33     ` Alexander Antonov
  0 siblings, 1 reply; 7+ messages in thread
From: Liang, Kan @ 2021-06-24 19:06 UTC (permalink / raw)
  To: 13145886936, tglx, bp, x86
  Cc: linux-perf-users, linux-kernel, gushengxian, Antonov, Alexander,
	Peter Zijlstra


Oops, forgot to add Peter.

On 6/24/2021 3:03 PM, Liang, Kan wrote:
> 
> Hi Shengxian,
> 
> Thanks for the patch.
> 
> On 6/24/2021 3:04 AM, 13145886936@163.com wrote:
>> From: gushengxian <gushengxian@yulong.com>
>>
>> "eas" should not be NULL when it is referenced.
>>
> 
> I think the NULL pointer dereference of eas should not happen, because 
> die is -1 if eas is NULL. But the whole error handling path looks fragile.
> 
> We already fixed one issue caused by it in commit ID f797f05d917f 
> ("perf/x86/intel/uncore: Fix for iio mapping on Skylake Server")
> https://lore.kernel.org/lkml/160149233331.7002.10919231011379055356.tip-bot2@tip-bot2/ 
> 
> 
> Maybe something as below?
> 
>  From 3de81ba3b04262ef3346297d82f6c4ffb4af7029 Mon Sep 17 00:00:00 2001
> From: Kan Liang <kan.liang@linux.intel.com>
> Date: Thu, 24 Jun 2021 11:17:57 -0700
> Subject: [PATCH] perf/x86/intel/uncore: Clean up error handling path of 
> iio mapping
> 
> The error handling path of iio mapping looks fragile. We already fixed
> one issue caused by it, commit ID f797f05d917f ("perf/x86/intel/uncore:
> Fix for iio mapping on Skylake Server"). Clean up the error handling
> path and make the code robust.
> 
> Reported-by: gushengxian <gushengxian@yulong.com>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>   arch/x86/events/intel/uncore_snbep.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c 
> b/arch/x86/events/intel/uncore_snbep.c
> index 7622762..6d4a5a9 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -3802,11 +3802,11 @@ pmu_iio_set_mapping(struct intel_uncore_type 
> *type, struct attribute_group *ag)
>       /* One more for NULL. */
>       attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), GFP_KERNEL);
>       if (!attrs)
> -        goto err;
> +        goto clear_topology;
> 
>       eas = kcalloc(uncore_max_dies(), sizeof(*eas), GFP_KERNEL);
>       if (!eas)
> -        goto err;
> +        goto clear_attrs;
> 
>       for (die = 0; die < uncore_max_dies(); die++) {
>           sprintf(buf, "die%ld", die);
> @@ -3827,7 +3827,9 @@ pmu_iio_set_mapping(struct intel_uncore_type 
> *type, struct attribute_group *ag)
>       for (; die >= 0; die--)
>           kfree(eas[die].attr.attr.name);
>       kfree(eas);
> +clear_attrs:
>       kfree(attrs);
> +clear_topology:
>       kfree(type->topology);
>   clear_attr_update:
>       type->attr_update = NULL;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86: eas should not be NULL when it is referenced
  2021-06-24 19:06   ` Liang, Kan
@ 2021-06-25 13:33     ` Alexander Antonov
  2021-06-25 14:11       ` Liang, Kan
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Antonov @ 2021-06-25 13:33 UTC (permalink / raw)
  To: Liang, Kan, 13145886936, tglx, bp, x86
  Cc: linux-perf-users, linux-kernel, gushengxian, Peter Zijlstra

Hello Kan,
> On 6/24/2021 3:03 PM, Liang, Kan wrote:
>> I think the NULL pointer dereference of eas should not happen, 
>> because die is -1 if eas is NULL. But the whole error handling path 
>> looks fragile.
>>
>> We already fixed one issue caused by it in commit ID f797f05d917f 
>> ("perf/x86/intel/uncore: Fix for iio mapping on Skylake Server")
>> https://lore.kernel.org/lkml/160149233331.7002.10919231011379055356.tip-bot2@tip-bot2/ 
>>
>>
>> Maybe something as below?
>>
>>  From 3de81ba3b04262ef3346297d82f6c4ffb4af7029 Mon Sep 17 00:00:00 2001
>> From: Kan Liang <kan.liang@linux.intel.com>
>> Date: Thu, 24 Jun 2021 11:17:57 -0700
>> Subject: [PATCH] perf/x86/intel/uncore: Clean up error handling path 
>> of iio mapping
>>
>> The error handling path of iio mapping looks fragile. We already fixed
>> one issue caused by it, commit ID f797f05d917f ("perf/x86/intel/uncore:
>> Fix for iio mapping on Skylake Server"). Clean up the error handling
>> path and make the code robust.
I didn't catch why does the current error handling path look fragile?
Are there cases when it works incorrect?

Thanks,
Alexander
>>
>> Reported-by: gushengxian <gushengxian@yulong.com>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> ---
>>   arch/x86/events/intel/uncore_snbep.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/uncore_snbep.c 
>> b/arch/x86/events/intel/uncore_snbep.c
>> index 7622762..6d4a5a9 100644
>> --- a/arch/x86/events/intel/uncore_snbep.c
>> +++ b/arch/x86/events/intel/uncore_snbep.c
>> @@ -3802,11 +3802,11 @@ pmu_iio_set_mapping(struct intel_uncore_type 
>> *type, struct attribute_group *ag)
>>       /* One more for NULL. */
>>       attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), 
>> GFP_KERNEL);
>>       if (!attrs)
>> -        goto err;
>> +        goto clear_topology;
>>
>>       eas = kcalloc(uncore_max_dies(), sizeof(*eas), GFP_KERNEL);
>>       if (!eas)
>> -        goto err;
>> +        goto clear_attrs;
>>
>>       for (die = 0; die < uncore_max_dies(); die++) {
>>           sprintf(buf, "die%ld", die);
>> @@ -3827,7 +3827,9 @@ pmu_iio_set_mapping(struct intel_uncore_type 
>> *type, struct attribute_group *ag)
>>       for (; die >= 0; die--)
>>           kfree(eas[die].attr.attr.name);
>>       kfree(eas);
>> +clear_attrs:
>>       kfree(attrs);
>> +clear_topology:
>>       kfree(type->topology);
>>   clear_attr_update:
>>       type->attr_update = NULL;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86: eas should not be NULL when it is referenced
  2021-06-25 13:33     ` Alexander Antonov
@ 2021-06-25 14:11       ` Liang, Kan
  2021-06-25 14:26         ` Alexander Antonov
  0 siblings, 1 reply; 7+ messages in thread
From: Liang, Kan @ 2021-06-25 14:11 UTC (permalink / raw)
  To: Alexander Antonov, 13145886936, tglx, bp, x86
  Cc: linux-perf-users, linux-kernel, gushengxian, Peter Zijlstra



On 6/25/2021 9:33 AM, Alexander Antonov wrote:
> Hello Kan,
>> On 6/24/2021 3:03 PM, Liang, Kan wrote:
>>> I think the NULL pointer dereference of eas should not happen, 
>>> because die is -1 if eas is NULL. But the whole error handling path 
>>> looks fragile.
>>>
>>> We already fixed one issue caused by it in commit ID f797f05d917f 
>>> ("perf/x86/intel/uncore: Fix for iio mapping on Skylake Server")
>>> https://lore.kernel.org/lkml/160149233331.7002.10919231011379055356.tip-bot2@tip-bot2/ 
>>>
>>>
>>> Maybe something as below?
>>>
>>>  From 3de81ba3b04262ef3346297d82f6c4ffb4af7029 Mon Sep 17 00:00:00 2001
>>> From: Kan Liang <kan.liang@linux.intel.com>
>>> Date: Thu, 24 Jun 2021 11:17:57 -0700
>>> Subject: [PATCH] perf/x86/intel/uncore: Clean up error handling path 
>>> of iio mapping
>>>
>>> The error handling path of iio mapping looks fragile. We already fixed
>>> one issue caused by it, commit ID f797f05d917f ("perf/x86/intel/uncore:
>>> Fix for iio mapping on Skylake Server"). Clean up the error handling
>>> path and make the code robust.
> I didn't catch why does the current error handling path look fragile?
> Are there cases when it works incorrect?
>


I don't think it causes any severe problem for now, e.g., crash, because 
current code checks die before the dereference.
But I think it violates the Linux kernel coding style (one err bug) and 
may bring potential issues.

https://www.kernel.org/doc/html/v4.10/process/coding-style.html

Thanks,
Kan

> Thanks,
> Alexander
>>>
>>> Reported-by: gushengxian <gushengxian@yulong.com>
>>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>>> ---
>>>   arch/x86/events/intel/uncore_snbep.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/events/intel/uncore_snbep.c 
>>> b/arch/x86/events/intel/uncore_snbep.c
>>> index 7622762..6d4a5a9 100644
>>> --- a/arch/x86/events/intel/uncore_snbep.c
>>> +++ b/arch/x86/events/intel/uncore_snbep.c
>>> @@ -3802,11 +3802,11 @@ pmu_iio_set_mapping(struct intel_uncore_type 
>>> *type, struct attribute_group *ag)
>>>       /* One more for NULL. */
>>>       attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), 
>>> GFP_KERNEL);
>>>       if (!attrs)
>>> -        goto err;
>>> +        goto clear_topology;
>>>
>>>       eas = kcalloc(uncore_max_dies(), sizeof(*eas), GFP_KERNEL);
>>>       if (!eas)
>>> -        goto err;
>>> +        goto clear_attrs;
>>>
>>>       for (die = 0; die < uncore_max_dies(); die++) {
>>>           sprintf(buf, "die%ld", die);
>>> @@ -3827,7 +3827,9 @@ pmu_iio_set_mapping(struct intel_uncore_type 
>>> *type, struct attribute_group *ag)
>>>       for (; die >= 0; die--)
>>>           kfree(eas[die].attr.attr.name);
>>>       kfree(eas);
>>> +clear_attrs:
>>>       kfree(attrs);
>>> +clear_topology:
>>>       kfree(type->topology);
>>>   clear_attr_update:
>>>       type->attr_update = NULL;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] x86: eas should not be NULL when it is referenced
  2021-06-25 14:11       ` Liang, Kan
@ 2021-06-25 14:26         ` Alexander Antonov
  0 siblings, 0 replies; 7+ messages in thread
From: Alexander Antonov @ 2021-06-25 14:26 UTC (permalink / raw)
  To: Liang, Kan, 13145886936, tglx, bp, x86
  Cc: linux-perf-users, linux-kernel, gushengxian, Peter Zijlstra


On 6/25/2021 5:11 PM, Liang, Kan wrote:
>
>
> On 6/25/2021 9:33 AM, Alexander Antonov wrote:
>> Hello Kan,
>>> On 6/24/2021 3:03 PM, Liang, Kan wrote:
>>>> I think the NULL pointer dereference of eas should not happen, 
>>>> because die is -1 if eas is NULL. But the whole error handling path 
>>>> looks fragile.
>>>>
>>>> We already fixed one issue caused by it in commit ID f797f05d917f 
>>>> ("perf/x86/intel/uncore: Fix for iio mapping on Skylake Server")
>>>> https://lore.kernel.org/lkml/160149233331.7002.10919231011379055356.tip-bot2@tip-bot2/ 
>>>>
>>>>
>>>> Maybe something as below?
>>>>
>>>>  From 3de81ba3b04262ef3346297d82f6c4ffb4af7029 Mon Sep 17 00:00:00 
>>>> 2001
>>>> From: Kan Liang <kan.liang@linux.intel.com>
>>>> Date: Thu, 24 Jun 2021 11:17:57 -0700
>>>> Subject: [PATCH] perf/x86/intel/uncore: Clean up error handling 
>>>> path of iio mapping
>>>>
>>>> The error handling path of iio mapping looks fragile. We already fixed
>>>> one issue caused by it, commit ID f797f05d917f 
>>>> ("perf/x86/intel/uncore:
>>>> Fix for iio mapping on Skylake Server"). Clean up the error handling
>>>> path and make the code robust.
>> I didn't catch why does the current error handling path look fragile?
>> Are there cases when it works incorrect?
>>
>
>
> I don't think it causes any severe problem for now, e.g., crash, 
> because current code checks die before the dereference.
> But I think it violates the Linux kernel coding style (one err bug) 
> and may bring potential issues.
>
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html
>
> Thanks,
> Kan

OK,
Thank you for the explanation.

- Alexander
>
>> Thanks,
>> Alexander
>>>>
>>>> Reported-by: gushengxian <gushengxian@yulong.com>
>>>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>>>> ---
>>>>   arch/x86/events/intel/uncore_snbep.c | 6 ++++--
>>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/events/intel/uncore_snbep.c 
>>>> b/arch/x86/events/intel/uncore_snbep.c
>>>> index 7622762..6d4a5a9 100644
>>>> --- a/arch/x86/events/intel/uncore_snbep.c
>>>> +++ b/arch/x86/events/intel/uncore_snbep.c
>>>> @@ -3802,11 +3802,11 @@ pmu_iio_set_mapping(struct 
>>>> intel_uncore_type *type, struct attribute_group *ag)
>>>>       /* One more for NULL. */
>>>>       attrs = kcalloc((uncore_max_dies() + 1), sizeof(*attrs), 
>>>> GFP_KERNEL);
>>>>       if (!attrs)
>>>> -        goto err;
>>>> +        goto clear_topology;
>>>>
>>>>       eas = kcalloc(uncore_max_dies(), sizeof(*eas), GFP_KERNEL);
>>>>       if (!eas)
>>>> -        goto err;
>>>> +        goto clear_attrs;
>>>>
>>>>       for (die = 0; die < uncore_max_dies(); die++) {
>>>>           sprintf(buf, "die%ld", die);
>>>> @@ -3827,7 +3827,9 @@ pmu_iio_set_mapping(struct intel_uncore_type 
>>>> *type, struct attribute_group *ag)
>>>>       for (; die >= 0; die--)
>>>>           kfree(eas[die].attr.attr.name);
>>>>       kfree(eas);
>>>> +clear_attrs:
>>>>       kfree(attrs);
>>>> +clear_topology:
>>>>       kfree(type->topology);
>>>>   clear_attr_update:
>>>>       type->attr_update = NULL;

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-06-25 14:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-24  7:04 [PATCH] x86: eas should not be NULL when it is referenced 13145886936
2021-06-24  7:24 ` Peter Zijlstra
2021-06-24 19:03 ` Liang, Kan
2021-06-24 19:06   ` Liang, Kan
2021-06-25 13:33     ` Alexander Antonov
2021-06-25 14:11       ` Liang, Kan
2021-06-25 14:26         ` Alexander Antonov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).