2021-04-08 12:13:12

by Igor Torrente

[permalink] [raw]
Subject: [PATCH] media: em28xx: Fix race condition between open and init function

Fixes a race condition - for lack of a more precise term - between
em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
media_pad and vdev structs from the em28xx_v4l2, and managing the
lifetime of those objects more dynamicaly.

The race happens when a thread[1] - containing the em28xx_v4l2_init()
code - calls the v4l2_mc_create_media_graph(), and it return a error,
if a thread[2] - running v4l2_open() - pass the verification point
and reaches the em28xx_v4l2_open() before the thread[1] finishes
the v4l2 subsystem deregistration, thread[1] will free all resources
before the em28xx_v4l2_open() can process their things,
because the em28xx_v4l2_init() has the dev->lock. And all this lead
the thread[2] to cause a user-after-free.

Reported-and-tested-by: [email protected]
Signed-off-by: Igor Matheus Andrade Torrente <[email protected]>
---
drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
drivers/media/usb/em28xx/em28xx-video.c | 188 ++++++++++++++---------
drivers/media/usb/em28xx/em28xx.h | 6 +-
3 files changed, 123 insertions(+), 75 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
index d1e66b503f4d..436c5a8cbbb6 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_xtal = 4300000;
pdata.xtal = v4l2->sensor_xtal;
if (NULL ==
- v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
+ v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
&mt9v011_info, NULL))
return -ENODEV;
v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
@@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_yres = 480;

subdev =
- v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
+ v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
&ov2640_info, NULL);
if (!subdev)
return -ENODEV;
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 6b84c3413e83..e1febb2bf06b 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
*/
static void em28xx_wake_i2c(struct em28xx *dev)
{
- struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;

v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
@@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct em28xx *dev)
struct em28xx_v4l2 *v4l2 = dev->v4l2;
int ret, i;

+ v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
+ if (!v4l2->video_pad) {
+ dev_err(&dev->intf->dev,
+ "failed to allocate video pad memory!\n");
+ v4l2->vdev->entity.num_pads = 0;
+ return;
+ }
+
/* Initialize Video, VBI and Radio pads */
- v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
- ret = media_entity_pads_init(&v4l2->vdev.entity, 1, &v4l2->video_pad);
+ v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
+ ret = media_entity_pads_init(&v4l2->vdev->entity, 1, v4l2->video_pad);
if (ret < 0)
dev_err(&dev->intf->dev,
"failed to initialize video media entity!\n");
@@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
f.type = V4L2_TUNER_RADIO;
else
f.type = V4L2_TUNER_ANALOG_TV;
- v4l2_device_call_all(&v4l2->v4l2_dev,
+ v4l2_device_call_all(v4l2->v4l2_dev,
0, tuner, s_frequency, &f);

/* Enable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
}

v4l2->streaming_users++;
@@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct vb2_queue *vq)

if (v4l2->streaming_users-- == 1) {
/* Disable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);

/* Last active user, so shutdown all the URBS */
em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
@@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue *vq)

if (v4l2->streaming_users-- == 1) {
/* Disable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);

/* Last active user, so shutdown all the URBS */
em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
@@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)

static void video_mux(struct em28xx *dev, int index)
{
- struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;

dev->ctl_input = index;
dev->ctl_ainput = INPUT(index)->amux;
@@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file, void *priv, v4l2_std_id *norm)
{
struct em28xx *dev = video_drvdata(file);

- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd, norm);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);

return 0;
}
@@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
&v4l2->hscale, &v4l2->vscale);

em28xx_resolution_set(dev);
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);

return 0;
}
@@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void *priv,
p->parm.capture.readbuffers = EM28XX_MIN_BUF;
p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
if (dev->is_webcam) {
- rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
+ rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
video, g_frame_interval, &ival);
if (!rc)
p->parm.capture.timeperframe = ival.interval;
@@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void *priv,
memset(&p->parm, 0, sizeof(p->parm));
p->parm.capture.readbuffers = EM28XX_MIN_BUF;
p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
- rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
+ rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
video, s_frame_interval, &ival);
if (!rc)
p->parm.capture.timeperframe = ival.interval;
@@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file, void *priv,
if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
i->type = V4L2_INPUT_TYPE_TUNER;

- i->std = dev->v4l2->vdev.tvnorms;
+ i->std = dev->v4l2->vdev->tvnorms;
/* webcams do not have the STD API */
if (dev->is_webcam)
i->capabilities = 0;
@@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file, void *priv,

strscpy(t->name, "Tuner", sizeof(t->name));

- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
return 0;
}

@@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
if (t->index != 0)
return -EINVAL;

- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
return 0;
}

@@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
if (f->tuner != 0)
return -EINVAL;

- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
v4l2->frequency = new_freq.frequency;

return 0;
@@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file, void *priv,
strscpy(chip->name, "ac97", sizeof(chip->name));
else
strscpy(chip->name,
- dev->v4l2->v4l2_dev.name, sizeof(chip->name));
+ dev->v4l2->v4l2_dev->name, sizeof(chip->name));
return 0;
}

@@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void *priv,

strscpy(t->name, "Radio", sizeof(t->name));

- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);

return 0;
}
@@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void *priv,
if (t->index != 0)
return -EINVAL;

- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);

return 0;
}
@@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
if (mutex_lock_interruptible(&dev->lock))
return -ERESTARTSYS;

+ if (!dev->v4l2) {
+ mutex_unlock(&dev->lock);
+ return -ENODEV;
+ }
+
ret = v4l2_fh_open(filp);
if (ret) {
dev_err(&dev->intf->dev,
@@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)

if (vdev->vfl_type == VFL_TYPE_RADIO) {
em28xx_videodbg("video_open: setting radio device\n");
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
}

kref_get(&dev->ref);
@@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)

mutex_lock(&dev->lock);

- v4l2_device_disconnect(&v4l2->v4l2_dev);
+ v4l2_device_disconnect(v4l2->v4l2_dev);

em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);

@@ -2238,14 +2251,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
video_unregister_device(&v4l2->vbi_dev);
}
- if (video_is_registered(&v4l2->vdev)) {
+ if (video_is_registered(v4l2->vdev)) {
dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
- video_device_node_name(&v4l2->vdev));
- video_unregister_device(&v4l2->vdev);
+ video_device_node_name(v4l2->vdev));
+ video_unregister_device(v4l2->vdev);
}

v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
- v4l2_device_unregister(&v4l2->v4l2_dev);
+ v4l2_device_unregister(v4l2->v4l2_dev);
+ v4l2_device_put(v4l2->v4l2_dev);

kref_put(&v4l2->ref, em28xx_free_v4l2);

@@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
goto exit;

/* Save some power by putting tuner to sleep */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);

/* do this before setting alternate! */
em28xx_set_mode(dev, EM28XX_SUSPEND);
@@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
return 0;
}

+void em28xx_vdev_release(struct video_device *vdev)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+ int i;
+
+ for (i = 0; i < vdev->entity.num_pads; i++)
+ kfree(&vdev->entity.pads[i]);
+#endif
+ kfree(vdev);
+}
+
static const struct v4l2_file_operations em28xx_v4l_fops = {
.owner = THIS_MODULE,
.open = em28xx_v4l2_open,
@@ -2387,7 +2412,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,
};

@@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
const char *type_name)
{
*vfd = *template;
- vfd->v4l2_dev = &dev->v4l2->v4l2_dev;
+ vfd->v4l2_dev = dev->v4l2->v4l2_dev;
vfd->lock = &dev->lock;
if (dev->is_webcam)
vfd->tvnorms = 0;
@@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
{
struct em28xx_v4l2 *v4l2 = dev->v4l2;
- struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
struct tuner_setup tun_setup;
struct v4l2_frequency f;

@@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
}

+void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+ kfree(v4l2_dev);
+}
+
static int em28xx_v4l2_init(struct em28xx *dev)
{
u8 val;
@@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)

v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
if (!v4l2) {
- mutex_unlock(&dev->lock);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto v4l2_error;
}
+
kref_init(&v4l2->ref);
v4l2->dev = dev;
dev->v4l2 = v4l2;

+ v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
+ if (!v4l2->v4l2_dev) {
+ ret = -ENOMEM;
+ goto v4l2_dev_error;
+ }
+
+ v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
+
#ifdef CONFIG_MEDIA_CONTROLLER
- v4l2->v4l2_dev.mdev = dev->media_dev;
+ v4l2->v4l2_dev->mdev = dev->media_dev;
#endif
- ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
+ ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
if (ret < 0) {
dev_err(&dev->intf->dev,
"Call to v4l2_device_register() failed!\n");
- goto err;
+ goto v4l2_device_register_error;
}

hdl = &v4l2->ctrl_handler;
v4l2_ctrl_handler_init(hdl, 8);
- v4l2->v4l2_dev.ctrl_handler = hdl;
+ v4l2->v4l2_dev->ctrl_handler = hdl;

if (dev->is_webcam)
v4l2->progressive = true;
@@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
/* request some modules */

if (dev->has_msp34xx)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"msp3400", 0, msp3400_addrs);

if (dev->board.decoder == EM28XX_SAA711X)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"saa7115_auto", 0, saa711x_addrs);

if (dev->board.decoder == EM28XX_TVP5150)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tvp5150", 0, tvp5150_addrs);

if (dev->board.adecoder == EM28XX_TVAUDIO)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tvaudio", dev->board.tvaudio_addr, NULL);

@@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);

if (dev->board.radio.type)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", dev->board.radio_addr,
NULL);

if (has_demod)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", 0,
v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
@@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
struct v4l2_subdev *sd;

- sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", 0,
v4l2_i2c_tuner_addrs(type));
@@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
if (sd)
tuner_addr = v4l2_i2c_subdev_addr(sd);
} else {
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", tuner_addr, NULL);
}
@@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)

/* set default norm */
v4l2->norm = V4L2_STD_PAL;
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;

/* Analog specific initialization */
@@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
goto unregister_dev;

/* allocate and fill video video_device struct */
- em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
+ v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
+ if (!v4l2->vdev) {
+ ret = -ENOMEM;
+ goto unregister_dev;
+ }
+
+ em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
mutex_init(&v4l2->vb_queue_lock);
mutex_init(&v4l2->vb_vbi_queue_lock);
- v4l2->vdev.queue = &v4l2->vb_vidq;
- v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
- v4l2->vdev.device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
+ v4l2->vdev->queue = &v4l2->vb_vidq;
+ v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
+ v4l2->vdev->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
V4L2_CAP_STREAMING;
if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
- v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
+ v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
if (dev->tuner_type != TUNER_ABSENT)
- v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
-
+ v4l2->vdev->device_caps |= V4L2_CAP_TUNER;

/* disable inapplicable ioctls */
if (dev->is_webcam) {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
} else {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
}
if (dev->tuner_type == TUNER_ABSENT) {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
}
if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
}

/* register v4l2 video video_device */
- ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
+ ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
video_nr[dev->devno]);
if (ret) {
dev_err(&dev->intf->dev,
@@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)

dev_info(&dev->intf->dev,
"V4L2 video device registered as %s\n",
- video_device_node_name(&v4l2->vdev));
+ video_device_node_name(v4l2->vdev));

if (video_is_registered(&v4l2->vbi_dev))
dev_info(&dev->intf->dev,
@@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));

/* Save some power by putting tuner to sleep */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);

/* initialize videobuf2 stuff */
em28xx_vb2_setup(dev);
@@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
video_unregister_device(&v4l2->vbi_dev);
}
- if (video_is_registered(&v4l2->vdev)) {
+ if (video_is_registered(v4l2->vdev)) {
dev_info(&dev->intf->dev,
"V4L2 device %s deregistered\n",
- video_device_node_name(&v4l2->vdev));
- video_unregister_device(&v4l2->vdev);
+ video_device_node_name(v4l2->vdev));
+ video_unregister_device(v4l2->vdev);
}

v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
- v4l2_device_unregister(&v4l2->v4l2_dev);
-err:
+ v4l2_device_unregister(v4l2->v4l2_dev);
+
+v4l2_device_register_error:
+ v4l2_device_put(v4l2->v4l2_dev);
+v4l2_dev_error:
dev->v4l2 = NULL;
kref_put(&v4l2->ref, em28xx_free_v4l2);
+v4l2_error:
mutex_unlock(&dev->lock);
return ret;
}
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 6648e11f1271..dbcc297b5a0d 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -552,10 +552,10 @@ struct em28xx_v4l2 {
struct kref ref;
struct em28xx *dev;

- struct v4l2_device v4l2_dev;
+ struct v4l2_device *v4l2_dev;
struct v4l2_ctrl_handler ctrl_handler;

- struct video_device vdev;
+ struct video_device *vdev;
struct video_device vbi_dev;
struct video_device radio_dev;

@@ -601,7 +601,7 @@ struct em28xx_v4l2 {
unsigned int field_count;

#ifdef CONFIG_MEDIA_CONTROLLER
- struct media_pad video_pad, vbi_pad;
+ struct media_pad *video_pad, vbi_pad;
struct media_entity *decoder;
#endif
};
--
2.20.1


2021-04-08 15:40:50

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] media: em28xx: Fix race condition between open and init function

Hi Igor,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.12-rc6 next-20210408]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Igor-Matheus-Andrade-Torrente/media-em28xx-Fix-race-condition-between-open-and-init-function/20210408-201217
base: git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-a014-20210408 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 56ea2e2fdd691136d5e6631fa0e447173694b82c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/e13d07271a1ee4cbd8ac421bf575a36f9d0e1008
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Igor-Matheus-Andrade-Torrente/media-em28xx-Fix-race-condition-between-open-and-init-function/20210408-201217
git checkout e13d07271a1ee4cbd8ac421bf575a36f9d0e1008
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/media/usb/em28xx/em28xx-video.c:2347:6: warning: no previous prototype for function 'em28xx_vdev_release' [-Wmissing-prototypes]
void em28xx_vdev_release(struct video_device *vdev)
^
drivers/media/usb/em28xx/em28xx-video.c:2347:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void em28xx_vdev_release(struct video_device *vdev)
^
static
>> drivers/media/usb/em28xx/em28xx-video.c:2545:6: warning: no previous prototype for function 'em28xx_v4l2_dev_release' [-Wmissing-prototypes]
void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
^
drivers/media/usb/em28xx/em28xx-video.c:2545:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
^
static
2 warnings generated.


vim +/em28xx_vdev_release +2347 drivers/media/usb/em28xx/em28xx-video.c

2346
> 2347 void em28xx_vdev_release(struct video_device *vdev)
2348 {
2349 #ifdef CONFIG_MEDIA_CONTROLLER
2350 int i;
2351
2352 for (i = 0; i < vdev->entity.num_pads; i++)
2353 kfree(&vdev->entity.pads[i]);
2354 #endif
2355 kfree(vdev);
2356 }
2357
2358 static const struct v4l2_file_operations em28xx_v4l_fops = {
2359 .owner = THIS_MODULE,
2360 .open = em28xx_v4l2_open,
2361 .release = em28xx_v4l2_close,
2362 .read = vb2_fop_read,
2363 .poll = vb2_fop_poll,
2364 .mmap = vb2_fop_mmap,
2365 .unlocked_ioctl = video_ioctl2,
2366 };
2367
2368 static const struct v4l2_ioctl_ops video_ioctl_ops = {
2369 .vidioc_querycap = vidioc_querycap,
2370 .vidioc_enum_fmt_vid_cap = vidioc_enum_fmt_vid_cap,
2371 .vidioc_g_fmt_vid_cap = vidioc_g_fmt_vid_cap,
2372 .vidioc_try_fmt_vid_cap = vidioc_try_fmt_vid_cap,
2373 .vidioc_s_fmt_vid_cap = vidioc_s_fmt_vid_cap,
2374 .vidioc_g_fmt_vbi_cap = vidioc_g_fmt_vbi_cap,
2375 .vidioc_try_fmt_vbi_cap = vidioc_g_fmt_vbi_cap,
2376 .vidioc_s_fmt_vbi_cap = vidioc_g_fmt_vbi_cap,
2377 .vidioc_enum_framesizes = vidioc_enum_framesizes,
2378 .vidioc_enumaudio = vidioc_enumaudio,
2379 .vidioc_g_audio = vidioc_g_audio,
2380 .vidioc_s_audio = vidioc_s_audio,
2381
2382 .vidioc_reqbufs = vb2_ioctl_reqbufs,
2383 .vidioc_create_bufs = vb2_ioctl_create_bufs,
2384 .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
2385 .vidioc_querybuf = vb2_ioctl_querybuf,
2386 .vidioc_qbuf = vb2_ioctl_qbuf,
2387 .vidioc_dqbuf = vb2_ioctl_dqbuf,
2388
2389 .vidioc_g_std = vidioc_g_std,
2390 .vidioc_querystd = vidioc_querystd,
2391 .vidioc_s_std = vidioc_s_std,
2392 .vidioc_g_parm = vidioc_g_parm,
2393 .vidioc_s_parm = vidioc_s_parm,
2394 .vidioc_enum_input = vidioc_enum_input,
2395 .vidioc_g_input = vidioc_g_input,
2396 .vidioc_s_input = vidioc_s_input,
2397 .vidioc_streamon = vb2_ioctl_streamon,
2398 .vidioc_streamoff = vb2_ioctl_streamoff,
2399 .vidioc_g_tuner = vidioc_g_tuner,
2400 .vidioc_s_tuner = vidioc_s_tuner,
2401 .vidioc_g_frequency = vidioc_g_frequency,
2402 .vidioc_s_frequency = vidioc_s_frequency,
2403 .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
2404 .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
2405 #ifdef CONFIG_VIDEO_ADV_DEBUG
2406 .vidioc_g_chip_info = vidioc_g_chip_info,
2407 .vidioc_g_register = vidioc_g_register,
2408 .vidioc_s_register = vidioc_s_register,
2409 #endif
2410 };
2411
2412 static const struct video_device em28xx_video_template = {
2413 .fops = &em28xx_v4l_fops,
2414 .ioctl_ops = &video_ioctl_ops,
2415 .release = em28xx_vdev_release,
2416 .tvnorms = V4L2_STD_ALL,
2417 };
2418
2419 static const struct v4l2_file_operations radio_fops = {
2420 .owner = THIS_MODULE,
2421 .open = em28xx_v4l2_open,
2422 .release = em28xx_v4l2_close,
2423 .unlocked_ioctl = video_ioctl2,
2424 };
2425
2426 static const struct v4l2_ioctl_ops radio_ioctl_ops = {
2427 .vidioc_querycap = vidioc_querycap,
2428 .vidioc_g_tuner = radio_g_tuner,
2429 .vidioc_s_tuner = radio_s_tuner,
2430 .vidioc_g_frequency = vidioc_g_frequency,
2431 .vidioc_s_frequency = vidioc_s_frequency,
2432 .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
2433 .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
2434 #ifdef CONFIG_VIDEO_ADV_DEBUG
2435 .vidioc_g_chip_info = vidioc_g_chip_info,
2436 .vidioc_g_register = vidioc_g_register,
2437 .vidioc_s_register = vidioc_s_register,
2438 #endif
2439 };
2440
2441 static struct video_device em28xx_radio_template = {
2442 .fops = &radio_fops,
2443 .ioctl_ops = &radio_ioctl_ops,
2444 .release = video_device_release_empty,
2445 };
2446
2447 /* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
2448 static unsigned short saa711x_addrs[] = {
2449 0x4a >> 1, 0x48 >> 1, /* SAA7111, SAA7111A and SAA7113 */
2450 0x42 >> 1, 0x40 >> 1, /* SAA7114, SAA7115 and SAA7118 */
2451 I2C_CLIENT_END };
2452
2453 static unsigned short tvp5150_addrs[] = {
2454 0xb8 >> 1,
2455 0xba >> 1,
2456 I2C_CLIENT_END
2457 };
2458
2459 static unsigned short msp3400_addrs[] = {
2460 0x80 >> 1,
2461 0x88 >> 1,
2462 I2C_CLIENT_END
2463 };
2464
2465 /******************************** usb interface ******************************/
2466
2467 static void em28xx_vdev_init(struct em28xx *dev,
2468 struct video_device *vfd,
2469 const struct video_device *template,
2470 const char *type_name)
2471 {
2472 *vfd = *template;
2473 vfd->v4l2_dev = dev->v4l2->v4l2_dev;
2474 vfd->lock = &dev->lock;
2475 if (dev->is_webcam)
2476 vfd->tvnorms = 0;
2477
2478 snprintf(vfd->name, sizeof(vfd->name), "%s %s",
2479 dev_name(&dev->intf->dev), type_name);
2480
2481 video_set_drvdata(vfd, dev);
2482 }
2483
2484 static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
2485 {
2486 struct em28xx_v4l2 *v4l2 = dev->v4l2;
2487 struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
2488 struct tuner_setup tun_setup;
2489 struct v4l2_frequency f;
2490
2491 memset(&tun_setup, 0, sizeof(tun_setup));
2492
2493 tun_setup.mode_mask = T_ANALOG_TV | T_RADIO;
2494 tun_setup.tuner_callback = em28xx_tuner_callback;
2495
2496 if (dev->board.radio.type) {
2497 tun_setup.type = dev->board.radio.type;
2498 tun_setup.addr = dev->board.radio_addr;
2499
2500 v4l2_device_call_all(v4l2_dev,
2501 0, tuner, s_type_addr, &tun_setup);
2502 }
2503
2504 if (dev->tuner_type != TUNER_ABSENT && dev->tuner_type) {
2505 tun_setup.type = dev->tuner_type;
2506 tun_setup.addr = tuner_addr;
2507
2508 v4l2_device_call_all(v4l2_dev,
2509 0, tuner, s_type_addr, &tun_setup);
2510 }
2511
2512 if (dev->board.tda9887_conf) {
2513 struct v4l2_priv_tun_config tda9887_cfg;
2514
2515 tda9887_cfg.tuner = TUNER_TDA9887;
2516 tda9887_cfg.priv = &dev->board.tda9887_conf;
2517
2518 v4l2_device_call_all(v4l2_dev,
2519 0, tuner, s_config, &tda9887_cfg);
2520 }
2521
2522 if (dev->tuner_type == TUNER_XC2028) {
2523 struct v4l2_priv_tun_config xc2028_cfg;
2524 struct xc2028_ctrl ctl;
2525
2526 memset(&xc2028_cfg, 0, sizeof(xc2028_cfg));
2527 memset(&ctl, 0, sizeof(ctl));
2528
2529 em28xx_setup_xc3028(dev, &ctl);
2530
2531 xc2028_cfg.tuner = TUNER_XC2028;
2532 xc2028_cfg.priv = &ctl;
2533
2534 v4l2_device_call_all(v4l2_dev, 0, tuner, s_config, &xc2028_cfg);
2535 }
2536
2537 /* configure tuner */
2538 f.tuner = 0;
2539 f.type = V4L2_TUNER_ANALOG_TV;
2540 f.frequency = 9076; /* just a magic number */
2541 v4l2->frequency = f.frequency;
2542 v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
2543 }
2544
> 2545 void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
2546 {
2547 kfree(v4l2_dev);
2548 }
2549

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (10.33 kB)
.config.gz (45.75 kB)
Download all attachments

2021-04-08 17:38:35

by Shuah Khan

[permalink] [raw]
Subject: Re: [PATCH] media: em28xx: Fix race condition between open and init function

On 4/8/21 6:10 AM, Igor Matheus Andrade Torrente wrote:
> Fixes a race condition - for lack of a more precise term - between
> em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
> media_pad and vdev structs from the em28xx_v4l2, and managing the
> lifetime of those objects more dynamicaly.
>
> The race happens when a thread[1] - containing the em28xx_v4l2_init()
> code - calls the v4l2_mc_create_media_graph(), and it return a error,
> if a thread[2] - running v4l2_open() - pass the verification point
> and reaches the em28xx_v4l2_open() before the thread[1] finishes
> the v4l2 subsystem deregistration, thread[1] will free all resources
> before the em28xx_v4l2_open() can process their things,
> because the em28xx_v4l2_init() has the dev->lock. And all this lead
> the thread[2] to cause a user-after-free.
>

Have you tried this patch with em28xx device? You will have to take into
account the dependencies between the subdevs using the v4l2_dev.

Also try rmmod invidual drivers - what happens if you were to rmmod a
subdev driver? With v4l2_dev is not embedded in v4l2, this could open
up memory leaks or user-after-frees.

> Reported-and-tested-by: [email protected]
> Signed-off-by: Igor Matheus Andrade Torrente <[email protected]>
> ---
> drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
> drivers/media/usb/em28xx/em28xx-video.c | 188 ++++++++++++++---------
> drivers/media/usb/em28xx/em28xx.h | 6 +-
> 3 files changed, 123 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> index d1e66b503f4d..436c5a8cbbb6 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
> v4l2->sensor_xtal = 4300000;
> pdata.xtal = v4l2->sensor_xtal;
> if (NULL ==
> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
> &mt9v011_info, NULL))
> return -ENODEV;
> v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
> @@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
> v4l2->sensor_yres = 480;
>
> subdev =
> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
> &ov2640_info, NULL);
> if (!subdev)
> return -ENODEV;
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 6b84c3413e83..e1febb2bf06b 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
> */
> static void em28xx_wake_i2c(struct em28xx *dev)
> {
> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>
> v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
> v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
> @@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct em28xx *dev)
> struct em28xx_v4l2 *v4l2 = dev->v4l2;
> int ret, i;
>
> + v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
> + if (!v4l2->video_pad) {
> + dev_err(&dev->intf->dev,
> + "failed to allocate video pad memory!\n");
> + v4l2->vdev->entity.num_pads = 0;
> + return;
> + }
> +
> /* Initialize Video, VBI and Radio pads */
> - v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
> - ret = media_entity_pads_init(&v4l2->vdev.entity, 1, &v4l2->video_pad);
> + v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
> + ret = media_entity_pads_init(&v4l2->vdev->entity, 1, v4l2->video_pad);
> if (ret < 0)
> dev_err(&dev->intf->dev,
> "failed to initialize video media entity!\n");
> @@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
> f.type = V4L2_TUNER_RADIO;
> else
> f.type = V4L2_TUNER_ANALOG_TV;
> - v4l2_device_call_all(&v4l2->v4l2_dev,
> + v4l2_device_call_all(v4l2->v4l2_dev,
> 0, tuner, s_frequency, &f);
>
> /* Enable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
> }
>
> v4l2->streaming_users++;
> @@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct vb2_queue *vq)
>
> if (v4l2->streaming_users-- == 1) {
> /* Disable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>
> /* Last active user, so shutdown all the URBS */
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
> @@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue *vq)
>
> if (v4l2->streaming_users-- == 1) {
> /* Disable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>
> /* Last active user, so shutdown all the URBS */
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
> @@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)
>
> static void video_mux(struct em28xx *dev, int index)
> {
> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>
> dev->ctl_input = index;
> dev->ctl_ainput = INPUT(index)->amux;
> @@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file, void *priv, v4l2_std_id *norm)
> {
> struct em28xx *dev = video_drvdata(file);
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd, norm);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);
>
> return 0;
> }
> @@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
> &v4l2->hscale, &v4l2->vscale);
>
> em28xx_resolution_set(dev);
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>
> return 0;
> }
> @@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void *priv,
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> if (dev->is_webcam) {
> - rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
> + rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
> video, g_frame_interval, &ival);
> if (!rc)
> p->parm.capture.timeperframe = ival.interval;
> @@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void *priv,
> memset(&p->parm, 0, sizeof(p->parm));
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> - rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
> + rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
> video, s_frame_interval, &ival);
> if (!rc)
> p->parm.capture.timeperframe = ival.interval;
> @@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file, void *priv,
> if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
> i->type = V4L2_INPUT_TYPE_TUNER;
>
> - i->std = dev->v4l2->vdev.tvnorms;
> + i->std = dev->v4l2->vdev->tvnorms;
> /* webcams do not have the STD API */
> if (dev->is_webcam)
> i->capabilities = 0;
> @@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file, void *priv,
>
> strscpy(t->name, "Tuner", sizeof(t->name));
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> return 0;
> }
>
> @@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
> if (t->index != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> return 0;
> }
>
> @@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
> if (f->tuner != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
> v4l2->frequency = new_freq.frequency;
>
> return 0;
> @@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file, void *priv,
> strscpy(chip->name, "ac97", sizeof(chip->name));
> else
> strscpy(chip->name,
> - dev->v4l2->v4l2_dev.name, sizeof(chip->name));
> + dev->v4l2->v4l2_dev->name, sizeof(chip->name));
> return 0;
> }
>
> @@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void *priv,
>
> strscpy(t->name, "Radio", sizeof(t->name));
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>
> return 0;
> }
> @@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void *priv,
> if (t->index != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>
> return 0;
> }
> @@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
> if (mutex_lock_interruptible(&dev->lock))
> return -ERESTARTSYS;
>
> + if (!dev->v4l2) {
> + mutex_unlock(&dev->lock);
> + return -ENODEV;
> + }
> +
> ret = v4l2_fh_open(filp);
> if (ret) {
> dev_err(&dev->intf->dev,
> @@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
>
> if (vdev->vfl_type == VFL_TYPE_RADIO) {
> em28xx_videodbg("video_open: setting radio device\n");
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
> }
>
> kref_get(&dev->ref);
> @@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>
> mutex_lock(&dev->lock);
>
> - v4l2_device_disconnect(&v4l2->v4l2_dev);
> + v4l2_device_disconnect(v4l2->v4l2_dev);
>
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>
> @@ -2238,14 +2251,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
> video_unregister_device(&v4l2->vbi_dev);
> }
> - if (video_is_registered(&v4l2->vdev)) {
> + if (video_is_registered(v4l2->vdev)) {
> dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
> - video_device_node_name(&v4l2->vdev));
> - video_unregister_device(&v4l2->vdev);
> + video_device_node_name(v4l2->vdev));
> + video_unregister_device(v4l2->vdev);
> }
>
> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
> - v4l2_device_unregister(&v4l2->v4l2_dev);
> + v4l2_device_unregister(v4l2->v4l2_dev);
> + v4l2_device_put(v4l2->v4l2_dev);
>
> kref_put(&v4l2->ref, em28xx_free_v4l2);
>
> @@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
> goto exit;
>
> /* Save some power by putting tuner to sleep */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>
> /* do this before setting alternate! */
> em28xx_set_mode(dev, EM28XX_SUSPEND);
> @@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
> return 0;
> }
>
> +void em28xx_vdev_release(struct video_device *vdev)
> +{
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + int i;
> +
> + for (i = 0; i < vdev->entity.num_pads; i++)
> + kfree(&vdev->entity.pads[i]);
> +#endif
> + kfree(vdev);
> +}
> +
> static const struct v4l2_file_operations em28xx_v4l_fops = {
> .owner = THIS_MODULE,
> .open = em28xx_v4l2_open,
> @@ -2387,7 +2412,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,
> };
>
> @@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
> const char *type_name)
> {
> *vfd = *template;
> - vfd->v4l2_dev = &dev->v4l2->v4l2_dev;
> + vfd->v4l2_dev = dev->v4l2->v4l2_dev;
> vfd->lock = &dev->lock;
> if (dev->is_webcam)
> vfd->tvnorms = 0;
> @@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
> static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
> {
> struct em28xx_v4l2 *v4l2 = dev->v4l2;
> - struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
> struct tuner_setup tun_setup;
> struct v4l2_frequency f;
>
> @@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
> v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
> }
>
> +void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> +{
> + kfree(v4l2_dev);
> +}
> +
> static int em28xx_v4l2_init(struct em28xx *dev)
> {
> u8 val;
> @@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
> if (!v4l2) {
> - mutex_unlock(&dev->lock);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto v4l2_error;
> }
> +
> kref_init(&v4l2->ref);
> v4l2->dev = dev;
> dev->v4l2 = v4l2;
>
> + v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
> + if (!v4l2->v4l2_dev) {
> + ret = -ENOMEM;
> + goto v4l2_dev_error;
> + }
> +
> + v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
> +
> #ifdef CONFIG_MEDIA_CONTROLLER
> - v4l2->v4l2_dev.mdev = dev->media_dev;
> + v4l2->v4l2_dev->mdev = dev->media_dev;
> #endif
> - ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
> + ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
> if (ret < 0) {
> dev_err(&dev->intf->dev,
> "Call to v4l2_device_register() failed!\n");
> - goto err;
> + goto v4l2_device_register_error;
> }
>
> hdl = &v4l2->ctrl_handler;
> v4l2_ctrl_handler_init(hdl, 8);
> - v4l2->v4l2_dev.ctrl_handler = hdl;
> + v4l2->v4l2_dev->ctrl_handler = hdl;
>
> if (dev->is_webcam)
> v4l2->progressive = true;
> @@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> /* request some modules */
>
> if (dev->has_msp34xx)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "msp3400", 0, msp3400_addrs);
>
> if (dev->board.decoder == EM28XX_SAA711X)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "saa7115_auto", 0, saa711x_addrs);
>
> if (dev->board.decoder == EM28XX_TVP5150)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tvp5150", 0, tvp5150_addrs);
>
> if (dev->board.adecoder == EM28XX_TVAUDIO)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tvaudio", dev->board.tvaudio_addr, NULL);
>
> @@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
>
> if (dev->board.radio.type)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", dev->board.radio_addr,
> NULL);

Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.

>
> if (has_demod)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", 0,
> v4l2_i2c_tuner_addrs(ADDRS_DEMOD));

Same here:

Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.

> @@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
> struct v4l2_subdev *sd;
>
> - sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", 0,
> v4l2_i2c_tuner_addrs(type));
> @@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> if (sd)
> tuner_addr = v4l2_i2c_subdev_addr(sd);

Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.

> } else {
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", tuner_addr, NULL);

Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.

> }
> @@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> /* set default norm */
> v4l2->norm = V4L2_STD_PAL;
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
>
> /* Analog specific initialization */
> @@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> goto unregister_dev;
>
> /* allocate and fill video video_device struct */
> - em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
> + v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
> + if (!v4l2->vdev) {
> + ret = -ENOMEM;
> + goto unregister_dev;
> + }
> +
> + em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
> mutex_init(&v4l2->vb_queue_lock);
> mutex_init(&v4l2->vb_vbi_queue_lock);
> - v4l2->vdev.queue = &v4l2->vb_vidq;
> - v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
> - v4l2->vdev.device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
> + v4l2->vdev->queue = &v4l2->vb_vidq;
> + v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
> + v4l2->vdev->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
> V4L2_CAP_STREAMING;
> if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
> - v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
> + v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
> if (dev->tuner_type != TUNER_ABSENT)
> - v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
> -
> + v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
>
> /* disable inapplicable ioctls */
> if (dev->is_webcam) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
> } else {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
> }
> if (dev->tuner_type == TUNER_ABSENT) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
> }
> if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
> }
>
> /* register v4l2 video video_device */
> - ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
> + ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
> video_nr[dev->devno]);
> if (ret) {
> dev_err(&dev->intf->dev,
> @@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> dev_info(&dev->intf->dev,
> "V4L2 video device registered as %s\n",
> - video_device_node_name(&v4l2->vdev));
> + video_device_node_name(v4l2->vdev));
>
> if (video_is_registered(&v4l2->vbi_dev))
> dev_info(&dev->intf->dev,
> @@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
>
> /* Save some power by putting tuner to sleep */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>
> /* initialize videobuf2 stuff */
> em28xx_vb2_setup(dev);
> @@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
> video_unregister_device(&v4l2->vbi_dev);
> }
> - if (video_is_registered(&v4l2->vdev)) {
> + if (video_is_registered(v4l2->vdev)) {
> dev_info(&dev->intf->dev,
> "V4L2 device %s deregistered\n",
> - video_device_node_name(&v4l2->vdev));
> - video_unregister_device(&v4l2->vdev);
> + video_device_node_name(v4l2->vdev));
> + video_unregister_device(v4l2->vdev);
> }
>
> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
> - v4l2_device_unregister(&v4l2->v4l2_dev);
> -err:
> + v4l2_device_unregister(v4l2->v4l2_dev);
> +
> +v4l2_device_register_error:
> + v4l2_device_put(v4l2->v4l2_dev);
> +v4l2_dev_error:
> dev->v4l2 = NULL;
> kref_put(&v4l2->ref, em28xx_free_v4l2);
> +v4l2_error:
> mutex_unlock(&dev->lock);
> return ret;
> }
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 6648e11f1271..dbcc297b5a0d 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -552,10 +552,10 @@ struct em28xx_v4l2 {
> struct kref ref;
> struct em28xx *dev;
>
> - struct v4l2_device v4l2_dev;
> + struct v4l2_device *v4l2_dev;
> struct v4l2_ctrl_handler ctrl_handler;
>
> - struct video_device vdev;
> + struct video_device *vdev;
> struct video_device vbi_dev;
> struct video_device radio_dev;
>
> @@ -601,7 +601,7 @@ struct em28xx_v4l2 {
> unsigned int field_count;
>
> #ifdef CONFIG_MEDIA_CONTROLLER
> - struct media_pad video_pad, vbi_pad;
> + struct media_pad *video_pad, vbi_pad;
> struct media_entity *decoder;
> #endif
> };
>

thanks,
-- Shuah

2021-04-09 13:42:24

by Igor Torrente

[permalink] [raw]
Subject: Re: [PATCH] media: em28xx: Fix race condition between open and init function



On 4/8/21 2:37 PM, Shuah Khan wrote:
> On 4/8/21 6:10 AM, Igor Matheus Andrade Torrente wrote:
>> Fixes a race condition - for lack of a more precise term - between
>> em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
>> media_pad and vdev structs from the em28xx_v4l2, and managing the
>> lifetime of those objects more dynamicaly.
>>
>> The race happens when a thread[1] - containing the em28xx_v4l2_init()
>> code - calls the v4l2_mc_create_media_graph(), and it return a error,
>> if a thread[2] - running v4l2_open() - pass the verification point
>> and reaches the em28xx_v4l2_open() before the thread[1] finishes
>> the v4l2 subsystem deregistration, thread[1] will free all resources
>> before the em28xx_v4l2_open() can process their things,
>> because the em28xx_v4l2_init() has the dev->lock. And all this lead
>> the thread[2] to cause a user-after-free.
>>
>
> Have you tried this patch with em28xx device? You will have to take into
> account the dependencies between the subdevs using the v4l2_dev.
>

No, I didn't because I don't have the device.
I was using the syzbot .c that produces this use-after-free.
I will try to modify the .c to test a complete initialization.

Actually, I forgot to put RFC alongside [PATCH].

> Also try rmmod invidual drivers - what happens if you were to rmmod a
> subdev driver? With v4l2_dev is not embedded in v4l2, this could open
> up memory leaks or user-after-frees.
>

OK. Let me see what I can do.

>> Reported-and-tested-by:
>> [email protected]
>> Signed-off-by: Igor Matheus Andrade Torrente <[email protected]>
>> ---
>>   drivers/media/usb/em28xx/em28xx-camera.c |   4 +-
>>   drivers/media/usb/em28xx/em28xx-video.c  | 188 ++++++++++++++---------
>>   drivers/media/usb/em28xx/em28xx.h        |   6 +-
>>   3 files changed, 123 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
>> b/drivers/media/usb/em28xx/em28xx-camera.c
>> index d1e66b503f4d..436c5a8cbbb6 100644
>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>> @@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
>>           v4l2->sensor_xtal = 4300000;
>>           pdata.xtal = v4l2->sensor_xtal;
>>           if (NULL ==
>> -            v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
>> +            v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
>>                             &mt9v011_info, NULL))
>>               return -ENODEV;
>>           v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
>> @@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
>>           v4l2->sensor_yres = 480;
>>           subdev =
>> -             v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
>> +             v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
>>                              &ov2640_info, NULL);
>>           if (!subdev)
>>               return -ENODEV;
>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c
>> b/drivers/media/usb/em28xx/em28xx-video.c
>> index 6b84c3413e83..e1febb2bf06b 100644
>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>> @@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
>>    */
>>   static void em28xx_wake_i2c(struct em28xx *dev)
>>   {
>> -    struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
>> +    struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>>       v4l2_device_call_all(v4l2_dev, 0, core,  reset, 0);
>>       v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
>> @@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct
>> em28xx *dev)
>>       struct em28xx_v4l2 *v4l2 = dev->v4l2;
>>       int ret, i;
>> +    v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
>> +    if (!v4l2->video_pad) {
>> +        dev_err(&dev->intf->dev,
>> +            "failed to allocate video pad memory!\n");
>> +        v4l2->vdev->entity.num_pads = 0;
>> +        return;
>> +    }
>> +
>>       /* Initialize Video, VBI and Radio pads */
>> -    v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
>> -    ret = media_entity_pads_init(&v4l2->vdev.entity, 1,
>> &v4l2->video_pad);
>> +    v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
>> +    ret = media_entity_pads_init(&v4l2->vdev->entity, 1,
>> v4l2->video_pad);
>>       if (ret < 0)
>>           dev_err(&dev->intf->dev,
>>               "failed to initialize video media entity!\n");
>> @@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct
>> vb2_queue *vq, unsigned int count)
>>               f.type = V4L2_TUNER_RADIO;
>>           else
>>               f.type = V4L2_TUNER_ANALOG_TV;
>> -        v4l2_device_call_all(&v4l2->v4l2_dev,
>> +        v4l2_device_call_all(v4l2->v4l2_dev,
>>                        0, tuner, s_frequency, &f);
>>           /* Enable video stream at TV decoder */
>> -        v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
>> +        v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
>>       }
>>       v4l2->streaming_users++;
>> @@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct
>> vb2_queue *vq)
>>       if (v4l2->streaming_users-- == 1) {
>>           /* Disable video stream at TV decoder */
>> -        v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
>> +        v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>>           /* Last active user, so shutdown all the URBS */
>>           em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>> @@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue
>> *vq)
>>       if (v4l2->streaming_users-- == 1) {
>>           /* Disable video stream at TV decoder */
>> -        v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
>> +        v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>>           /* Last active user, so shutdown all the URBS */
>>           em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>> @@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)
>>   static void video_mux(struct em28xx *dev, int index)
>>   {
>> -    struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
>> +    struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>>       dev->ctl_input = index;
>>       dev->ctl_ainput = INPUT(index)->amux;
>> @@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file,
>> void *priv, v4l2_std_id *norm)
>>   {
>>       struct em28xx *dev = video_drvdata(file);
>> -    v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd,
>> norm);
>> +    v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);
>>       return 0;
>>   }
>> @@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void
>> *priv, v4l2_std_id norm)
>>                 &v4l2->hscale, &v4l2->vscale);
>>       em28xx_resolution_set(dev);
>> -    v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> +    v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>>       return 0;
>>   }
>> @@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void
>> *priv,
>>       p->parm.capture.readbuffers = EM28XX_MIN_BUF;
>>       p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>>       if (dev->is_webcam) {
>> -        rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
>> +        rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
>>                           video, g_frame_interval, &ival);
>>           if (!rc)
>>               p->parm.capture.timeperframe = ival.interval;
>> @@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void
>> *priv,
>>       memset(&p->parm, 0, sizeof(p->parm));
>>       p->parm.capture.readbuffers = EM28XX_MIN_BUF;
>>       p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> -    rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
>> +    rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
>>                       video, s_frame_interval, &ival);
>>       if (!rc)
>>           p->parm.capture.timeperframe = ival.interval;
>> @@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file,
>> void *priv,
>>       if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
>>           i->type = V4L2_INPUT_TYPE_TUNER;
>> -    i->std = dev->v4l2->vdev.tvnorms;
>> +    i->std = dev->v4l2->vdev->tvnorms;
>>       /* webcams do not have the STD API */
>>       if (dev->is_webcam)
>>           i->capabilities = 0;
>> @@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file,
>> void *priv,
>>       strscpy(t->name, "Tuner", sizeof(t->name));
>> -    v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> +    v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>>       return 0;
>>   }
>> @@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file,
>> void *priv,
>>       if (t->index != 0)
>>           return -EINVAL;
>> -    v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> +    v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>>       return 0;
>>   }
>> @@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file,
>> void *priv,
>>       if (f->tuner != 0)
>>           return -EINVAL;
>> -    v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
>> -    v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency,
>> &new_freq);
>> +    v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
>> +    v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency,
>> &new_freq);
>>       v4l2->frequency = new_freq.frequency;
>>       return 0;
>> @@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file,
>> void *priv,
>>           strscpy(chip->name, "ac97", sizeof(chip->name));
>>       else
>>           strscpy(chip->name,
>> -            dev->v4l2->v4l2_dev.name, sizeof(chip->name));
>> +            dev->v4l2->v4l2_dev->name, sizeof(chip->name));
>>       return 0;
>>   }
>> @@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void
>> *priv,
>>       strscpy(t->name, "Radio", sizeof(t->name));
>> -    v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> +    v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>>       return 0;
>>   }
>> @@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void
>> *priv,
>>       if (t->index != 0)
>>           return -EINVAL;
>> -    v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> +    v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>>       return 0;
>>   }
>> @@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
>>       if (mutex_lock_interruptible(&dev->lock))
>>           return -ERESTARTSYS;
>> +    if (!dev->v4l2) {
>> +        mutex_unlock(&dev->lock);
>> +        return -ENODEV;
>> +    }
>> +
>>       ret = v4l2_fh_open(filp);
>>       if (ret) {
>>           dev_err(&dev->intf->dev,
>> @@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
>>       if (vdev->vfl_type == VFL_TYPE_RADIO) {
>>           em28xx_videodbg("video_open: setting radio device\n");
>> -        v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
>> +        v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
>>       }
>>       kref_get(&dev->ref);
>> @@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>>       mutex_lock(&dev->lock);
>> -    v4l2_device_disconnect(&v4l2->v4l2_dev);
>> +    v4l2_device_disconnect(v4l2->v4l2_dev);
>>       em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>> @@ -2238,14 +2251,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>>                video_device_node_name(&v4l2->vbi_dev));
>>           video_unregister_device(&v4l2->vbi_dev);
>>       }
>> -    if (video_is_registered(&v4l2->vdev)) {
>> +    if (video_is_registered(v4l2->vdev)) {
>>           dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
>> -             video_device_node_name(&v4l2->vdev));
>> -        video_unregister_device(&v4l2->vdev);
>> +             video_device_node_name(v4l2->vdev));
>> +        video_unregister_device(v4l2->vdev);
>>       }
>>       v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
>> -    v4l2_device_unregister(&v4l2->v4l2_dev);
>> +    v4l2_device_unregister(v4l2->v4l2_dev);
>> +    v4l2_device_put(v4l2->v4l2_dev);
>>       kref_put(&v4l2->ref, em28xx_free_v4l2);
>> @@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
>>               goto exit;
>>           /* Save some power by putting tuner to sleep */
>> -        v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
>> +        v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>>           /* do this before setting alternate! */
>>           em28xx_set_mode(dev, EM28XX_SUSPEND);
>> @@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
>>       return 0;
>>   }
>> +void em28xx_vdev_release(struct video_device *vdev)
>> +{
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +    int i;
>> +
>> +    for (i = 0; i < vdev->entity.num_pads; i++)
>> +        kfree(&vdev->entity.pads[i]);
>> +#endif
>> +    kfree(vdev);
>> +}
>> +
>>   static const struct v4l2_file_operations em28xx_v4l_fops = {
>>       .owner         = THIS_MODULE,
>>       .open          = em28xx_v4l2_open,
>> @@ -2387,7 +2412,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,
>>   };
>> @@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
>>                    const char *type_name)
>>   {
>>       *vfd        = *template;
>> -    vfd->v4l2_dev    = &dev->v4l2->v4l2_dev;
>> +    vfd->v4l2_dev    = dev->v4l2->v4l2_dev;
>>       vfd->lock    = &dev->lock;
>>       if (dev->is_webcam)
>>           vfd->tvnorms = 0;
>> @@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
>>   static void em28xx_tuner_setup(struct em28xx *dev, unsigned short
>> tuner_addr)
>>   {
>>       struct em28xx_v4l2      *v4l2 = dev->v4l2;
>> -    struct v4l2_device      *v4l2_dev = &v4l2->v4l2_dev;
>> +    struct v4l2_device      *v4l2_dev = v4l2->v4l2_dev;
>>       struct tuner_setup      tun_setup;
>>       struct v4l2_frequency   f;
>> @@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx
>> *dev, unsigned short tuner_addr)
>>       v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
>>   }
>> +void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
>> +{
>> +    kfree(v4l2_dev);
>> +}
>> +
>>   static int em28xx_v4l2_init(struct em28xx *dev)
>>   {
>>       u8 val;
>> @@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>       v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
>>       if (!v4l2) {
>> -        mutex_unlock(&dev->lock);
>> -        return -ENOMEM;
>> +        ret = -ENOMEM;
>> +        goto v4l2_error;
>>       }
>> +
>>       kref_init(&v4l2->ref);
>>       v4l2->dev = dev;
>>       dev->v4l2 = v4l2;
>> +    v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
>> +    if (!v4l2->v4l2_dev) {
>> +        ret = -ENOMEM;
>> +        goto v4l2_dev_error;
>> +    }
>> +
>> +    v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
>> +
>>   #ifdef CONFIG_MEDIA_CONTROLLER
>> -    v4l2->v4l2_dev.mdev = dev->media_dev;
>> +    v4l2->v4l2_dev->mdev = dev->media_dev;
>>   #endif
>> -    ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
>> +    ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
>>       if (ret < 0) {
>>           dev_err(&dev->intf->dev,
>>               "Call to v4l2_device_register() failed!\n");
>> -        goto err;
>> +        goto v4l2_device_register_error;
>>       }
>>       hdl = &v4l2->ctrl_handler;
>>       v4l2_ctrl_handler_init(hdl, 8);
>> -    v4l2->v4l2_dev.ctrl_handler = hdl;
>> +    v4l2->v4l2_dev->ctrl_handler = hdl;
>>       if (dev->is_webcam)
>>           v4l2->progressive = true;
>> @@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>       /* request some modules */
>>       if (dev->has_msp34xx)
>> -        v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> +        v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>>                       &dev->i2c_adap[dev->def_i2c_bus],
>>                       "msp3400", 0, msp3400_addrs);
>>       if (dev->board.decoder == EM28XX_SAA711X)
>> -        v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> +        v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>>                       &dev->i2c_adap[dev->def_i2c_bus],
>>                       "saa7115_auto", 0, saa711x_addrs);
>>       if (dev->board.decoder == EM28XX_TVP5150)
>> -        v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> +        v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>>                       &dev->i2c_adap[dev->def_i2c_bus],
>>                       "tvp5150", 0, tvp5150_addrs);
>>       if (dev->board.adecoder == EM28XX_TVAUDIO)
>> -        v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> +        v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>>                       &dev->i2c_adap[dev->def_i2c_bus],
>>                       "tvaudio", dev->board.tvaudio_addr, NULL);
>> @@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>           int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
>>           if (dev->board.radio.type)
>> -            v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> +            v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>>                           &dev->i2c_adap[dev->def_i2c_bus],
>>                           "tuner", dev->board.radio_addr,
>>                           NULL);
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.


I will add to all bellow.

>
>>           if (has_demod)
>> -            v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> +            v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>>                           &dev->i2c_adap[dev->def_i2c_bus],
>>                           "tuner", 0,
>>                           v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
>
> Same here:
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
>
>> @@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>                   has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
>>               struct v4l2_subdev *sd;
>> -            sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> +            sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>>                            &dev->i2c_adap[dev->def_i2c_bus],
>>                            "tuner", 0,
>>                            v4l2_i2c_tuner_addrs(type));
>> @@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>               if (sd)
>>                   tuner_addr = v4l2_i2c_subdev_addr(sd);
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
>
>>           } else {
>> -            v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> +            v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>>                           &dev->i2c_adap[dev->def_i2c_bus],
>>                           "tuner", tuner_addr, NULL);
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
>
>>           }
>> @@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>       /* set default norm */
>>       v4l2->norm = V4L2_STD_PAL;
>> -    v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> +    v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>>       v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
>>       /* Analog specific initialization */
>> @@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>           goto unregister_dev;
>>       /* allocate and fill video video_device struct */
>> -    em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
>> +    v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
>> +    if (!v4l2->vdev) {
>> +        ret = -ENOMEM;
>> +        goto unregister_dev;
>> +    }
>> +
>> +    em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
>>       mutex_init(&v4l2->vb_queue_lock);
>>       mutex_init(&v4l2->vb_vbi_queue_lock);
>> -    v4l2->vdev.queue = &v4l2->vb_vidq;
>> -    v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
>> -    v4l2->vdev.device_caps = V4L2_CAP_READWRITE |
>> V4L2_CAP_VIDEO_CAPTURE |
>> +    v4l2->vdev->queue = &v4l2->vb_vidq;
>> +    v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
>> +    v4l2->vdev->device_caps = V4L2_CAP_READWRITE |
>> V4L2_CAP_VIDEO_CAPTURE |
>>                    V4L2_CAP_STREAMING;
>>       if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
>> -        v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
>> +        v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
>>       if (dev->tuner_type != TUNER_ABSENT)
>> -        v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
>> -
>> +        v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
>>       /* disable inapplicable ioctls */
>>       if (dev->is_webcam) {
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
>>       } else {
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
>>       }
>>       if (dev->tuner_type == TUNER_ABSENT) {
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
>>       }
>>       if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
>> -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
>> +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
>>       }
>>       /* register v4l2 video video_device */
>> -    ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
>> +    ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
>>                       video_nr[dev->devno]);
>>       if (ret) {
>>           dev_err(&dev->intf->dev,
>> @@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>       dev_info(&dev->intf->dev,
>>            "V4L2 video device registered as %s\n",
>> -         video_device_node_name(&v4l2->vdev));
>> +         video_device_node_name(v4l2->vdev));
>>       if (video_is_registered(&v4l2->vbi_dev))
>>           dev_info(&dev->intf->dev,
>> @@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>                video_device_node_name(&v4l2->vbi_dev));
>>       /* Save some power by putting tuner to sleep */
>> -    v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
>> +    v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>>       /* initialize videobuf2 stuff */
>>       em28xx_vb2_setup(dev);
>> @@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>>                video_device_node_name(&v4l2->vbi_dev));
>>           video_unregister_device(&v4l2->vbi_dev);
>>       }
>> -    if (video_is_registered(&v4l2->vdev)) {
>> +    if (video_is_registered(v4l2->vdev)) {
>>           dev_info(&dev->intf->dev,
>>                "V4L2 device %s deregistered\n",
>> -             video_device_node_name(&v4l2->vdev));
>> -        video_unregister_device(&v4l2->vdev);
>> +             video_device_node_name(v4l2->vdev));
>> +        video_unregister_device(v4l2->vdev);
>>       }
>>       v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
>> -    v4l2_device_unregister(&v4l2->v4l2_dev);
>> -err:
>> +    v4l2_device_unregister(v4l2->v4l2_dev);
>> +
>> +v4l2_device_register_error:
>> +    v4l2_device_put(v4l2->v4l2_dev);
>> +v4l2_dev_error:
>>       dev->v4l2 = NULL;
>>       kref_put(&v4l2->ref, em28xx_free_v4l2);
>> +v4l2_error:
>>       mutex_unlock(&dev->lock);
>>       return ret;
>>   }
>> diff --git a/drivers/media/usb/em28xx/em28xx.h
>> b/drivers/media/usb/em28xx/em28xx.h
>> index 6648e11f1271..dbcc297b5a0d 100644
>> --- a/drivers/media/usb/em28xx/em28xx.h
>> +++ b/drivers/media/usb/em28xx/em28xx.h
>> @@ -552,10 +552,10 @@ struct em28xx_v4l2 {
>>       struct kref ref;
>>       struct em28xx *dev;
>> -    struct v4l2_device v4l2_dev;
>> +    struct v4l2_device *v4l2_dev;
>>       struct v4l2_ctrl_handler ctrl_handler;
>> -    struct video_device vdev;
>> +    struct video_device *vdev;
>>       struct video_device vbi_dev;
>>       struct video_device radio_dev;
>> @@ -601,7 +601,7 @@ struct em28xx_v4l2 {
>>       unsigned int field_count;
>>   #ifdef CONFIG_MEDIA_CONTROLLER
>> -    struct media_pad video_pad, vbi_pad;
>> +    struct media_pad *video_pad, vbi_pad;
>>       struct media_entity *decoder;
>>   #endif
>>   };
>>
>
> thanks,
> -- Shuah

Thanks,
---
Igor Matheus Andrade Torrente