lkml.org 
[lkml]   [2024]   [Apr]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 04/10] perf/arm_cspmu: Avoid placing cpumask var on stack
    On Wed, Apr 03, 2024 at 08:51:03PM +0800, Dawei Li wrote:
    > For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
    > variable on stack is not recommended since it can cause potential stack
    > overflow.
    >
    > Instead, kernel code should always use *cpumask_var API(s) to allocate
    > cpumask var in config-neutral way, leaving allocation strategy to
    > CONFIG_CPUMASK_OFFSTACK.
    >
    > But dynamic allocation in cpuhp's teardown callback is somewhat problematic
    > for if allocation fails(which is unlikely but still possible):
    > - If -ENOMEM is returned to caller, kernel crashes for non-bringup
    > teardown;
    > - If callback pretends nothing happened and returns 0 to caller, it may
    > trap system into an in-consisitent/compromised state;
    >
    > Use newly-introduced cpumask_any_and_but() to address all issues above.
    > It eliminates usage of temporary cpumask var in generic way, no matter how
    > the cpumask var is allocated.
    >
    > Suggested-by: Mark Rutland <mark.rutland@arm.com>
    >
    > Signed-off-by: Dawei Li <dawei.li@shingroup.cn>

    The logic looks good to me, but I'd like the commit message updated the same as
    per my comment on patch 2.

    With that commit message:

    Reviewed-by: Mark Rutland <mark.rutland@arm.com>

    Mark.

    > ---
    > drivers/perf/arm_cspmu/arm_cspmu.c | 8 +++-----
    > 1 file changed, 3 insertions(+), 5 deletions(-)
    >
    > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
    > index b9a252272f1e..fd1004251665 100644
    > --- a/drivers/perf/arm_cspmu/arm_cspmu.c
    > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
    > @@ -1322,8 +1322,7 @@ static int arm_cspmu_cpu_online(unsigned int cpu, struct hlist_node *node)
    >
    > static int arm_cspmu_cpu_teardown(unsigned int cpu, struct hlist_node *node)
    > {
    > - int dst;
    > - struct cpumask online_supported;
    > + unsigned int dst;
    >
    > struct arm_cspmu *cspmu =
    > hlist_entry_safe(node, struct arm_cspmu, cpuhp_node);
    > @@ -1333,9 +1332,8 @@ static int arm_cspmu_cpu_teardown(unsigned int cpu, struct hlist_node *node)
    > return 0;
    >
    > /* Choose a new CPU to migrate ownership of the PMU to */
    > - cpumask_and(&online_supported, &cspmu->associated_cpus,
    > - cpu_online_mask);
    > - dst = cpumask_any_but(&online_supported, cpu);
    > + dst = cpumask_any_and_but(&cspmu->associated_cpus,
    > + cpu_online_mask, cpu);
    > if (dst >= nr_cpu_ids)
    > return 0;
    >
    > --
    > 2.27.0
    >

    \
     
     \ /
      Last update: 2024-05-27 16:22    [W:5.393 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site