On Wed, Nov 04, 2026, Ville Syrjala wrote: > The correct fix would be to not assign the error pointer into > ppgtt->base.pd in the first place. [...] one should always use a > local variable first, and only assign the final pointer after having > checked that the pointer is valid.
Agreed, that's cleaner. v2 does exactly that: gen6_alloc_top_pd()'s result goes into a local first and ppgtt->base.pd is only assigned after validation, so the field can never hold an error pointer. On the failure path it stays NULL, and gen6_ppgtt_free_pd() now bails out on a NULL pd (the cleanup path is reached with nothing assigned). (gen6_alloc_top_pd() itself uses the assign-then-clear pattern for ppgtt->vma you described; this patch doesn't touch that site.) Chengyu On Wed, Nov 4, 2026 at 11:18 PM Ville Syrjälä <[email protected]> wrote: > > On Wed, Nov 04, 2026 at 03:33:10PM +0800, Chengyu Liu wrote: > > gen6_ppgtt_create() stores the result of gen6_alloc_top_pd() into > > ppgtt->base.pd and, on failure, jumps to err_put with the ERR_PTR still > > in the field. i915_vm_put() runs vm->cleanup = gen6_ppgtt_cleanup(), > > which calls gen6_ppgtt_free_pd(), where gen6_for_all_pdes() immediately > > dereferences pd - with an ERR_PTR that is an invalid dereference and a > > crash (in the async release worker). The free_pd() right after is only > > guarded against NULL, not ERR_PTR. > > > > Bail out of gen6_ppgtt_free_pd() for an invalid pd and skip free_pd() > > likewise, so an allocation failure under memory pressure stays a plain > > -ENOMEM instead of an oops. > > The correct fix would be to not assign the error pointer > into ppgtt->base.pd in the first place. > > Looks like a somwhat common bad pattern in this code that > received pointers are assigned directly into their final > location, and if the pointer is an error then the pointer is > cleared. IMO one should never do that because it's very easy > to forget the clearing part. Instead one should always use > local variable first, and only assign the final pointer after > having checked that the pointer is valid. > > > > > Found by static analysis. > > > > Cc: [email protected] > > Signed-off-by: Chengyu Liu <[email protected]> > > --- > > drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > index 438cd4724a..72a9ad4642 100644 > > --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c > > @@ -259,6 +259,9 @@ static void gen6_ppgtt_free_pd(struct gen6_ppgtt *ppgtt) > > struct i915_page_table *pt; > > u32 pde; > > > > + if (IS_ERR_OR_NULL(pd)) > > + return; > > + > > gen6_for_all_pdes(pt, pd, pde) > > if (pt) > > free_pt(&ppgtt->base.vm, pt); > > @@ -271,7 +274,7 @@ static void gen6_ppgtt_cleanup(struct > > i915_address_space *vm) > > gen6_ppgtt_free_pd(ppgtt); > > free_scratch(vm); > > > > - if (ppgtt->base.pd) > > + if (!IS_ERR_OR_NULL(ppgtt->base.pd)) > > free_pd(&ppgtt->base.vm, ppgtt->base.pd); > > > > mutex_destroy(&ppgtt->flush); > > -- > > 2.25.1 > > -- > Ville Syrjälä > Intel
