| Message ID | ce92a438-4708-74a0-8fb4-c8fb6495e6b8@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for,v5.12] v4l2-ctrls.c: fix race condition in hdl->requests list | expand |
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@xs4all.nl> >Reported-by: John Cox <jsc66@cam.ac.uk> >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@kynesim.co.uk> Could you also change the report to Reported-by: John Cox <jc@kynesim.co.uk> Many thanks John Cox
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; }
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@xs4all.nl> Reported-by: John Cox <jsc66@cam.ac.uk> Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support") --- drivers/media/v4l2-core/v4l2-ctrls.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)