Re: [PATCH i-g-t v2] tests/intel/gem_exec_params: add invalid-offset-multiple-buffers test

[Krzysztof Niemiec <krzysztof.niemiec@intel.com>]

Hi Krzysztof

On 2026-08-20 at 07:15:23 +0000, Krzysztof Karas wrote:
> Hi Krzysztof,
> 
> On 2026-08-19 at 17:21:09 +0200, Krzysztof Niemiec wrote:
> > Add coverage for the case where the batch start offset is invalid with
> > multiple BOs submitted, which was buggy in the kernel until 4fe2bd195435
> > ("drm/i915/gem: Zero-initialize the eb.vma array in i915_gem_do_execbuffer")
> > 
> > Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> > ---
> > 
> > v2:
> >  - Reword comments to be more specific (Krzysztof K, Sebastian)
> >  - Get rid of the context id parameter (previously set in rsvd2, which
> >    should have been rsvd1) - which turns out isn't actually required to
> >    trigger the correct error path (Sebastian)
> >  - Capitalize sentences (Andi, off-list)
> > 
> >  tests/intel/gem_exec_params.c | 50 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> > 
> > diff --git a/tests/intel/gem_exec_params.c b/tests/intel/gem_exec_params.c
> > index 3ba4c530b..c405190a0 100644
> > --- a/tests/intel/gem_exec_params.c
> > +++ b/tests/intel/gem_exec_params.c
> > @@ -85,6 +85,8 @@
> >   *
> >   * SUBTEST: invalid-flag
> >   *
> > + * SUBTEST: invalid-offset-multiple-buffers
> > + *
> >   * SUBTEST: invalid-ring
> >   *
> >   * SUBTEST: invalid-ring2
> > @@ -404,6 +406,51 @@ static void test_invalid_batch_start(int fd)
> >  	gem_close(fd, exec.handle);
> >  }
> >  
> > +/*
> > + * This test is checking for a NULL deref in eb_release_vmas()
> > + * (see 4fe2bd195435 for kernel fix), which happens when multiple buffers
> > + * are submitted to the execbuf ioctl and some of them are invalid.
> > + *
> > + * The __gem_execbuf() call is always expected to fail with an -EINVAL,
> > + * because the bug mentioned above is in the error path, which is
> > + * deliberately triggered. Affected systems, however, will not handle it
> > + * cleanly and will crash with a NULL deref.
> > + */
> > +
> > +static void test_invalid_offset_multiple_buffers(int fd)
> > +{
> > +	struct drm_i915_gem_exec_object2 exec[2];
> > +	struct drm_i915_gem_execbuffer2 execbuf;
> > +	uint32_t size = 0x1000;
> Why this particular size?
> 

gem_create ioctl rounds up size to "the largest of minimum page sizes in the
specified memory regions" (in this case simply PAGE_SIZE), then checks
the alignment to PAGE_SIZE anyway. So this can be anything as long as it is
not 0, and most of the tests I see default to 4096 byte objects with no
additional explanation.

If 0x1000 is less readable than 4096 then I can change it

> > +
> > +	memset(exec, 0, sizeof(exec));
> > +	memset(&execbuf, 0, sizeof(execbuf));
> > +
> > +	exec[0].handle = batch_create_size(fd, size);
> > +	exec[1].handle = batch_create_size(fd, size);
> > +
> > +	/*
> > +	 * To test 4fe2bd195435, we need multiple buffers submitted and
> > +	 * eb_add_vma() to fail. eb_add_vma() checks whether
> > +	 * batch_start_offset + batch_len is smaller than the vma size, and
> > +	 * fails with -EINVAL if it isn't. Submitting two buffers and setting
> > +	 * the offset to the size of the first batch will trigger this
> > +	 * error path.
> > +	 */
> > +
> > +	execbuf.buffers_ptr = to_user_pointer(exec);
> > +	execbuf.buffer_count = 2;
> > +	execbuf.batch_start_offset = size;
> Would setting any large value as offset trigger the desired
> error path? Is using "size" here significant?
> 

batch_start_offset >= size triggers the error path. The actual check I'm
targeting [1] uses range_overflows_t() to check the interval
[batch_start_offset, batch_start_offset + batch_len), if it lies in [0, size)
(both right-open). AKA it checks if what we say is the batch is within the
batch's vma.

Any value of batch_start_offset >= size will fail the range_overflows() check,
triggering the failure (see [2], "Any range starting at or beyond @max
is considered an overflow", batch_len needn't be set with start_offset >=
size). I think this is the most straightforward way to fail eb_add_vma()

[1] https://elixir.bootlin.com/linux/v7.2/source/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c#L608-L614
[2] https://elixir.bootlin.com/linux/v7.2/source/include/linux/overflow.h#L235-L268

Thanks
Krzysztof

> > +
> > +	igt_assert_eq(__gem_execbuf(fd, &execbuf), -EINVAL);
> > +
> > +	gem_sync(fd, exec[0].handle);
> > +	gem_sync(fd, exec[1].handle);
> > +
> > +	gem_close(fd, exec[0].handle);
> > +	gem_close(fd, exec[1].handle);
> > +}
> > +
> >  static void test_larger_than_life_batch(int fd)
> >  {
> >  	const struct intel_execution_engine2 *e;
> > @@ -705,6 +752,9 @@ int igt_main()
> >  	igt_subtest("larger-than-life-batch")
> >  		test_larger_than_life_batch(fd);
> >  
> > +	igt_subtest("invalid-offset-multiple-buffers")
> > +		test_invalid_offset_multiple_buffers(fd);
> > +
> >  #define DIRT(name) \
> >  	igt_subtest(#name "-dirt") { \
> >  		execbuf.flags = 0; \
> > -- 
> > 2.55.0
> > 
> 
> -- 
> Best Regards,
> Krzysztof