| Message ID | 20191010113937.15962-2-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cpuidle: psci: Support hierarchical CPU arrangement | expand |
On Thu, Oct 10, 2019 at 01:39:25PM +0200, Ulf Hansson wrote: > When the WFI state have been selected, the in-parameter idx to > psci_enter_idle_state() is zero. In this case, we must not index the state > array as "state[idx - 1]", as it means accessing data outside the array. > Fix the bug by pre-checking if idx is zero. > > Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling") > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/cpuidle/cpuidle-psci.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c > index f3c1a2396f98..2e91c8d6c211 100644 > --- a/drivers/cpuidle/cpuidle-psci.c > +++ b/drivers/cpuidle/cpuidle-psci.c > @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state); > static int psci_enter_idle_state(struct cpuidle_device *dev, > struct cpuidle_driver *drv, int idx) > { > - u32 *state = __this_cpu_read(psci_power_state); > + u32 *states = __this_cpu_read(psci_power_state); > + u32 state = idx ? states[idx - 1] : 0; > > - return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, > - idx, state[idx - 1]); > + return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state); Technically we don't dereference that array entry but I agree this is ugly and potentially broken. My preference is aligning it with ACPI code and allocate one more entry in the psci_power_state array (useless for wfi, agreed but at least we remove this (-1) handling from the code). Thanks, Lorenzo > } > > static struct cpuidle_driver psci_idle_driver __initdata = { > -- > 2.17.1 >
On Fri, 18 Oct 2019 at 11:38, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Thu, Oct 10, 2019 at 01:39:25PM +0200, Ulf Hansson wrote: > > When the WFI state have been selected, the in-parameter idx to > > psci_enter_idle_state() is zero. In this case, we must not index the state > > array as "state[idx - 1]", as it means accessing data outside the array. > > Fix the bug by pre-checking if idx is zero. > > > > Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling") > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > drivers/cpuidle/cpuidle-psci.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c > > index f3c1a2396f98..2e91c8d6c211 100644 > > --- a/drivers/cpuidle/cpuidle-psci.c > > +++ b/drivers/cpuidle/cpuidle-psci.c > > @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state); > > static int psci_enter_idle_state(struct cpuidle_device *dev, > > struct cpuidle_driver *drv, int idx) > > { > > - u32 *state = __this_cpu_read(psci_power_state); > > + u32 *states = __this_cpu_read(psci_power_state); > > + u32 state = idx ? states[idx - 1] : 0; > > > > - return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, > > - idx, state[idx - 1]); > > + return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state); > > Technically we don't dereference that array entry but I agree this > is ugly and potentially broken. No sure understand the non-deference part. If the governor selects WFI, the idx will be 0 - and thus we end up using state[-1], doesn't that dereference an invalid address, no? > > My preference is aligning it with ACPI code and allocate one more > entry in the psci_power_state array (useless for wfi, agreed but > at least we remove this (-1) handling from the code). I can do that, but sounds like a slightly bigger change. Are you fine if I do that on top, so we can get this sent as fix for v5.4-rc[n]? Kind regards Uffe
On Fri, Oct 18, 2019 at 11:51:11AM +0200, Ulf Hansson wrote: > On Fri, 18 Oct 2019 at 11:38, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > > > On Thu, Oct 10, 2019 at 01:39:25PM +0200, Ulf Hansson wrote: > > > When the WFI state have been selected, the in-parameter idx to > > > psci_enter_idle_state() is zero. In this case, we must not index the state > > > array as "state[idx - 1]", as it means accessing data outside the array. > > > Fix the bug by pre-checking if idx is zero. > > > > > > Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling") > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > > drivers/cpuidle/cpuidle-psci.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c > > > index f3c1a2396f98..2e91c8d6c211 100644 > > > --- a/drivers/cpuidle/cpuidle-psci.c > > > +++ b/drivers/cpuidle/cpuidle-psci.c > > > @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state); > > > static int psci_enter_idle_state(struct cpuidle_device *dev, > > > struct cpuidle_driver *drv, int idx) > > > { > > > - u32 *state = __this_cpu_read(psci_power_state); > > > + u32 *states = __this_cpu_read(psci_power_state); > > > + u32 state = idx ? states[idx - 1] : 0; > > > > > > - return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, > > > - idx, state[idx - 1]); > > > + return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state); > > > > Technically we don't dereference that array entry but I agree this > > is ugly and potentially broken. > > No sure understand the non-deference part. > > If the governor selects WFI, the idx will be 0 - and thus we end up > using state[-1], doesn't that dereference an invalid address, no? No because CPU_PM_CPU_IDLE_ENTER_PARAM is a macro, the code it preprocesses to won't dereference state[idx - 1] if idx == 0. I agree it is *very* ugly but technically code is not broken. > > My preference is aligning it with ACPI code and allocate one more > > entry in the psci_power_state array (useless for wfi, agreed but > > at least we remove this (-1) handling from the code). > > I can do that, but sounds like a slightly bigger change. Are you fine > if I do that on top, so we can get this sent as fix for v5.4-rc[n]? Technically we are not fixing anything; it is not such a big change, we need to allocate one entry more and update the array indexing. Lorenzo
On Fri, 18 Oct 2019 at 12:03, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Fri, Oct 18, 2019 at 11:51:11AM +0200, Ulf Hansson wrote: > > On Fri, 18 Oct 2019 at 11:38, Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > > > > > > On Thu, Oct 10, 2019 at 01:39:25PM +0200, Ulf Hansson wrote: > > > > When the WFI state have been selected, the in-parameter idx to > > > > psci_enter_idle_state() is zero. In this case, we must not index the state > > > > array as "state[idx - 1]", as it means accessing data outside the array. > > > > Fix the bug by pre-checking if idx is zero. > > > > > > > > Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling") > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > --- > > > > drivers/cpuidle/cpuidle-psci.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c > > > > index f3c1a2396f98..2e91c8d6c211 100644 > > > > --- a/drivers/cpuidle/cpuidle-psci.c > > > > +++ b/drivers/cpuidle/cpuidle-psci.c > > > > @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state); > > > > static int psci_enter_idle_state(struct cpuidle_device *dev, > > > > struct cpuidle_driver *drv, int idx) > > > > { > > > > - u32 *state = __this_cpu_read(psci_power_state); > > > > + u32 *states = __this_cpu_read(psci_power_state); > > > > + u32 state = idx ? states[idx - 1] : 0; > > > > > > > > - return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, > > > > - idx, state[idx - 1]); > > > > + return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state); > > > > > > Technically we don't dereference that array entry but I agree this > > > is ugly and potentially broken. > > > > No sure understand the non-deference part. > > > > If the governor selects WFI, the idx will be 0 - and thus we end up > > using state[-1], doesn't that dereference an invalid address, no? > > No because CPU_PM_CPU_IDLE_ENTER_PARAM is a macro, the code it > preprocesses to won't dereference state[idx - 1] if idx == 0. > > I agree it is *very* ugly but technically code is not broken. Ahh, got it, thanks! > > > > My preference is aligning it with ACPI code and allocate one more > > > entry in the psci_power_state array (useless for wfi, agreed but > > > at least we remove this (-1) handling from the code). > > > > I can do that, but sounds like a slightly bigger change. Are you fine > > if I do that on top, so we can get this sent as fix for v5.4-rc[n]? > > Technically we are not fixing anything; it is not such a big > change, we need to allocate one entry more and update the array > indexing. Okay, let me do the change - and it seems like it doesn't even have to be sent as a fix then. Right? Kind regards Uffe
On Fri, Oct 18, 2019 at 12:29:54PM +0200, Ulf Hansson wrote: [...] > > Technically we are not fixing anything; it is not such a big > > change, we need to allocate one entry more and update the array > > indexing. > > Okay, let me do the change - and it seems like it doesn't even have to > be sent as a fix then. Right? No it does not (even though I agree that's misleading and "fixing" it for v5.4 would not hurt either). Lorenzo
diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c index f3c1a2396f98..2e91c8d6c211 100644 --- a/drivers/cpuidle/cpuidle-psci.c +++ b/drivers/cpuidle/cpuidle-psci.c @@ -27,10 +27,10 @@ static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state); static int psci_enter_idle_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, int idx) { - u32 *state = __this_cpu_read(psci_power_state); + u32 *states = __this_cpu_read(psci_power_state); + u32 state = idx ? states[idx - 1] : 0; - return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, - idx, state[idx - 1]); + return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state); } static struct cpuidle_driver psci_idle_driver __initdata = {
When the WFI state have been selected, the in-parameter idx to psci_enter_idle_state() is zero. In this case, we must not index the state array as "state[idx - 1]", as it means accessing data outside the array. Fix the bug by pre-checking if idx is zero. Fixes: 9ffeb6d08c3a ("PSCI: cpuidle: Refactor CPU suspend power_state parameter handling") Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/cpuidle/cpuidle-psci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)