On Sun, 2 Apr 2023 at 18:45, Dmitry Osipenko
<dmitry.osipenko at collabora.com> wrote:>
> On 3/30/23 20:24, Emil Velikov wrote:
> > Hi Dmitry,
> >
> > Have you considered creating a few DRM helpers for this functionality?
> >
> > AFAICT this is the third driver which supports syncobj timelines and
> > looking at one of the implementations ... it is not great. Note that
> > this suggestion is _not_ a blocker.
>
> Would like to see a third driver starting to use the exactly same
> drm_execbuffer_syncobj struct because UABI part isn't generic, though
> it's a replica of the MSM driver for now.
>
> The virtio-gpu is only at the beginning of starting to use sync objects,
> compared to MSM driver. Will be better to defer the generalization until
> virtio-gpu will become more mature, like maybe after a year since the
> time virtio userspace will start using sync objects, IMO.
>
I wasn't talking about generic UAPI, but having drm helpers instead.
The former (as you pointed out) would need time to crystallize. While
the latter can be done even today.
> ...
> >> +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);
> >
> > Side note: the drm_syncobj_put() called immediately after also calls
> > replace/reset fence internally. Although reading from the docs,
I'm not
> > sure if relying on that is a wise move.
> >
> > Just thought I'd point it out.
>
> The drm_syncobj_put() doesn't call replace/reset fence until syncobj is
> freed. We drop the old fence for active/alive in-syncobj here after
> handling the fence-wait, this makes syncobj reusable, otherwise
> userpsace would have to re-create syncobjs after each submission.
>
I see, thanks.
> >>
> >> + ret = virtio_gpu_parse_deps(&submit);
> >> + if (ret)
> >> + goto cleanup;
> >> +
> >> + ret = virtio_gpu_parse_post_deps(&submit);
> >> + if (ret)
> >> + goto cleanup;
> >> +
> >
> > I think we should zero num_(in|out)_syncobjs when the respective parse
> > fails. Otherwise we get one "cleanup" within the parse
function itself
> > and a second during the cleanup_submit. Haven't looked at it too
closely
> > but I suspect that will trigger an UAF or two.
>
> There are checks for NULL pointers in the code that will prevent the
> UAF. I'll add zeroing of the nums for more consistency.
>
Riiiight the drm_syncobj is attached to the encapsulating struct
virtio_gpu_submit _only_ on success.
By clearing the num variables, the NULL checks will no longer be
needed ... in case you'd want to drop that.
Either way - even as-is the code is safe.
> >> ret = virtio_gpu_install_out_fence_fd(&submit);
> >> if (ret)
> >> goto cleanup;
> >> @@ -294,6 +512,7 @@ int virtio_gpu_execbuffer_ioctl(struct
drm_device *dev, void *data,
> >> goto cleanup;
> >>
> >> virtio_gpu_submit(&submit);
> >> + virtio_gpu_process_post_deps(&submit);
> >
> > Any particular reason why the virtio_gpu_reset_syncobjs is deferred to
> > virtio_gpu_cleanup_submit(). Having it just above the
process_post_deps
> > (similar to msm) allows the reader to get closure about the in
syncobjs.
> >
> > This is just personal preference, so don't read too much into it.
>
> The job submission path should be short as possible in general.
> Technically, virtio_gpu_process_post_deps() should be fast, but since
> I'm not 100% sure about all the corner cases, it's better to hold
until
> job is sent out.
>
Ack, thanks again
-Emil