On Fri, Aug 31, 2018 at 11:55 PM Tomasz Figa <tf...@chromium.org> wrote:
>
> Hi Hans,
>
> On Tue, Aug 28, 2018 at 10:49 PM Hans Verkuil <hverk...@xs4all.nl> wrote:
> >
> > From: Hans Verkuil <hans.verk...@cisco.com>
> >
> > When getting control values from a completed request, we have
> > to protect the request against being re-inited why it is
> > being accessed by calling media_request_(un)lock_for_access.
> >
> > Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> > ---
> > drivers/media/v4l2-core/v4l2-ctrls.c | 21 +++++++++++++++------
> > 1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> > b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index ccaf3068de6d..cc266a4a6e88 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -3289,11 +3289,10 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> > struct media_device *mdev,
> > struct v4l2_ext_controls *cs)
> > {
> > struct media_request_object *obj = NULL;
> > + struct media_request *req = NULL;
> > int ret;
> >
> > if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) {
> > - struct media_request *req;
> > -
> > if (!mdev || cs->request_fd < 0)
> > return -EINVAL;
> >
> > @@ -3306,11 +3305,18 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> > struct media_device *mdev,
> > return -EACCES;
> > }
> >
> > + ret = media_request_lock_for_access(req);
> > + if (ret) {
> > + media_request_put(req);
> > + return ret;
> > + }
> > +
> > obj = v4l2_ctrls_find_req_obj(hdl, req, false);
> > - /* Reference to the request held through obj */
> > - media_request_put(req);
> > - if (IS_ERR(obj))
> > + if (IS_ERR(obj)) {
> > + media_request_unlock_for_access(req);
> > + media_request_put(req);
> > return PTR_ERR(obj);
> > + }
> >
> > hdl = container_of(obj, struct v4l2_ctrl_handler,
> > req_obj);
> > @@ -3318,8 +3324,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> > struct media_device *mdev,
> >
> > ret = v4l2_g_ext_ctrls_common(hdl, cs);
> >
> > - if (obj)
> > + if (obj) {
> > + media_request_unlock_for_access(req);
>
> We called media_request_lock_for_access() before looking up obj. Don't
> we also need to call media_request_unlock_for_access() regardless of
> whether obj is non-NULL?
Aha, never mind. I checked the full context and we can't have !obj
here if we operated on a request. Sorry for the noise.
Best regards,
Tomasz
