Re-sending because of my response unintentionally HTML formatted, with correct email address of Tvrtko by the way.
Hi Krzysztof, On Tuesday, 25 November 2025 14:33:38 CET Krzysztof Niemiec wrote: > Initialize eb->vma[].vma pointers to NULL when the eb structure is first > set up. > > During the execution of eb_lookup_vmas(), the eb->vma array is > successively filled up with struct eb_vma objects. This process includes > calling eb_add_vma(), which might fail; however, even in the event of > failure, eb->vma[i].vma is set for the currently processed buffer. > > If eb_add_vma() fails, eb_lookup_vmas() returns with an error, which > prompts a call to eb_release_vmas() to clean up the mess. Since > eb_lookup_vmas() might fail during processing any (possibly not first) > buffer, eb_release_vmas() checks whether a buffer's vma is NULL to know > at what point did the lookup function fail. > > In eb_lookup_vmas(), eb->vma[i].vma is set to NULL if either the helper > function eb_lookup_vma() or eb_validate_vma() fails. eb->vma[i+1].vma is > set to NULL in case i915_gem_object_userptr_submit_init() fails; the > current one needs to be cleaned up by eb_release_vmas() at this point, > so the next one is set. If eb_add_vma() fails, neither the current nor > the next vma is nullified, which is a source of a NULL deref bug > described in [1]. > > When entering eb_lookup_vmas(), the vma pointers are set to the slab > poison value, instead of NULL. Your commit description still doesn't answer my question why the whole memory area allocated to the table of VMAs is not initialized to 0 on allocation, only left populated with poison values. Thanks, Janusz > This doesn't matter for the actual > lookup, since it gets overwritten anyway, however the eb_release_vmas() > function only recognizes NULL as the stopping value, hence the pointers > are being nullified as they go in case of intermediate failure. This > patch changes the approach to filling them all with NULL at the start > instead, rather than handling that manually during failure. > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15062 > Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf") > Reported-by: Gangmin Kim <[email protected]> > Cc: <[email protected]> # 5.16.x > Signed-off-by: Krzysztof Niemiec <[email protected]> > --- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 27 ++++++------------- > 1 file changed, 8 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index b057c2fa03a4..02120203af55 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -951,13 +951,13 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) > vma = eb_lookup_vma(eb, eb->exec[i].handle); > if (IS_ERR(vma)) { > err = PTR_ERR(vma); > - goto err; > + return err; > } > > err = eb_validate_vma(eb, &eb->exec[i], vma); > if (unlikely(err)) { > i915_vma_put(vma); > - goto err; > + return err; > } > > err = eb_add_vma(eb, ¤t_batch, i, vma); > @@ -966,19 +966,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) > > if (i915_gem_object_is_userptr(vma->obj)) { > err = i915_gem_object_userptr_submit_init(vma->obj); > - if (err) { > - if (i + 1 < eb->buffer_count) { > - /* > - * Execbuffer code expects last vma > entry to be NULL, > - * since we already initialized this > entry, > - * set the next value to NULL or we > mess up > - * cleanup handling. > - */ > - eb->vma[i + 1].vma = NULL; > - } > - > + if (err) > return err; > - } > > eb->vma[i].flags |= __EXEC_OBJECT_USERPTR_INIT; > eb->args->flags |= __EXEC_USERPTR_USED; > @@ -986,10 +975,6 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) > } > > return 0; > - > -err: > - eb->vma[i].vma = NULL; > - return err; > } > > static int eb_lock_vmas(struct i915_execbuffer *eb) > @@ -3362,6 +3347,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, > struct sync_file *out_fence = NULL; > int out_fence_fd = -1; > int err; > + int i; > > BUILD_BUG_ON(__EXEC_INTERNAL_FLAGS & ~__I915_EXEC_ILLEGAL_FLAGS); > BUILD_BUG_ON(__EXEC_OBJECT_INTERNAL_FLAGS & > @@ -3375,7 +3361,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, > > eb.exec = exec; > eb.vma = (struct eb_vma *)(exec + args->buffer_count + 1); > - eb.vma[0].vma = NULL; > + > + for (i = 0; i < args->buffer_count; i++) > + eb.vma[i].vma = NULL; > + > eb.batch_pool = NULL; > > eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS; >
