Messages in this thread | | | Subject | Re: [PATCH] x86: eas should not be NULL when it is referenced | From | "Liang, Kan" <> | Date | Fri, 25 Jun 2021 10:11:32 -0400 |
| |
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;
| |