* [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 related [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 related [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).