* [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; 8+ 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] 8+ 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; 8+ 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] 8+ 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 2021-07-05 7:53 ` [tip: perf/urgent] perf/x86/intel/uncore: Clean up error handling path of iio mapping tip-bot2 for Kan Liang 1 sibling, 2 replies; 8+ 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] 8+ 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 2021-07-05 7:53 ` [tip: perf/urgent] perf/x86/intel/uncore: Clean up error handling path of iio mapping tip-bot2 for Kan Liang 1 sibling, 1 reply; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ messages in thread
* [tip: perf/urgent] perf/x86/intel/uncore: Clean up error handling path of iio mapping 2021-06-24 19:03 ` Liang, Kan 2021-06-24 19:06 ` Liang, Kan @ 2021-07-05 7:53 ` tip-bot2 for Kan Liang 1 sibling, 0 replies; 8+ messages in thread From: tip-bot2 for Kan Liang @ 2021-07-05 7:53 UTC (permalink / raw) To: linux-tip-commits Cc: gushengxian, Kan Liang, Peter Zijlstra (Intel), x86, linux-kernel The following commit has been merged into the perf/urgent branch of tip: Commit-ID: d4ba0b06306a70c99a43f9d452886a86e2d3bd26 Gitweb: https://git.kernel.org/tip/d4ba0b06306a70c99a43f9d452886a86e2d3bd26 Author: Kan Liang <kan.liang@linux.intel.com> AuthorDate: Thu, 24 Jun 2021 11:17:57 -07:00 Committer: Peter Zijlstra <peterz@infradead.org> CommitterDate: Fri, 02 Jul 2021 15:58:33 +02:00 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 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> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: https://lkml.kernel.org/r/40e66cf9-398b-20d7-ce4d-433be6e08921@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 3a75a2c..1f7bb48 100644 --- a/arch/x86/events/intel/uncore_snbep.c +++ b/arch/x86/events/intel/uncore_snbep.c @@ -3789,11 +3789,11 @@ static int skx_iio_set_mapping(struct intel_uncore_type *type) /* 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); @@ -3814,7 +3814,9 @@ err: 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 related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-05 7:54 UTC | newest] Thread overview: 8+ 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 2021-07-05 7:53 ` [tip: perf/urgent] perf/x86/intel/uncore: Clean up error handling path of iio mapping tip-bot2 for Kan Liang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.