| Message ID | d399804a-3c18-4514-9eec-69b0935fef71@xs4all.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] media: v4l2-ctrls: unlink all subscribed events | expand |
On 22/11/2024 14:51, 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@xs4all.nl> > Cc: stable@vger.kernel.org # 6.7.x Please note that this caused a kernel oops when testing with virtme. So this needs more work. Regards, Hans > --- > This is v2 of a very old patch from 2022: > > https://patchwork.linuxtv.org/project/linux-media/patch/66246ea5-2bd7-6c9e-56c8-9d683ec58ffc@xs4all.nl/ > > It looks like I forgot to follow-up on Laurent's comments. Recently > we had to upgrade to a newer kernel and apply this same patch again > since it was still failing, at which point I realized that this fix > wasn't in mainline. > > This issue only happens if you have a larger pipeline consisting of > several subdevs, and one of the subdevs is forcibly unbound. > > Changes since v1: > - fixed Laurent's comments > - add locking > --- > drivers/media/v4l2-core/v4l2-ctrls-api.c | 8 ++++++-- > drivers/media/v4l2-core/v4l2-ctrls-core.c | 10 +++++++++- > drivers/media/v4l2-core/v4l2-event.c | 1 + > 3 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > index 95a2202879d8..96c3e40f589f 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > @@ -1249,13 +1249,17 @@ 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; > > + if (list_empty(&sev->node)) > + return; > + > + 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 eeab6a5eb7ba..9fb9b81a8a4f 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" > @@ -1569,13 +1570,20 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl) > list_del(&ref->node); > if (ref->p_req_array_alloc_elems) > kvfree(ref->p_req.p); > + if (ref->ctrl->handler != hdl) { > + mutex_lock(ref->ctrl->handler->lock); > + list_for_each_entry_safe(sev, next_sev, &ref->ctrl->ev_subs, node) > + if (sev->fh->ctrl_handler == hdl) > + list_del_init(&sev->node); > + mutex_unlock(ref->ctrl->handler->lock); > + } > 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->p_array); > kvfree(ctrl); > } > diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c > index 3898ff7edddb..3ad6318c9908 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->flags = sub->flags; > sev->fh = fh; > sev->ops = ops; > + INIT_LIST_HEAD(&sev->node); > > mutex_lock(&fh->subscribe_lock); >
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c index 95a2202879d8..96c3e40f589f 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c @@ -1249,13 +1249,17 @@ 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; + if (list_empty(&sev->node)) + return; + + 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 eeab6a5eb7ba..9fb9b81a8a4f 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" @@ -1569,13 +1570,20 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl) list_del(&ref->node); if (ref->p_req_array_alloc_elems) kvfree(ref->p_req.p); + if (ref->ctrl->handler != hdl) { + mutex_lock(ref->ctrl->handler->lock); + list_for_each_entry_safe(sev, next_sev, &ref->ctrl->ev_subs, node) + if (sev->fh->ctrl_handler == hdl) + list_del_init(&sev->node); + mutex_unlock(ref->ctrl->handler->lock); + } 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->p_array); kvfree(ctrl); } diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c index 3898ff7edddb..3ad6318c9908 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->flags = sub->flags; sev->fh = fh; sev->ops = ops; + INIT_LIST_HEAD(&sev->node); mutex_lock(&fh->subscribe_lock);
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@xs4all.nl> Cc: stable@vger.kernel.org # 6.7.x --- This is v2 of a very old patch from 2022: https://patchwork.linuxtv.org/project/linux-media/patch/66246ea5-2bd7-6c9e-56c8-9d683ec58ffc@xs4all.nl/ It looks like I forgot to follow-up on Laurent's comments. Recently we had to upgrade to a newer kernel and apply this same patch again since it was still failing, at which point I realized that this fix wasn't in mainline. This issue only happens if you have a larger pipeline consisting of several subdevs, and one of the subdevs is forcibly unbound. Changes since v1: - fixed Laurent's comments - add locking --- drivers/media/v4l2-core/v4l2-ctrls-api.c | 8 ++++++-- drivers/media/v4l2-core/v4l2-ctrls-core.c | 10 +++++++++- drivers/media/v4l2-core/v4l2-event.c | 1 + 3 files changed, 16 insertions(+), 3 deletions(-)