On 21/03/2022 13:09, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the patch.
>
> On Thu, Mar 17, 2022 at 01:56:08PM +0100, Hans Verkuil wrote:
>> The v4l2_ctrl_handler_free() function must unlink all subscribed events
>> of the control handler that is being freed, but it only did so for the
>> controls owned by that control handler and not for the controls referred
>> to by that control handler. This leaves stale data in the ev_subs list
>> of those controls.
>>
>> The node list header is also properly initialized and list_del_init is
>> called instead of list_del to ensure that list_empty() can be used
>> to detect whether a v4l2_subscribed_event is part of a list or not.
>>
>> This makes v4l2_ctrl_del_event() more robust since it will not attempt
>> to find the control if the v4l2_subscribed_event has already been unlinked
>> from the control.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>> ---
>> drivers/media/v4l2-core/v4l2-ctrls-api.c | 7 +++++--
>> drivers/media/v4l2-core/v4l2-ctrls-core.c | 6 +++++-
>> drivers/media/v4l2-core/v4l2-event.c | 1 +
>> 3 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
>> index db9baa0bd05f..d64b22cb182c 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
>> @@ -1159,13 +1159,16 @@ static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev,
>>
>> static void v4l2_ctrl_del_event(struct v4l2_subscribed_event *sev)
>> {
>> - struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
>> + struct v4l2_ctrl *ctrl = NULL;
>> +
>> + if (!list_empty(&sev->node))
>> + ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
>>
>> if (!ctrl)
>> return;
>
> I'd go for
>
> if (list_empty(&sdev->node))
> return;
>
> ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
> if (!ctrl)
> return;
>
> and drop the initialization of ctrl to NULL, it's easier to read.
Good point, that's much better.
>
>>
>> v4l2_ctrl_lock(ctrl);
>
> v4l2_ctrl_lock() simply calls
>
> mutex_lock(ctrl->handler->lock);
>
> Is there any case where ctrl->handler != sev->fh->ctrl_handler ? If not
> this could be simplified to drop the v4l2_ctrl_find() call.
Yes, ctrl->handler might be different from sev->fh->ctrl_handler. Specifically
for inherited controls, in fact.
Regards,
Hans
>
>> - list_del(&sev->node);
>> + list_del_init(&sev->node);
>> v4l2_ctrl_unlock(ctrl);
>> }
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> index 8968cec8454e..1a9d60cb119c 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> @@ -10,6 +10,7 @@
>> #include <linux/slab.h>
>> #include <media/v4l2-ctrls.h>
>> #include <media/v4l2-event.h>
>> +#include <media/v4l2-fh.h>
>> #include <media/v4l2-fwnode.h>
>>
>> #include "v4l2-ctrls-priv.h"
>> @@ -1165,13 +1166,16 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>> /* Free all nodes */
>> list_for_each_entry_safe(ref, next_ref, &hdl->ctrl_refs, node) {
>> list_del(&ref->node);
>> + list_for_each_entry_safe(sev, next_sev, &ref->ctrl->ev_subs, node)
>> + if (sev->fh->ctrl_handler == hdl)
>> + list_del_init(&sev->node);
>
> I have no idea how control reference works or what they're used for, so
> I can't tell if this is safe without locking ref->ctrl.
>
>> kfree(ref);
>> }
>> /* Free all controls owned by the handler */
>> list_for_each_entry_safe(ctrl, next_ctrl, &hdl->ctrls, node) {
>> list_del(&ctrl->node);
>> list_for_each_entry_safe(sev, next_sev, &ctrl->ev_subs, node)
>> - list_del(&sev->node);
>> + list_del_init(&sev->node);
>> kvfree(ctrl);
>> }
>> kvfree(hdl->buckets);
>> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
>> index c5ce9f11ad7b..2fd9f7187c04 100644
>> --- a/drivers/media/v4l2-core/v4l2-event.c
>> +++ b/drivers/media/v4l2-core/v4l2-event.c
>> @@ -246,6 +246,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>> sev->fh = fh;
>> sev->ops = ops;
>> sev->elems = elems;
>> + INIT_LIST_HEAD(&sev->node);
>>
>> mutex_lock(&fh->subscribe_lock);
>
