On Sat, 27 Mar 2021 12:27:40 +0100, you wrote:
>When a request is re-inited it will release all control handler
>objects that are still in the request. It does that by unbinding
>and putting all those objects. When the object is unbound the
>obj->req pointer is set to NULL, and the object's unbind op is
>called. When the object it put the object's release op is called
>to free the memory.
>
>For a request object that contains a control handler that means
>that v4l2_ctrl_handler_free() is called in the release op.
>
>A control handler used in a request has a pointer to the main
>control handler that is created by the driver and contains the
>current state of all controls. If the device is unbound (due to
>rmmod or a forced unbind), then that main handler is freed, again
>by calling v4l2_ctrl_handler_free(), and any outstanding request
>objects that refer to that main handler have to be unbound and put
>as well.
>
>It does that by this test:
>
> if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {
>
>I.e. the handler has no pointer to a request, so is the main
>handler, and one or more request objects refer to this main
>handler.
>
>However, this test is wrong since hdl->req_obj.req is actually
>NULL when re-initing a request (the object unbind will set req to
>NULL), and the only reason this seemingly worked is that the
>requests list is typically empty since the request's unbind op
>will remove the handler from the requests list.
>
>But if another thread is at the same time adding a new control
>to a request, then there is a race condition where one thread
>is removing a control handler object from the requests list and
>another thread is adding one. The result is that hdl->requests
>is no longer empty and the code thinks that a main handler is
>being freed instead of a control handler that is part of a request.
>
>There are two bugs here: first the test for hdl->req_obj.req: this
>should be hdl->req_obj.ops since only the main control handler will
>have a NULL pointer there.
>
>The second is that adding or deleting request objects from the
>requests list of the main handler isn't protected by taking the
>main handler's lock.
>
>Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>Reported-by: John Cox <jsc66@xxxxxxxxx>
>Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support")
>---
> drivers/media/v4l2-core/v4l2-ctrls.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>index 39038c6ad8fb..757d215c2be4 100644
>--- a/drivers/media/v4l2-core/v4l2-ctrls.c
>+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>@@ -2560,7 +2560,15 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> if (hdl == NULL || hdl->buckets == NULL)
> return;
>
>- if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {
>+ /*
>+ * If the main handler is freed and it is used by handler objects in
>+ * outstanding requests, then unbind and put those objects before
>+ * freeing the main handler.
>+ *
>+ * The main handler can be identified by having a NULL ops pointer in
>+ * the request object.
>+ */
>+ if (!hdl->req_obj.ops && !list_empty(&hdl->requests)) {
> struct v4l2_ctrl_handler *req, *next_req;
>
> list_for_each_entry_safe(req, next_req, &hdl->requests, requests) {
>@@ -3634,8 +3642,8 @@ static void v4l2_ctrl_request_unbind(struct media_request_object *obj)
> container_of(obj, struct v4l2_ctrl_handler, req_obj);
> struct v4l2_ctrl_handler *main_hdl = obj->priv;
>
>- list_del_init(&hdl->requests);
> mutex_lock(main_hdl->lock);
>+ list_del_init(&hdl->requests);
> if (hdl->request_is_queued) {
> list_del_init(&hdl->requests_queued);
> hdl->request_is_queued = false;
>@@ -3694,8 +3702,11 @@ static int v4l2_ctrl_request_bind(struct media_request *req,
> if (!ret) {
> ret = media_request_object_bind(req, &req_ops,
> from, false, &hdl->req_obj);
>- if (!ret)
>+ if (!ret) {
>+ mutex_lock(from->lock);
> list_add_tail(&hdl->requests, &from->requests);
>+ mutex_unlock(from->lock);
>+ }
> }
> return ret;
> }
Tested-by: John Cox <jc@xxxxxxxxxxxxx>
Could you also change the report to
Reported-by: John Cox <jc@xxxxxxxxxxxxx>
Many thanks
John Cox
