On 12/04/2021 12:10, Hans Verkuil wrote:
> When controls are used together with the Request API, then for
> each request a v4l2_ctrl_handler struct is allocated. This contains
> the controls that can be set in a request. If a control is *not* set in
> the request, then the value used in the most recent previous request
> must be used, or the current value if it is not found in any outstanding
> requests.
>
> The framework tried to find such a previous request and it would set
> the 'req' pointer in struct v4l2_ctrl_ref to the v4l2_ctrl_ref of the
> control in such a previous request. So far, so good. However, when that
> previous request was applied to the hardware, returned to userspace, and
> then userspace would re-init or free that request, any 'ref' pointer in
> still-queued requests would suddenly point to freed memory.
>
> This was not noticed before since the drivers that use this expected
> that each request would always have the controls set, so there was
> never any need to find a control in older requests. This requirement
> was relaxed, and now this bug surfaced.
>
> The use of the 'req' pointer in v4l2_ctrl_ref was very fragile, so
> drop this entirely. Instead add a valid_p_req bool to indicate that
> p_req contains a valid value for this control. And if it is false,
> then call the new find_ctrl_in_prev_request() helper function that
> walks through older outstanding requests to find if one of them set
> the same control, and uses that value as per the API specification.
>
> This simplifies the code as well and is a lot more robust.
>
> Also added some more comments, since it was not always clear what
> was going on.
Ignore this patch, I just realized that it can be simplified some more.
I'll post a v2 soon.
Regards,
Hans
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Reported-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> Reported-by: Yunfei Dong (董云飞) <Yunfei.Dong@xxxxxxxxxxxx>
> Tested-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> Tested-by: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>
> Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support")
> Cc: <stable@xxxxxxxxxxxxxxx> # for v5.9 and up
> ---
> While the bug was actually introduced when the Request API was merged,
> it was not in active use until the H.264 stateless codec API was made
> part of the uAPI. That's why the CC to stable explicitly says v5.9 and
> up, since this patch won't apply for older kernels, and that's OK since
> it was not in active use AFAIK.
>
> If that assumption is wrong, then let me know and I will have to create
> custom patches for older kernels, but I prefer to avoid doing that.
>
> I'm not planning to get this merged for 5.12 since we are close to a
> release, so I think it is OK to get this in for 5.13 and have it backported
> to 5.12. I also would like to have feedback from Alexandre and Yunfei
> that this patch indeed fixes your issues as well.
>
> I plan on creating better tests in v4l2-compliance using vivid to verify
> correct behavior of controls in requests. Proper tests are definitely
> needed to avoid bugs like this.
>
> Regards,
>
> Hans
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 124 +++++++++++++--------------
> include/media/v4l2-ctrls.h | 12 +--
> 2 files changed, 69 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index c7bcc5c25771..0627d16f6334 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2504,7 +2504,7 @@ static void new_to_req(struct v4l2_ctrl_ref *ref)
> if (!ref)
> return;
> ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
> - ref->req = ref;
> + ref->valid_p_req = true;
> }
>
> /* Copy the request value to the new value */
> @@ -2512,8 +2512,8 @@ static void req_to_new(struct v4l2_ctrl_ref *ref)
> {
> if (!ref)
> return;
> - if (ref->req)
> - ptr_to_ptr(ref->ctrl, ref->req->p_req, ref->ctrl->p_new);
> + if (ref->valid_p_req)
> + ptr_to_ptr(ref->ctrl, ref->p_req, ref->ctrl->p_new);
> else
> ptr_to_ptr(ref->ctrl, ref->ctrl->p_cur, ref->ctrl->p_new);
> }
> @@ -3694,39 +3694,8 @@ static void v4l2_ctrl_request_queue(struct media_request_object *obj)
> struct v4l2_ctrl_handler *hdl =
> container_of(obj, struct v4l2_ctrl_handler, req_obj);
> struct v4l2_ctrl_handler *main_hdl = obj->priv;
> - struct v4l2_ctrl_handler *prev_hdl = NULL;
> - struct v4l2_ctrl_ref *ref_ctrl, *ref_ctrl_prev = NULL;
>
> mutex_lock(main_hdl->lock);
> - if (list_empty(&main_hdl->requests_queued))
> - goto queue;
> -
> - prev_hdl = list_last_entry(&main_hdl->requests_queued,
> - struct v4l2_ctrl_handler, requests_queued);
> - /*
> - * Note: prev_hdl and hdl must contain the same list of control
> - * references, so if any differences are detected then that is a
> - * driver bug and the WARN_ON is triggered.
> - */
> - mutex_lock(prev_hdl->lock);
> - ref_ctrl_prev = list_first_entry(&prev_hdl->ctrl_refs,
> - struct v4l2_ctrl_ref, node);
> - list_for_each_entry(ref_ctrl, &hdl->ctrl_refs, node) {
> - if (ref_ctrl->req)
> - continue;
> - while (ref_ctrl_prev->ctrl->id < ref_ctrl->ctrl->id) {
> - /* Should never happen, but just in case... */
> - if (list_is_last(&ref_ctrl_prev->node,
> - &prev_hdl->ctrl_refs))
> - break;
> - ref_ctrl_prev = list_next_entry(ref_ctrl_prev, node);
> - }
> - if (WARN_ON(ref_ctrl_prev->ctrl->id != ref_ctrl->ctrl->id))
> - break;
> - ref_ctrl->req = ref_ctrl_prev->req;
> - }
> - mutex_unlock(prev_hdl->lock);
> -queue:
> list_add_tail(&hdl->requests_queued, &main_hdl->requests_queued);
> hdl->request_is_queued = true;
> mutex_unlock(main_hdl->lock);
> @@ -3783,7 +3752,7 @@ v4l2_ctrl_request_hdl_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id)
> {
> struct v4l2_ctrl_ref *ref = find_ref_lock(hdl, id);
>
> - return (ref && ref->req == ref) ? ref->ctrl : NULL;
> + return (ref && ref->valid_p_req) ? ref->ctrl : NULL;
> }
> EXPORT_SYMBOL_GPL(v4l2_ctrl_request_hdl_ctrl_find);
>
> @@ -3972,6 +3941,31 @@ static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
> return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
> }
>
> +/*
> + * Find the most recent queued request that contains the given control.
> + * Return the reference to that control if found, or NULL otherwise.
> + */
> +static struct v4l2_ctrl_ref *
> +find_ctrl_in_prev_request(struct v4l2_ctrl_handler *hdl,
> + struct v4l2_ctrl_ref *ref)
> +{
> + struct v4l2_ctrl_handler *main_hdl = ref->ctrl->handler;
> + u32 id = ref->ctrl->id;
> +
> + /*
> + * The main handler must be locked since we're traversing
> + * the requests_queued list.
> + */
> + WARN_ON(!mutex_is_locked(main_hdl->lock));
> + list_for_each_entry_continue_reverse(hdl, &main_hdl->requests_queued,
> + requests_queued) {
> + ref = find_ref_lock(hdl, id);
> + if (ref && ref->valid_p_req)
> + return ref;
> + }
> + return NULL;
> +}
> +
> /* Get extended controls. Allocates the helpers array if needed. */
> static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
> struct v4l2_ext_controls *cs,
> @@ -3982,8 +3976,10 @@ static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
> int ret;
> int i, j;
> bool def_value;
> + bool is_request;
>
> def_value = (cs->which == V4L2_CTRL_WHICH_DEF_VAL);
> + is_request = (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL);
>
> cs->error_idx = cs->count;
> cs->which = V4L2_CTRL_ID2WHICH(cs->which);
> @@ -4012,6 +4008,7 @@ static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
> int (*ctrl_to_user)(struct v4l2_ext_control *c,
> struct v4l2_ctrl *ctrl);
> struct v4l2_ctrl *master;
> + u32 idx = i;
>
> ctrl_to_user = def_value ? def_to_user : cur_to_user;
>
> @@ -4032,22 +4029,35 @@ static int v4l2_g_ext_ctrls_common(struct v4l2_ctrl_handler *hdl,
> ret = call_op(master, g_volatile_ctrl);
> ctrl_to_user = new_to_user;
> }
> +
> + if (ret) {
> + v4l2_ctrl_unlock(master);
> + break;
> + }
> +
> /* If OK, then copy the current (for non-volatile controls)
> or the new (for volatile controls) control values to the
> caller */
> - if (!ret) {
> - u32 idx = i;
> + do {
> + struct v4l2_ctrl_ref *ref = helpers[idx].ref;
> +
> + /*
> + * If this is part of a request, and the request does
> + * not contain the value of this control, then try to
> + * find it in an older, still queued request. If found,
> + * then use the value from that request, otherwise use
> + * the current control value.
> + */
> + if (is_request && !ref->valid_p_req)
> + ref = find_ctrl_in_prev_request(hdl, ref) ? : ref;
> +
> + if (ref->valid_p_req)
> + ret = req_to_user(cs->controls + idx, ref);
> + else
> + ret = ctrl_to_user(cs->controls + idx, ref->ctrl);
> + idx = helpers[idx].next;
> + } while (!ret && idx);
>
> - do {
> - if (helpers[idx].ref->req)
> - ret = req_to_user(cs->controls + idx,
> - helpers[idx].ref->req);
> - else
> - ret = ctrl_to_user(cs->controls + idx,
> - helpers[idx].ref->ctrl);
> - idx = helpers[idx].next;
> - } while (!ret && idx);
> - }
> v4l2_ctrl_unlock(master);
> }
>
> @@ -4690,8 +4700,6 @@ void v4l2_ctrl_request_complete(struct media_request *req,
> unsigned int i;
>
> if (ctrl->flags & V4L2_CTRL_FLAG_VOLATILE) {
> - ref->req = ref;
> -
> v4l2_ctrl_lock(master);
> /* g_volatile_ctrl will update the current control values */
> for (i = 0; i < master->ncontrols; i++)
> @@ -4701,21 +4709,13 @@ void v4l2_ctrl_request_complete(struct media_request *req,
> v4l2_ctrl_unlock(master);
> continue;
> }
> - if (ref->req == ref)
> + if (ref->valid_p_req)
> continue;
>
> + /* Copy the current control value into the request */
> v4l2_ctrl_lock(ctrl);
> - if (ref->req) {
> - ptr_to_ptr(ctrl, ref->req->p_req, ref->p_req);
> - } else {
> - ptr_to_ptr(ctrl, ctrl->p_cur, ref->p_req);
> - /*
> - * Set ref->req to ensure that when userspace wants to
> - * obtain the controls of this request it will take
> - * this value and not the current value of the control.
> - */
> - ref->req = ref;
> - }
> + ptr_to_ptr(ctrl, ctrl->p_cur, ref->p_req);
> + ref->valid_p_req = true;
> v4l2_ctrl_unlock(ctrl);
> }
>
> @@ -4779,7 +4779,7 @@ int v4l2_ctrl_request_setup(struct media_request *req,
> struct v4l2_ctrl_ref *r =
> find_ref(hdl, master->cluster[i]->id);
>
> - if (r->req && r == r->req) {
> + if (r->valid_p_req) {
> have_new_data = true;
> break;
> }
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index c1d20bd8f25f..a5953b812878 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -304,12 +304,14 @@ struct v4l2_ctrl {
> * the control has been applied. This prevents applying controls
> * from a cluster with multiple controls twice (when the first
> * control of a cluster is applied, they all are).
> - * @req: If set, this refers to another request that sets this control.
> + * @valid_p_req: If set, then p_req contains the control value for the request.
> * @p_req: If the control handler containing this control reference
> * is bound to a media request, then this points to the
> - * value of the control that should be applied when the request
> + * value of the control that must be applied when the request
> * is executed, or to the value of the control at the time
> - * that the request was completed.
> + * that the request was completed. If @valid_p_req is false,
> + * then this control was never set for this request and the
> + * control will not be updated when this request is applied.
> *
> * Each control handler has a list of these refs. The list_head is used to
> * keep a sorted-by-control-ID list of all controls, while the next pointer
> @@ -322,7 +324,7 @@ struct v4l2_ctrl_ref {
> struct v4l2_ctrl_helper *helper;
> bool from_other_dev;
> bool req_done;
> - struct v4l2_ctrl_ref *req;
> + bool valid_p_req;
> union v4l2_ctrl_ptr p_req;
> };
>
> @@ -349,7 +351,7 @@ struct v4l2_ctrl_ref {
> * @error: The error code of the first failed control addition.
> * @request_is_queued: True if the request was queued.
> * @requests: List to keep track of open control handler request objects.
> - * For the parent control handler (@req_obj.req == NULL) this
> + * For the parent control handler (@req_obj.ops == NULL) this
> * is the list header. When the parent control handler is
> * removed, it has to unbind and put all these requests since
> * they refer to the parent.
>
