Hello,
On 4/11/23 14:07, Emil Velikov wrote:
> Hi Dmitry,
>
> On Sun, 9 Apr 2023 at 13:40, Dmitry Osipenko
> <dmitry.osipenko@xxxxxxxxxxxxx> wrote:
>
>> +static void virtio_gpu_free_syncobjs(struct drm_syncobj **syncobjs,
>> + uint32_t nr_syncobjs)
>> +{
>> + uint32_t i = nr_syncobjs;
>> +
>> + while (i--) {
>> + if (syncobjs[i])
>> + drm_syncobj_put(syncobjs[i]);
>> + }
>> +
>> + kvfree(syncobjs);
>> +}
>> +
>
>> +static void virtio_gpu_reset_syncobjs(struct drm_syncobj **syncobjs,
>> + uint32_t nr_syncobjs)
>> +{
>> + uint32_t i;
>> +
>> + for (i = 0; i < nr_syncobjs; i++) {
>> + if (syncobjs[i])
>> + drm_syncobj_replace_fence(syncobjs[i], NULL);
>> + }
>> +}
>> +
>
> Can I bribe you (with cookies) about dropping the NULL checks above?
> They're dead code and rather misleading IMHO.
When userspace doesn't set the VIRTGPU_EXECBUF_SYNCOBJ_RESET flag in
virtio_gpu_parse_deps(), the syncobjs[i] is NULL. Hence not a dead code
at all :)
>> +static void
>> +virtio_gpu_free_post_deps(struct virtio_gpu_submit_post_dep *post_deps,
>> + uint32_t nr_syncobjs)
>> +{
>> + uint32_t i = nr_syncobjs;
>> +
>> + while (i--) {
>> + kfree(post_deps[i].chain);
>> + drm_syncobj_put(post_deps[i].syncobj);
>> + }
>> +
>> + kvfree(post_deps);
>> +}
>> +
>> +static int virtio_gpu_parse_post_deps(struct virtio_gpu_submit *submit)
>> +{
>> + struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
>> + struct drm_virtgpu_execbuffer_syncobj syncobj_desc;
>> + struct virtio_gpu_submit_post_dep *post_deps;
>> + u32 num_out_syncobjs = exbuf->num_out_syncobjs;
>> + size_t syncobj_stride = exbuf->syncobj_stride;
>> + int ret = 0, i;
>> +
>> + if (!num_out_syncobjs)
>> + return 0;
>> +
>> + post_deps = kvcalloc(num_out_syncobjs, sizeof(*post_deps), GFP_KERNEL);
>> + if (!post_deps)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < num_out_syncobjs; i++) {
>> + uint64_t address = exbuf->out_syncobjs + i * syncobj_stride;
>> +
>
> For my own education: what's the specifics/requirements behind the
> stride? is there a use-case for the stride to vary across in/out
> syncobj?
Stride is the same for in/out syncobjs as the same struct
drm_virtgpu_execbuffer_syncobj is used by both.
The out-syncobj don't use the "flags" field of
drm_virtgpu_execbuffer_syncobj. We could use separate strides and desc
for in/out syncobjs, but in practice it's unlikely that we will be
extending the desc anytime soon and usually there are not many
out-syncobj to care about the wasted field.
On the other hand, if we will ever need to extend desc for in-syncobjs,
there will be more wasted fields. Maybe it indeed won't hurt to split
in/out syncobjs, for consistency. I'll think about it for v6, thanks.
> Off the top of my head: userspace could have an array of larger
> structs, each embedding an syncobj. Thus passing the stride, the
> kernel will fetch/update them in-place w/o caring about the other
> data.
> Or perhaps there is another trick that userspace utilises the stride for?
Stride is only about potential future expansion of the struct
drm_virtgpu_execbuffer_syncobj with new fields. There shouldn't be any
special tricks for userspace to use.
>> + if (copy_from_user(&syncobj_desc,
>> + u64_to_user_ptr(address),
>> + min(syncobj_stride, sizeof(syncobj_desc)))) {
>> + ret = -EFAULT;
>> + break;
>> + }
>> +
>
> We seem to be parsing garbage(?) stack data in the syncobj_stride <
> sizeof(syncobj_desc) case.
>
> Zeroing/reseting the syncobj_desc on each iteration is one approach -
> be that fully or in part. Alternatively we could error out on
> syncobj_stride < sizeof(syncobj_desc).
Good catch! It indeed needs to be zeroed. Nothing terrible will happen
today for kernel if it will use garbage data, but a malfunctioning
userspace may happen to appear working properly.
>> + post_deps[i].point = syncobj_desc.point;
>> +
>> + if (syncobj_desc.flags) {
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + if (syncobj_desc.point) {
>> + post_deps[i].chain = dma_fence_chain_alloc();
>> + if (!post_deps[i].chain) {
>> + ret = -ENOMEM;
>> + break;
>> + }
>> + }
>> +
>> + post_deps[i].syncobj = drm_syncobj_find(submit->file,
>> + syncobj_desc.handle);
>> + if (!post_deps[i].syncobj) {
>> + ret = -EINVAL;
>
> I think we want a kfree(chain) here. Otherwise we'll leak it, right?
I'm sure there was a kfree here in one of previous version of the patch.
Another good catch, thanks :)
>> + break;
>> + }
>> + }
>> +
>> + if (ret) {
>> + virtio_gpu_free_post_deps(post_deps, i);
>> + return ret;
>> + }
>> +
>> + submit->num_out_syncobjs = num_out_syncobjs;
>> + submit->post_deps = post_deps;
>> +
>> + return 0;
>> +}
>> +
>
>
> With the two issues in virtio_gpu_parse_post_deps() addressed, the series is:
> Reviewed-by; Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
Thanks you for the review!
--
Best regards,
Dmitry
