Re: [PATCH] media: em28xx: keep device state alive for registered video nodes

[Yousef Alhouseen]

Understood. I had not found that series. Please drop my patch; I will
defer to the broader lifetime fix in series 26968.

Thanks,
Yousef

On Mon, 29 Jun 2026 09:45:34 +0200, Hans Verkuil
<hverkuil+cisco@xxxxxxxxxx> wrote:
> On 28/06/2026 02:31, Yousef Alhouseen wrote:
> > The V4L2 core takes a video_device reference before invoking the
> > driver open callback. That reference does not protect em28xx state
> > because all three video_device objects are embedded in em28xx_v4l2 and
> > use video_device_release_empty().
> >
> > If initialization fails after registering a node, the error path can
> > unregister it and drop the last em28xx_v4l2 reference while a concurrent
> > open has passed the core registration check. The open callback then
> > dereferences the freed video_device in video_drvdata(), as observed by
> > KASAN. A disconnect has the same lifetime gap.
> >
> > Give each successfully registered video node references to both the
> > enclosing V4L2 state and the parent em28xx device. Release those
> > references from the video_device release callback, after the core has
> > drained pending opens and existing file references.
>
> This patch series should fix this issue properly:
>
> https://patchwork.linuxtv.org/project/linux-media/list/?series=26968
>
> Rejecting this patch, manually manipulating refcounts is not the way to go.
>
> Regards,
>
> Hans
>
> >
> > Fixes: ef74a0b9ff56 ("[media] em28xx: move video_device structs from struct em28xx to struct v4l2")
> > Reported-by: syzbot+39ff299961a7c07f00f0@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=39ff299961a7c07f00f0
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Yousef Alhouseen <alhouseenyousef@xxxxxxxxx>
> > ---
> > drivers/media/usb/em28xx/em28xx-video.c | 35 +++++++++++++++++++++++--
> > 1 file changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > index da0422c65e5f..4274a9bcb432 100644
> > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > @@ -2289,6 +2289,31 @@ static void em28xx_free_v4l2(struct kref *ref)
> > kfree(v4l2);
> > }
> >
> > +static void em28xx_vdev_release(struct video_device *vdev)
> > +{
> > + struct em28xx_v4l2 *v4l2;
> > + struct em28xx *dev;
> > +
> > + switch (vdev->vfl_type) {
> > + case VFL_TYPE_VIDEO:
> > + v4l2 = container_of(vdev, struct em28xx_v4l2, vdev);
> > + break;
> > + case VFL_TYPE_VBI:
> > + v4l2 = container_of(vdev, struct em28xx_v4l2, vbi_dev);
> > + break;
> > + case VFL_TYPE_RADIO:
> > + v4l2 = container_of(vdev, struct em28xx_v4l2, radio_dev);
> > + break;
> > + default:
> > + WARN_ON_ONCE(1);
> > + return;
> > + }
> > +
> > + dev = v4l2->dev;
> > + kref_put(&v4l2->ref, em28xx_free_v4l2);
> > + kref_put(&dev->ref, em28xx_free_device);
> > +}
> > +
> > /*
> > * em28xx_v4l2_open()
> > * inits the device and starts isoc transfer
> > @@ -2554,7 +2579,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
> > static const struct video_device em28xx_video_template = {
> > .fops = &em28xx_v4l_fops,
> > .ioctl_ops = &video_ioctl_ops,
> > - .release = video_device_release_empty,
> > + .release = em28xx_vdev_release,
> > .tvnorms = V4L2_STD_ALL,
> > };
> >
> > @@ -2583,7 +2608,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
> > static struct video_device em28xx_radio_template = {
> > .fops = &radio_fops,
> > .ioctl_ops = &radio_ioctl_ops,
> > - .release = video_device_release_empty,
> > + .release = em28xx_vdev_release,
> > };
> >
> > /* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
> > @@ -2965,6 +2990,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> > "unable to register video device (error=%i).\n", ret);
> > goto unregister_dev;
> > }
> > + kref_get(&v4l2->ref);
> > + kref_get(&dev->ref);
> >
> > /* Allocate and fill vbi video_device struct */
> > if (em28xx_vbi_supported(dev) == 1) {
> > @@ -2999,6 +3026,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> > "unable to register vbi device\n");
> > goto unregister_dev;
> > }
> > + kref_get(&v4l2->ref);
> > + kref_get(&dev->ref);
> > }
> >
> > if (em28xx_boards[dev->model].radio.type == EM28XX_RADIO) {
> > @@ -3012,6 +3041,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> > "can't register radio device\n");
> > goto unregister_dev;
> > }
> > + kref_get(&v4l2->ref);
> > + kref_get(&dev->ref);
> > dev_info(&dev->intf->dev,
> > "Registered radio device as %s\n",
> > video_device_node_name(&v4l2->radio_dev));