Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3618a293-4f61-b076-0a9c-c70812436431@collabora.com>
Date:   Sun, 2 Apr 2023 20:45:19 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v4 2/2] drm/virtio: Support sync objects
Content-Language: en-US
To:     Emil Velikov <emil.velikov@collabora.com>
Cc:     David Airlie <airlied@redhat.com>,
        Gerd Hoffmann <kraxel@redhat.com>,
        Gurchetan Singh <gurchetansingh@chromium.org>,
        Chia-I Wu <olvaffe@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
        Rob Clark <robdclark@gmail.com>,
        =?UTF-8?B?TWFyZWsgT2zFocOhaw==?= <maraeo@gmail.com>,
        Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        kernel@collabora.com, virtualization@lists.linux-foundation.org
References: <20230323230755.1094832-1-dmitry.osipenko@collabora.com>
 <20230323230755.1094832-3-dmitry.osipenko@collabora.com>
 <ZCXF4q81wPcczkqx@arch-x395>
From:   Dmitry Osipenko <dmitry.osipenko@collabora.com>
In-Reply-To: <ZCXF4q81wPcczkqx@arch-x395>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

...
>> +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.

>>  
>> +	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.

>>  	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.

Thank you very much for the review! I'll address the rest of comments in v5.

-- 
Best regards,
Dmitry