* [PATCH for v5.18] v4l2-ctrls: unlink all subscribed events
@ 2022-03-17 12:56 Hans Verkuil
2022-03-21 12:09 ` Laurent Pinchart
0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2022-03-17 12:56 UTC (permalink / raw)
To: linux-media
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@xs4all.nl>
---
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;
v4l2_ctrl_lock(ctrl);
- 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);
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);
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH for v5.18] v4l2-ctrls: unlink all subscribed events
2022-03-17 12:56 [PATCH for v5.18] v4l2-ctrls: unlink all subscribed events Hans Verkuil
@ 2022-03-21 12:09 ` Laurent Pinchart
2022-03-21 12:43 ` Hans Verkuil
0 siblings, 1 reply; 3+ messages in thread
From: Laurent Pinchart @ 2022-03-21 12:09 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media
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@xs4all.nl>
> ---
> 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.
>
> 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.
> - 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);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH for v5.18] v4l2-ctrls: unlink all subscribed events
2022-03-21 12:09 ` Laurent Pinchart
@ 2022-03-21 12:43 ` Hans Verkuil
0 siblings, 0 replies; 3+ messages in thread
From: Hans Verkuil @ 2022-03-21 12:43 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
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@xs4all.nl>
>> ---
>> 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);
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-21 12:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-17 12:56 [PATCH for v5.18] v4l2-ctrls: unlink all subscribed events Hans Verkuil
2022-03-21 12:09 ` Laurent Pinchart
2022-03-21 12:43 ` Hans Verkuil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.