Received-SPF: pass (google.com: domain of linux-kernel+bounces-84697-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <b8400b7b-0716-4e8c-9ae5-f594778066c5@xs4all.nl>
Date: Wed, 28 Feb 2024 09:45:17 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] media: usbtv: Remove useless locks in usbtv_video_free()
Content-Language: en-US, nl
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>, mchehab@kernel.org,
 tfiga@chromium.org
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
 kernel@collabora.com
References: <20240220080628.13141-1-benjamin.gaignard@collabora.com>
From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
In-Reply-To: <20240220080628.13141-1-benjamin.gaignard@collabora.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 20/02/2024 09:06, Benjamin Gaignard wrote:
> Remove locks calls in usbtv_video_free() because
> are useless and may led to a deadlock as reported here:
> https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")

Hmm, the code was really always wrong, it is just that before this commit
it would only fail if you did a disconnect while streaming, and now it also
fails if you disconnect when you are not streaming, so it is much more
noticeable now.

That should probably be explained better in the commit log. You could add a
second Fixes line:

Fixes: f3d27f34fdd7 ("[media] usbtv: Add driver for Fushicai USBTV007 video frame grabber")

but I am not sure how useful that is.

> ---
>  drivers/media/usb/usbtv/usbtv-video.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
> index 62a583040cd4..96276358d116 100644
> --- a/drivers/media/usb/usbtv/usbtv-video.c
> +++ b/drivers/media/usb/usbtv/usbtv-video.c
> @@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
>  
>  void usbtv_video_free(struct usbtv *usbtv)
>  {
> -	mutex_lock(&usbtv->vb2q_lock);
> -	mutex_lock(&usbtv->v4l2_lock);
> -
>  	usbtv_stop(usbtv);

This call should also be dropped...

>  	vb2_video_unregister_device(&usbtv->vdev);

..since this function will call stop_streaming if streaming is in progress,
which will call usbtv_stop with all the correct locks held.

Regards,

	Hans

>  	v4l2_device_disconnect(&usbtv->v4l2_dev);
>  
> -	mutex_unlock(&usbtv->v4l2_lock);
> -	mutex_unlock(&usbtv->vb2q_lock);
> -
>  	v4l2_device_put(&usbtv->v4l2_dev);
>  }