Messages in this thread | | | Subject | Re: [PATCH 1/2] drm/nouveau/device: avoid usage of list iterator after loop | From | Lyude Paul <> | Date | Tue, 07 Mar 2023 17:49:08 -0500 |
| |
On Wed, 2023-03-01 at 18:25 +0100, Jakob Koschel wrote: > If potentially no valid element is found, 'pstate' would contain an > invalid pointer past the iterator loop. To ensure 'pstate' is always > valid, we only set it if the correct element was found. That allows > adding a BUG_ON in case the code works incorrectly, exposing currently > undetectable potential bugs. > > Additionally, Linus proposed to avoid any use of the list iterator > variable after the loop, in the attempt to move the list iterator > variable declaration into the marcro to avoid any potential misuse after > the loop [1]. > > Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] > Signed-off-by: Jakob Koschel <jkl820.git@gmail.com> > --- > drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c > index ce774579c89d..7c9dd91e98ee 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c > @@ -72,7 +72,7 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control *ctrl, void *data, u32 size) > } *args = data; > struct nvkm_clk *clk = ctrl->device->clk; > const struct nvkm_domain *domain; > - struct nvkm_pstate *pstate; > + struct nvkm_pstate *pstate = NULL, *iter; > struct nvkm_cstate *cstate; > int i = 0, j = -1; > u32 lo, hi; > @@ -103,11 +103,14 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control *ctrl, void *data, u32 size) > return -EINVAL; > > if (args->v0.state != NVIF_CONTROL_PSTATE_ATTR_V0_STATE_CURRENT) { > - list_for_each_entry(pstate, &clk->states, head) { > - if (i++ == args->v0.state) > + list_for_each_entry(iter, &clk->states, head) { > + if (i++ == args->v0.state) { > + pstate = iter; > break; > + } > } > > + BUG_ON(!pstate);
Let's replace this with
if (WARN_ON_ONCE(!pstate)) return -EINVAL;
> lo = pstate->base.domain[domain->name]; > hi = lo; > list_for_each_entry(cstate, &pstate->list, head) { >
-- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
| |