Hi,
This's a tiny series which replace explicit cpumask var allocation on
stack with *cpumask_var API to achieve neutrality on config and avoid
possible stack overfow.
Dawei Li (2):
net/iucv: Avoid explicit cpumask var allocation on stack
net/dpaa2: Avoid explicit cpumask var allocation on stack
.../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 14 ++++---
net/iucv/iucv.c | 37 +++++++++++++------
2 files changed, 35 insertions(+), 16 deletions(-)
Thanks,
Dawei
--
2.27.0
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.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <[email protected]>
---
net/iucv/iucv.c | 37 ++++++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index a4ab615ca3e3..b51f46ec32f9 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -520,14 +520,19 @@ static void iucv_setmask_mp(void)
*/
static void iucv_setmask_up(void)
{
- cpumask_t cpumask;
+ cpumask_var_t cpumask;
int cpu;
+ if (!alloc_cpumask_var(&cpumask, GFP_KERNEL))
+ return;
+
/* Disable all cpu but the first in cpu_irq_cpumask. */
- cpumask_copy(&cpumask, &iucv_irq_cpumask);
- cpumask_clear_cpu(cpumask_first(&iucv_irq_cpumask), &cpumask);
- for_each_cpu(cpu, &cpumask)
+ cpumask_copy(cpumask, &iucv_irq_cpumask);
+ cpumask_clear_cpu(cpumask_first(&iucv_irq_cpumask), cpumask);
+ for_each_cpu(cpu, cpumask)
smp_call_function_single(cpu, iucv_block_cpu, NULL, 1);
+
+ free_cpumask_var(cpumask);
}
/*
@@ -628,23 +633,33 @@ static int iucv_cpu_online(unsigned int cpu)
static int iucv_cpu_down_prep(unsigned int cpu)
{
- cpumask_t cpumask;
+ cpumask_var_t cpumask;
+ int ret = 0;
if (!iucv_path_table)
return 0;
- cpumask_copy(&cpumask, &iucv_buffer_cpumask);
- cpumask_clear_cpu(cpu, &cpumask);
- if (cpumask_empty(&cpumask))
+ if (!alloc_cpumask_var(&cpumask, GFP_KERNEL))
+ return -ENOMEM;
+
+ cpumask_copy(cpumask, &iucv_buffer_cpumask);
+ cpumask_clear_cpu(cpu, cpumask);
+ if (cpumask_empty(cpumask)) {
/* Can't offline last IUCV enabled cpu. */
- return -EINVAL;
+ ret = -EINVAL;
+ goto __free_cpumask;
+ }
iucv_retrieve_cpu(NULL);
if (!cpumask_empty(&iucv_irq_cpumask))
- return 0;
+ goto __free_cpumask;
+
smp_call_function_single(cpumask_first(&iucv_buffer_cpumask),
iucv_allow_cpu, NULL, 1);
- return 0;
+
+__free_cpumask:
+ free_cpumask_var(cpumask);
+ return ret;
}
/**
--
2.27.0
On Fri, Mar 29, 2024 at 11:57 AM Dawei Li <[email protected]> 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.
>
> Use *cpumask_var API(s) to address it.
>
> Signed-off-by: Dawei Li <[email protected]>
> ---
> net/iucv/iucv.c | 37 ++++++++++++++++++++++++++-----------
> 1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index a4ab615ca3e3..b51f46ec32f9 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -520,14 +520,19 @@ static void iucv_setmask_mp(void)
> */
> static void iucv_setmask_up(void)
> {
> - cpumask_t cpumask;
> + cpumask_var_t cpumask;
> int cpu;
>
> + if (!alloc_cpumask_var(&cpumask, GFP_KERNEL))
> + return;
This can not be right. iucv_setmask_up() is not supposed to fail.
Since iucv_setmask_up() is only called with iucv_register_mutex held,
you could simply add a 'static' for @cpumask variable.
> +
> /* Disable all cpu but the first in cpu_irq_cpumask. */
> - cpumask_copy(&cpumask, &iucv_irq_cpumask);
> - cpumask_clear_cpu(cpumask_first(&iucv_irq_cpumask), &cpumask);
> - for_each_cpu(cpu, &cpumask)
> + cpumask_copy(cpumask, &iucv_irq_cpumask);
> + cpumask_clear_cpu(cpumask_first(&iucv_irq_cpumask), cpumask);
> + for_each_cpu(cpu, cpumask)
> smp_call_function_single(cpu, iucv_block_cpu, NULL, 1);
> +
> + free_cpumask_var(cpumask);
> }
Hi Eric,
On Fri, Mar 29, 2024 at 02:21:28PM +0100, Eric Dumazet wrote:
> On Fri, Mar 29, 2024 at 11:57 AM Dawei Li <[email protected]> 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.
> >
> > Use *cpumask_var API(s) to address it.
> >
> > Signed-off-by: Dawei Li <[email protected]>
> > ---
> > net/iucv/iucv.c | 37 ++++++++++++++++++++++++++-----------
> > 1 file changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> > index a4ab615ca3e3..b51f46ec32f9 100644
> > --- a/net/iucv/iucv.c
> > +++ b/net/iucv/iucv.c
> > @@ -520,14 +520,19 @@ static void iucv_setmask_mp(void)
> > */
> > static void iucv_setmask_up(void)
> > {
> > - cpumask_t cpumask;
> > + cpumask_var_t cpumask;
> > int cpu;
> >
> > + if (!alloc_cpumask_var(&cpumask, GFP_KERNEL))
> > + return;
>
> This can not be right. iucv_setmask_up() is not supposed to fail.
>
> Since iucv_setmask_up() is only called with iucv_register_mutex held,
> you could simply add a 'static' for @cpumask variable.
Correct, iucv_register_mutex is a global lock and can serialize access
on static cpumask var.
I will respin V2 as you suggested.
Thanks,
Dawei
>
>
>
> > +
> > /* Disable all cpu but the first in cpu_irq_cpumask. */
> > - cpumask_copy(&cpumask, &iucv_irq_cpumask);
> > - cpumask_clear_cpu(cpumask_first(&iucv_irq_cpumask), &cpumask);
> > - for_each_cpu(cpu, &cpumask)
> > + cpumask_copy(cpumask, &iucv_irq_cpumask);
> > + cpumask_clear_cpu(cpumask_first(&iucv_irq_cpumask), cpumask);
> > + for_each_cpu(cpu, cpumask)
> > smp_call_function_single(cpu, iucv_block_cpu, NULL, 1);
> > +
> > + free_cpumask_var(cpumask);
> > }
>