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@xxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx # 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@xxxxxxxxx/ > > 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); >
