| Message ID | 20231026142930.2670705-1-sdonthineni@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] drivers/perf: Fix kernel panic due to the invalid mon_ctx pointer | expand |
[ Since I made the mistake of looking... ] On 26/10/2023 3:29 pm, Shanker Donthineni wrote: > The return pointer from the resctrl_arch_mon_ctx_alloc_no_wait() function > is saved in a 32-bit variable 'hwc->idx,' which results in the loss of > the upper 32 bits. This, in turn, triggers a kernel panic when attempting > to access a corrupted pointer. > > This patch addresses the issue by utilizing the IDR data structure to > keep track of a 32-bit value associated with a pointer (64-bit value). This seems unnecessaraily complicated - there's plenty of space in that anonymous hw_perf_event "driver data" union (I know I cram quite a lot in there for arm-cmn) so it should just be a case of picking a different appropriately-sized field to (ab)use. Thanks, Robin. > Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com> > --- > drivers/perf/resctrl_pmu.c | 27 ++++++++++++++++++++++++--- > 1 file changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/perf/resctrl_pmu.c b/drivers/perf/resctrl_pmu.c > index 99a2b90b5d83..68cda619da48 100644 > --- a/drivers/perf/resctrl_pmu.c > +++ b/drivers/perf/resctrl_pmu.c > @@ -6,6 +6,8 @@ > #include <linux/perf_event.h> > #include <linux/platform_device.h> > #include <linux/resctrl.h> > +#include <linux/mutex.h> > +#include <linux/idr.h> > > struct resctrl_pmu { > struct pmu pmu; > @@ -25,6 +27,9 @@ RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(event, config, 0, 7); > RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(domain, config, 16, 23); > RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(resctrl_id, config1, 0, 63); > > +static DEFINE_MUTEX(resctrl_pmu_lock); > +static DEFINE_IDR(resctrl_pmu_idr); > + > static void resctrl_pmu_do_nothing(struct pmu *pmu) > { > } > @@ -69,12 +74,17 @@ static void resctrl_pmu_event_destroy(struct perf_event *event) > struct hw_perf_event *hwc = &event->hw; > u16 event_num = get_event(event); > struct rdt_resource *r; > + void *mon_ctx; > > r = resctrl_event_get_resource(event_num); > if (!r) > return; > > - resctrl_arch_mon_ctx_free(r, event_num, hwc->idx); > + mutex_lock(&resctrl_pmu_lock); > + mon_ctx = idr_remove(&resctrl_pmu_idr, hwc->idx); > + mutex_unlock(&resctrl_pmu_lock); > + > + resctrl_arch_mon_ctx_free(r, event_num, mon_ctx); > } > > static int resctrl_pmu_event_init(struct perf_event *event) > @@ -87,6 +97,7 @@ static int resctrl_pmu_event_init(struct perf_event *event) > struct rdt_domain *d; > u16 event_num, domain_id; > u32 closid, rmid; > + void *mon_ctx; > int err; > u64 id; > > @@ -144,7 +155,12 @@ static int resctrl_pmu_event_init(struct perf_event *event) > return -EINVAL; > } > > - hwc->idx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num); > + mon_ctx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num); > + > + mutex_lock(&resctrl_pmu_lock); > + hwc->idx = idr_alloc(&resctrl_pmu_idr, mon_ctx, 0, INT_MAX, GFP_KERNEL); > + mutex_unlock(&resctrl_pmu_lock); > + > if (hwc->idx == -ENOSPC) > return -ENOSPC; > event->destroy = resctrl_pmu_event_destroy; > @@ -162,6 +178,7 @@ static void resctrl_pmu_event_update(struct perf_event *event) > struct rdt_resource *r; > struct rdt_domain *d; > u32 closid, rmid; > + void *mon_ctx; > int err; > > event_num = get_event(event); > @@ -179,11 +196,15 @@ static void resctrl_pmu_event_update(struct perf_event *event) > if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask)) > return; > > + mutex_lock(&resctrl_pmu_lock); > + mon_ctx = idr_find(&resctrl_pmu_idr, hwc->idx); > + mutex_unlock(&resctrl_pmu_lock); > + > do { > prev = local64_read(&hwc->prev_count); > > err = resctrl_arch_rmid_read(r, d, closid, rmid, > - event_num, &now, hwc->idx); > + event_num, &now, mon_ctx); > if (err) > return; > } while (local64_cmpxchg(&hwc->prev_count, prev, now) != prev);
Hi Robin, On 10/26/23 11:39, Robin Murphy wrote: > External email: Use caution opening links or attachments > > > [ Since I made the mistake of looking... ] > > On 26/10/2023 3:29 pm, Shanker Donthineni wrote: >> The return pointer from the resctrl_arch_mon_ctx_alloc_no_wait() function >> is saved in a 32-bit variable 'hwc->idx,' which results in the loss of >> the upper 32 bits. This, in turn, triggers a kernel panic when attempting >> to access a corrupted pointer. >> >> This patch addresses the issue by utilizing the IDR data structure to >> keep track of a 32-bit value associated with a pointer (64-bit value). > > This seems unnecessaraily complicated - there's plenty of space in that > anonymous hw_perf_event "driver data" union (I know I cram quite a lot > in there for arm-cmn) so it should just be a case of picking a different > appropriately-sized field to (ab)use. > It appears that there is already a private pointer named 'pmu_private' in the 'perf_event' structure. I will submit version 2 of the code with this modification. --- a/drivers/perf/resctrl_pmu.c +++ b/drivers/perf/resctrl_pmu.c @@ -66,7 +66,6 @@ static struct rdt_resource *resctrl_event_get_resource(u16 event_num) static void resctrl_pmu_event_destroy(struct perf_event *event) { - struct hw_perf_event *hwc = &event->hw; u16 event_num = get_event(event); struct rdt_resource *r; @@ -74,7 +73,7 @@ static void resctrl_pmu_event_destroy(struct perf_event *event) if (!r) return; - resctrl_arch_mon_ctx_free(r, event_num, hwc->idx); + resctrl_arch_mon_ctx_free(r, event_num, event->pmu_private); } static int resctrl_pmu_event_init(struct perf_event *event) @@ -144,8 +143,8 @@ static int resctrl_pmu_event_init(struct perf_event *event) return -EINVAL; } - hwc->idx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num); - if (hwc->idx == -ENOSPC) + event->pmu_private = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num); + if (IS_ERR_OR_NULL(event->pmu_private)) return -ENOSPC; event->destroy = resctrl_pmu_event_destroy; local64_set(&hwc->prev_count, 0); @@ -183,7 +182,8 @@ static void resctrl_pmu_event_update(struct perf_event *event) prev = local64_read(&hwc->prev_count); err = resctrl_arch_rmid_read(r, d, closid, rmid, - event_num, &now, hwc->idx); + event_num, &now, + event->pmu_private); > Thanks, > Robin. > >> Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com> >> --- >> drivers/perf/resctrl_pmu.c | 27 ++++++++++++++++++++++++--- >> 1 file changed, 24 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/perf/resctrl_pmu.c b/drivers/perf/resctrl_pmu.c >> index 99a2b90b5d83..68cda619da48 100644 >> --- a/drivers/perf/resctrl_pmu.c >> +++ b/drivers/perf/resctrl_pmu.c >> @@ -6,6 +6,8 @@ >> #include <linux/perf_event.h> >> #include <linux/platform_device.h> >> #include <linux/resctrl.h> >> +#include <linux/mutex.h> >> +#include <linux/idr.h> >> >> struct resctrl_pmu { >> struct pmu pmu; >> @@ -25,6 +27,9 @@ RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(event, config, 0, 7); >> RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(domain, config, 16, 23); >> RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(resctrl_id, config1, 0, 63); >> >> +static DEFINE_MUTEX(resctrl_pmu_lock); >> +static DEFINE_IDR(resctrl_pmu_idr); >> + >> static void resctrl_pmu_do_nothing(struct pmu *pmu) >> { >> } >> @@ -69,12 +74,17 @@ static void resctrl_pmu_event_destroy(struct perf_event *event) >> struct hw_perf_event *hwc = &event->hw; >> u16 event_num = get_event(event); >> struct rdt_resource *r; >> + void *mon_ctx; >> >> r = resctrl_event_get_resource(event_num); >> if (!r) >> return; >> >> - resctrl_arch_mon_ctx_free(r, event_num, hwc->idx); >> + mutex_lock(&resctrl_pmu_lock); >> + mon_ctx = idr_remove(&resctrl_pmu_idr, hwc->idx); >> + mutex_unlock(&resctrl_pmu_lock); >> + >> + resctrl_arch_mon_ctx_free(r, event_num, mon_ctx); >> } >> >> static int resctrl_pmu_event_init(struct perf_event *event) >> @@ -87,6 +97,7 @@ static int resctrl_pmu_event_init(struct perf_event *event) >> struct rdt_domain *d; >> u16 event_num, domain_id; >> u32 closid, rmid; >> + void *mon_ctx; >> int err; >> u64 id; >> >> @@ -144,7 +155,12 @@ static int resctrl_pmu_event_init(struct perf_event *event) >> return -EINVAL; >> } >> >> - hwc->idx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num); >> + mon_ctx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num); >> + >> + mutex_lock(&resctrl_pmu_lock); >> + hwc->idx = idr_alloc(&resctrl_pmu_idr, mon_ctx, 0, INT_MAX, GFP_KERNEL); >> + mutex_unlock(&resctrl_pmu_lock); >> + >> if (hwc->idx == -ENOSPC) >> return -ENOSPC; >> event->destroy = resctrl_pmu_event_destroy; >> @@ -162,6 +178,7 @@ static void resctrl_pmu_event_update(struct perf_event *event) >> struct rdt_resource *r; >> struct rdt_domain *d; >> u32 closid, rmid; >> + void *mon_ctx; >> int err; >> >> event_num = get_event(event); >> @@ -179,11 +196,15 @@ static void resctrl_pmu_event_update(struct perf_event *event) >> if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask)) >> return; >> >> + mutex_lock(&resctrl_pmu_lock); >> + mon_ctx = idr_find(&resctrl_pmu_idr, hwc->idx); >> + mutex_unlock(&resctrl_pmu_lock); >> + >> do { >> prev = local64_read(&hwc->prev_count); >> >> err = resctrl_arch_rmid_read(r, d, closid, rmid, >> - event_num, &now, hwc->idx); >> + event_num, &now, mon_ctx); >> if (err) >> return; >> } while (local64_cmpxchg(&hwc->prev_count, prev, now) != prev);
diff --git a/drivers/perf/resctrl_pmu.c b/drivers/perf/resctrl_pmu.c index 99a2b90b5d83..68cda619da48 100644 --- a/drivers/perf/resctrl_pmu.c +++ b/drivers/perf/resctrl_pmu.c @@ -6,6 +6,8 @@ #include <linux/perf_event.h> #include <linux/platform_device.h> #include <linux/resctrl.h> +#include <linux/mutex.h> +#include <linux/idr.h> struct resctrl_pmu { struct pmu pmu; @@ -25,6 +27,9 @@ RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(event, config, 0, 7); RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(domain, config, 16, 23); RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(resctrl_id, config1, 0, 63); +static DEFINE_MUTEX(resctrl_pmu_lock); +static DEFINE_IDR(resctrl_pmu_idr); + static void resctrl_pmu_do_nothing(struct pmu *pmu) { } @@ -69,12 +74,17 @@ static void resctrl_pmu_event_destroy(struct perf_event *event) struct hw_perf_event *hwc = &event->hw; u16 event_num = get_event(event); struct rdt_resource *r; + void *mon_ctx; r = resctrl_event_get_resource(event_num); if (!r) return; - resctrl_arch_mon_ctx_free(r, event_num, hwc->idx); + mutex_lock(&resctrl_pmu_lock); + mon_ctx = idr_remove(&resctrl_pmu_idr, hwc->idx); + mutex_unlock(&resctrl_pmu_lock); + + resctrl_arch_mon_ctx_free(r, event_num, mon_ctx); } static int resctrl_pmu_event_init(struct perf_event *event) @@ -87,6 +97,7 @@ static int resctrl_pmu_event_init(struct perf_event *event) struct rdt_domain *d; u16 event_num, domain_id; u32 closid, rmid; + void *mon_ctx; int err; u64 id; @@ -144,7 +155,12 @@ static int resctrl_pmu_event_init(struct perf_event *event) return -EINVAL; } - hwc->idx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num); + mon_ctx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num); + + mutex_lock(&resctrl_pmu_lock); + hwc->idx = idr_alloc(&resctrl_pmu_idr, mon_ctx, 0, INT_MAX, GFP_KERNEL); + mutex_unlock(&resctrl_pmu_lock); + if (hwc->idx == -ENOSPC) return -ENOSPC; event->destroy = resctrl_pmu_event_destroy; @@ -162,6 +178,7 @@ static void resctrl_pmu_event_update(struct perf_event *event) struct rdt_resource *r; struct rdt_domain *d; u32 closid, rmid; + void *mon_ctx; int err; event_num = get_event(event); @@ -179,11 +196,15 @@ static void resctrl_pmu_event_update(struct perf_event *event) if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask)) return; + mutex_lock(&resctrl_pmu_lock); + mon_ctx = idr_find(&resctrl_pmu_idr, hwc->idx); + mutex_unlock(&resctrl_pmu_lock); + do { prev = local64_read(&hwc->prev_count); err = resctrl_arch_rmid_read(r, d, closid, rmid, - event_num, &now, hwc->idx); + event_num, &now, mon_ctx); if (err) return; } while (local64_cmpxchg(&hwc->prev_count, prev, now) != prev);
The return pointer from the resctrl_arch_mon_ctx_alloc_no_wait() function is saved in a 32-bit variable 'hwc->idx,' which results in the loss of the upper 32 bits. This, in turn, triggers a kernel panic when attempting to access a corrupted pointer. This patch addresses the issue by utilizing the IDR data structure to keep track of a 32-bit value associated with a pointer (64-bit value). Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com> --- drivers/perf/resctrl_pmu.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)