Hi,
On 2-Dec-24 3:49 PM, Ricardo Ribalda wrote:
> Hi Hans
>
>
> On Mon, 2 Dec 2024 at 15:44, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On 2-Dec-24 3:24 PM, Ricardo Ribalda wrote:
>>> When an async control is written, we copy a pointer to the file handle
>>> that started the operation. That pointer will be used when the device is
>>> done. Which could be anytime in the future.
>>>
>>> If the user closes that file descriptor, its structure will be freed,
>>> and there will be one dangling pointer per pending async control, that
>>> the driver will try to use.
>>>
>>> Clean all the dangling pointers during release().
>>>
>>> To avoid adding a performance penalty in the most common case (no async
>>> operation), a counter has been introduced with some logic to make sure
>>> that it is properly handled.
>>>
>>> Cc: stable@xxxxxxxxxxxxxxx
>>> Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
>>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>>> ---
>>> drivers/media/usb/uvc/uvc_ctrl.c | 52 ++++++++++++++++++++++++++++++++++++++--
>>> drivers/media/usb/uvc/uvc_v4l2.c | 2 ++
>>> drivers/media/usb/uvc/uvcvideo.h | 9 ++++++-
>>> 3 files changed, 60 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>> index 9a80a7d8e73a..af1e38f5c6e9 100644
>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>> @@ -1579,6 +1579,37 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
>>> uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
>>> }
>>>
>>> +static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
>>> +{
>>> + lockdep_assert_held(&handle->chain->ctrl_mutex);
>>> +
>>> + if (ctrl->handle)
>>> + dev_warn_ratelimited(&handle->stream->dev->udev->dev,
>>> + "UVC non compliance: Setting an async control with a pending operation.");
>>> +
>>> + if (handle == ctrl->handle)
>>> + return;
>>> +
>>> + if (ctrl->handle)
>>> + ctrl->handle->pending_async_ctrls--;
>>> +
>>> + ctrl->handle = handle;
>>> + handle->pending_async_ctrls++;
>>> +}
>>
>> Maybe simplify this to:
>
> I do not think that we can do that simplification.
>
> If we do that, the original file handle `pending_async_ctrls` value
> will be out of sync.
Ah good point, I missed that the -- and ++ are done on 2 potentially
different handles.
Regards,
Hans
>> static void uvc_ctrl_get_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
>> {
>> lockdep_assert_held(&handle->chain->ctrl_mutex);
>>
>> if (!ctrl->handle)
>> handle->pending_async_ctrls++;
>> else
>> dev_warn_ratelimited(&handle->stream->dev->udev->dev,
>> "UVC non compliance: Setting an async control with a pending operation.");
>>
>> ctrl->handle = handle;
>> }
>>
>> ?
>>
>> Otherwise the patch looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>
>
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>> +static void uvc_ctrl_put_handle(struct uvc_fh *handle, struct uvc_control *ctrl)
>>> +{
>>> + lockdep_assert_held(&handle->chain->ctrl_mutex);
>>> +
>>> + if (ctrl->handle != handle) /* Nothing to do here.*/
>>> + return;
>>> +
>>> + ctrl->handle = NULL;
>>> + if (WARN_ON(!handle->pending_async_ctrls))
>>> + return;
>>> + handle->pending_async_ctrls--;
>>> +}
>>> +
>>> void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>>> struct uvc_control *ctrl, const u8 *data)
>>> {
>>> @@ -1589,7 +1620,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain,
>>> mutex_lock(&chain->ctrl_mutex);
>>>
>>> handle = ctrl->handle;
>>> - ctrl->handle = NULL;
>>> + if (handle)
>>> + uvc_ctrl_put_handle(handle, ctrl);
>>>
>>> list_for_each_entry(mapping, &ctrl->info.mappings, list) {
>>> s32 value = __uvc_ctrl_get_value(mapping, data);
>>> @@ -1865,7 +1897,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev,
>>>
>>> if (!rollback && handle &&
>>> ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
>>> - ctrl->handle = handle;
>>> + uvc_ctrl_get_handle(handle, ctrl);
>>> }
>>>
>>> return 0;
>>> @@ -2774,6 +2806,22 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>>> return 0;
>>> }
>>>
>>> +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>>> +{
>>> + struct uvc_entity *entity;
>>> +
>>> + guard(mutex)(&handle->chain->ctrl_mutex);
>>> +
>>> + if (!handle->pending_async_ctrls)
>>> + return;
>>> +
>>> + list_for_each_entry(entity, &handle->chain->dev->entities, list)
>>> + for (unsigned int i = 0; i < entity->ncontrols; ++i)
>>> + uvc_ctrl_put_handle(handle, &entity->controls[i]);
>>> +
>>> + WARN_ON(handle->pending_async_ctrls);
>>> +}
>>> +
>>> /*
>>> * Cleanup device controls.
>>> */
>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>> index 97c5407f6603..b425306a3b8c 100644
>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>> @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file)
>>>
>>> uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
>>>
>>> + uvc_ctrl_cleanup_fh(handle);
>>> +
>>> /* Only free resources if this is a privileged handle. */
>>> if (uvc_has_privileges(handle))
>>> uvc_queue_release(&stream->queue);
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 07f9921d83f2..92ecdd188587 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -337,7 +337,11 @@ struct uvc_video_chain {
>>> struct uvc_entity *processing; /* Processing unit */
>>> struct uvc_entity *selector; /* Selector unit */
>>>
>>> - struct mutex ctrl_mutex; /* Protects ctrl.info */
>>> + struct mutex ctrl_mutex; /*
>>> + * Protects ctrl.info,
>>> + * ctrl.handle and
>>> + * uvc_fh.pending_async_ctrls
>>> + */
>>>
>>> struct v4l2_prio_state prio; /* V4L2 priority state */
>>> u32 caps; /* V4L2 chain-wide caps */
>>> @@ -612,6 +616,7 @@ struct uvc_fh {
>>> struct uvc_video_chain *chain;
>>> struct uvc_streaming *stream;
>>> enum uvc_handle_state state;
>>> + unsigned int pending_async_ctrls;
>>> };
>>>
>>> struct uvc_driver {
>>> @@ -797,6 +802,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id,
>>> int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>>> struct uvc_xu_control_query *xqry);
>>>
>>> +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle);
>>> +
>>> /* Utility functions */
>>> struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts,
>>> u8 epaddr);
>>>
>>
>
>
