[Intel-gfx] [PATCH] drm/i915/execbuf: don't allow zero batch_len

[Chris Wilson]

Quoting Matthew Auld (2020-10-13 15:07:46)
> On 13/10/2020 12:58, Chris Wilson wrote:
> > Quoting Matthew Auld (2020-10-13 12:18:39)
> >> As per the ABI batch_len is u32, however if the batch_len is left unset,
> >> then the kernel will just assume batch_len is the size of the whole
> >> batch object, however since the vma->size is u64, while the batch_len is
> >> just u32 we can end up with batch_len = 0 if we are given too large batch
> >> object(e.g 1ULL << 32), which doesn't look the intended behaviour and
> >> probably leads to explosions on some HW.
> >>
> >> Testcase: igt/gem_exec_params/larger-than-life-batch
> >> Fixes: 0b5372727be3 ("drm/i915/cmdparser: Use cached vmappings")
> > 
> > Nah. That's setting exec_len used for dispatch, not for parsing, which
> > is still using
> > 
> > i915_gem_execbuffer_parse(engine, &shadow_exec_entry,
> >                         params->batch->obj,
> >                         eb,
> >                         args->batch_start_offset,
> >                         args->batch_len,
> >                         drm_is_current_master(file));
> > (and args->batch_len is straight from userspace and passed onwards)
> > 
> > It's right up until 435e8fc059db ("drm/i915: Allow parsing of unsized batches")
> > where we are using the user value of batch_len for allocating the shadow
> > object and parsing.
> > 
> > Fixes: 435e8fc059db ("drm/i915: Allow parsing of unsized batches")
> 
> On the topic of that patch, why is it looking at args->batch_len(might 
> be zero), even though it looks like it is trying to move the 
> eb->batch_len calculation to before we call eb_use_cmdparser(), so it 
> can use it(the commit message seems to suggest that?), but then it only 
> looks at the args version anyway. I don't get it.

iirc, it was so that we could change the order around and later modify
eb.batch_len before eb_use_cmdparser() [so eb.batch_len no longer would
be zero, defeating the cheat].
-Chris