media: em28xx: Fix possible memory leak of em28xx struct

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 deregistration of v4l2 subsystem, the 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-by kernel test robot <[email protected]>

Reported-and-tested-by [email protected]

Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---

V2: Add v4l2_i2c_new_subdev null check
Deal with v4l2 subdevs dependencies

V3: Fix link error when compiled as a module

V4: Remove duplicated v4l2_device_disconnect

    in the em28xx_v4l2_fini

---
 drivers/media/usb/em28xx/em28xx-camera.c |   4 +-
 drivers/media/usb/em28xx/em28xx-video.c  | 299 +++++++++++++++--------
 drivers/media/usb/em28xx/em28xx.h        |   6 +-
 3 files changed, 208 insertions(+), 101 deletions(-)

drivers/media/usb/em28xx/em28xx-camera.c +2 -2

index d1e66b503f4d..436c5a8cbbb6 100644
@@ -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;

drivers/media/usb/em28xx/em28xx-video.c +203 -96

index 6b84c3413e83..6bc6baf88644 100644
@@ -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,9 +2197,10 @@ 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);
 	}
 
+	v4l2_device_get(v4l2->v4l2_dev);
 	kref_get(&dev->ref);
 	kref_get(&v4l2->ref);
 	v4l2->users++;

@@ -2222,7 +2236,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 +2252,14 @@ 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_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);

@@ -2322,6 +2336,7 @@ static int em28xx_v4l2_close(struct file *filp)
 	}
 
 exit:
+	v4l2_device_put(v4l2->v4l2_dev);
 	v4l2->users--;
 	kref_put(&v4l2->ref, em28xx_free_v4l2);
 	mutex_unlock(&dev->lock);

@@ -2330,6 +2345,17 @@ static int em28xx_v4l2_close(struct file *filp)
 	return 0;
 }
 
+static 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 +2413,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 +2471,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 +2485,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 +2543,22 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
 	v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
 }
 
+static void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+	struct v4l2_subdev *sd, *next;
+	struct i2c_client *client;
+
+	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) {
+		v4l2_device_unregister_subdev(sd);
+		client = sd->dev_priv;
+		if (client && !client->dev.of_node && !client->dev.fwnode &&
+		    sd->flags & V4L2_SUBDEV_FL_IS_I2C)
+			i2c_unregister_device(client);
+	}
+
+	kfree(v4l2_dev);
+}
+
 static int em28xx_v4l2_init(struct em28xx *dev)
 {
 	u8 val;

@@ -2524,6 +2566,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 	unsigned int maxw;
 	struct v4l2_ctrl_handler *hdl;
 	struct em28xx_v4l2 *v4l2;
+	struct v4l2_subdev *sd;
 
 	if (dev->is_audio_only) {
 		/* Shouldn't initialize IR for this interface */

@@ -2541,26 +2584,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;

@@ -2574,25 +2626,49 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 
 	/* request some modules */
 
-	if (dev->has_msp34xx)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "msp3400", 0, msp3400_addrs);
+	if (dev->has_msp34xx) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "msp3400", 0, msp3400_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering msp34xx v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.decoder == EM28XX_SAA711X)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "saa7115_auto", 0, saa711x_addrs);
+	if (dev->board.decoder == EM28XX_SAA711X) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "saa7115_auto", 0, saa711x_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_SAA711X v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.decoder == EM28XX_TVP5150)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "tvp5150", 0, tvp5150_addrs);
+	if (dev->board.decoder == EM28XX_TVP5150) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "tvp5150", 0, tvp5150_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_TVP5150 v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.adecoder == EM28XX_TVAUDIO)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "tvaudio", dev->board.tvaudio_addr, NULL);
+	if (dev->board.adecoder == EM28XX_TVAUDIO) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "tvaudio", dev->board.tvaudio_addr, NULL);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_TVAUDIO v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
 	/* Initialize tuner and camera */
 

@@ -2600,33 +2676,55 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 		unsigned short tuner_addr = dev->board.tuner_addr;
 		int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
 
-		if (dev->board.radio.type)
-			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,
-					    &dev->i2c_adap[dev->def_i2c_bus],
-					    "tuner", 0,
-					    v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
+		if (dev->board.radio.type) {
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", dev->board.radio_addr,
+						 NULL);
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name1> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+		}
+
+		if (has_demod) {
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", 0,
+						 v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name2> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+		}
+
 		if (tuner_addr == 0) {
 			enum v4l2_i2c_tuner_type type =
 				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));
-
-			if (sd)
+			if (sd) {
 				tuner_addr = v4l2_i2c_subdev_addr(sd);
+			} else {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name3> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+
 		} else {
-			v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-					    &dev->i2c_adap[dev->def_i2c_bus],
-					    "tuner", tuner_addr, NULL);
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", tuner_addr, NULL);
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name4> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
 		}
 
 		em28xx_tuner_setup(dev, tuner_addr);

@@ -2686,7 +2784,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 +2854,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 +2966,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 +2974,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 +3000,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_subdev_register_error:
+	v4l2_device_disconnect(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;
 }

drivers/media/usb/em28xx/em28xx.h +3 -3

index 6648e11f1271..dbcc297b5a0d 100644
@@ -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
 };

Hi Igor,

Quoted 5 lines · Igor Matheus Andrade Torrente

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.

Quoted 8 lines

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 deregistration of v4l2 subsystem, the 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.

This isn't the right approach. This driver is quite old and tried to do life-time management itself (and poorly at that), while today there are better mechanisms, something that your patch tries to use to some extent.

The cleanup for em28xx-video.c has to take place in the release callback of struct v4l2_device. All allocated memory can be cleaned up there. The release callback of the video_device structs can just remains as it is today, i.e. video_device_release_empty.

Also, the video_unregister_device calls should be replaced by vb2_video_unregister_device, see include/media/videobuf2-v4l2.h why.

The v4l2->ref can be removed, since struct v4l2_device takes over that role.

And when the release callback of v4l2_device is called, then you can call kref_get(&dev->ref). That reference count really just needs to be incremented once in em28xx_v4l2_init and decremented once in the v4l2_device release callback, and not for every open and close.

I hope I haven't forgotten anything

Regards,

Hans

Quoted 690 lines

Reported-by kernel test robot <[email protected]>

Reported-and-tested-by [email protected]

Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---

V2: Add v4l2_i2c_new_subdev null check
Deal with v4l2 subdevs dependencies

V3: Fix link error when compiled as a module

V4: Remove duplicated v4l2_device_disconnect

     in the em28xx_v4l2_fini

---
  drivers/media/usb/em28xx/em28xx-camera.c |   4 +-
  drivers/media/usb/em28xx/em28xx-video.c  | 299 +++++++++++++++--------
  drivers/media/usb/em28xx/em28xx.h        |   6 +-
  3 files changed, 208 insertions(+), 101 deletions(-)

drivers/media/usb/em28xx/em28xx-camera.c +2 -2

index d1e66b503f4d..436c5a8cbbb6 100644
@@ -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;

drivers/media/usb/em28xx/em28xx-video.c +203 -96

index 6b84c3413e83..6bc6baf88644 100644
@@ -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,9 +2197,10 @@ 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);
  	}
 
+	v4l2_device_get(v4l2->v4l2_dev);
  	kref_get(&dev->ref);
  	kref_get(&v4l2->ref);
  	v4l2->users++;

@@ -2222,7 +2236,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 +2252,14 @@ 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_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);

@@ -2322,6 +2336,7 @@ static int em28xx_v4l2_close(struct file *filp)
  	}
 
  exit:
+	v4l2_device_put(v4l2->v4l2_dev);
  	v4l2->users--;
  	kref_put(&v4l2->ref, em28xx_free_v4l2);
  	mutex_unlock(&dev->lock);

@@ -2330,6 +2345,17 @@ static int em28xx_v4l2_close(struct file *filp)
  	return 0;
  }
 
+static 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 +2413,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 +2471,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 +2485,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 +2543,22 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
  	v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
  }
 
+static void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+	struct v4l2_subdev *sd, *next;
+	struct i2c_client *client;
+
+	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) {
+		v4l2_device_unregister_subdev(sd);
+		client = sd->dev_priv;
+		if (client && !client->dev.of_node && !client->dev.fwnode &&
+		    sd->flags & V4L2_SUBDEV_FL_IS_I2C)
+			i2c_unregister_device(client);
+	}
+
+	kfree(v4l2_dev);
+}
+
  static int em28xx_v4l2_init(struct em28xx *dev)
  {
  	u8 val;

@@ -2524,6 +2566,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  	unsigned int maxw;
  	struct v4l2_ctrl_handler *hdl;
  	struct em28xx_v4l2 *v4l2;
+	struct v4l2_subdev *sd;
 
  	if (dev->is_audio_only) {
  		/* Shouldn't initialize IR for this interface */

@@ -2541,26 +2584,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;

@@ -2574,25 +2626,49 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 
  	/* request some modules */
 
-	if (dev->has_msp34xx)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "msp3400", 0, msp3400_addrs);
+	if (dev->has_msp34xx) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "msp3400", 0, msp3400_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering msp34xx v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.decoder == EM28XX_SAA711X)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "saa7115_auto", 0, saa711x_addrs);
+	if (dev->board.decoder == EM28XX_SAA711X) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "saa7115_auto", 0, saa711x_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_SAA711X v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.decoder == EM28XX_TVP5150)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "tvp5150", 0, tvp5150_addrs);
+	if (dev->board.decoder == EM28XX_TVP5150) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "tvp5150", 0, tvp5150_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_TVP5150 v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.adecoder == EM28XX_TVAUDIO)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "tvaudio", dev->board.tvaudio_addr, NULL);
+	if (dev->board.adecoder == EM28XX_TVAUDIO) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "tvaudio", dev->board.tvaudio_addr, NULL);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_TVAUDIO v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
  	/* Initialize tuner and camera */
 

@@ -2600,33 +2676,55 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  		unsigned short tuner_addr = dev->board.tuner_addr;
  		int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
 
-		if (dev->board.radio.type)
-			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,
-					    &dev->i2c_adap[dev->def_i2c_bus],
-					    "tuner", 0,
-					    v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
+		if (dev->board.radio.type) {
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", dev->board.radio_addr,
+						 NULL);
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name1> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+		}
+
+		if (has_demod) {
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", 0,
+						 v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name2> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+		}
+
  		if (tuner_addr == 0) {
  			enum v4l2_i2c_tuner_type type =
  				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));
-
-			if (sd)
+			if (sd) {
  				tuner_addr = v4l2_i2c_subdev_addr(sd);
+			} else {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name3> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+
  		} else {
-			v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-					    &dev->i2c_adap[dev->def_i2c_bus],
-					    "tuner", tuner_addr, NULL);
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", tuner_addr, NULL);
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name4> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
  		}
 
  		em28xx_tuner_setup(dev, tuner_addr);

@@ -2686,7 +2784,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 +2854,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 +2966,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 +2974,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 +3000,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_subdev_register_error:
+	v4l2_device_disconnect(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;
  }

drivers/media/usb/em28xx/em28xx.h +3 -3

index 6648e11f1271..dbcc297b5a0d 100644
@@ -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
  };

Quoted 17 lines · Hans Verkuil

Hi Igor,

Quoted 5 lines · Igor Matheus Andrade Torrente

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.

Quoted 8 lines

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 deregistration of v4l2 subsystem, the 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.

Quoted 9 lines

This isn't the right approach. This driver is quite old and tried to do life-time management itself (and poorly at that), while today there are better mechanisms, something that your patch tries to use to some extent.

The cleanup for em28xx-video.c has to take place in the release callback of struct v4l2_device. All allocated memory can be cleaned up there. The release callback of the video_device structs can just remains as it is today, i.e. video_device_release_empty.

Do you mean only the free of v4l2 and v4l2_device structs? Or the release callback should include vb2_video_unregister_device, v4l2_ctrl_handler_free, etc in the release callback?

Quoted 4 lines

Also, the video_unregister_device calls should be replaced by vb2_video_unregister_device, see include/media/videobuf2-v4l2.h why.

Quoted 8 lines

The v4l2->ref can be removed, since struct v4l2_device takes over that role.

And when the release callback of v4l2_device is called, then you can call kref_get(&dev->ref). That reference count really just needs to be incremented once in em28xx_v4l2_init and decremented once in the v4l2_device release callback, and not for every open and close.

I hope I haven't forgotten anything

Thanks for the review,
---
Igor M. A. Torrente

Quoted 697 lines

Regards,

Hans

Quoted 690 lines

Reported-by kernel test robot <[email protected]>

Reported-and-tested-by [email protected]

Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---

V2: Add v4l2_i2c_new_subdev null check
Deal with v4l2 subdevs dependencies

V3: Fix link error when compiled as a module

V4: Remove duplicated v4l2_device_disconnect

     in the em28xx_v4l2_fini

---
  drivers/media/usb/em28xx/em28xx-camera.c |   4 +-
  drivers/media/usb/em28xx/em28xx-video.c  | 299 +++++++++++++++--------
  drivers/media/usb/em28xx/em28xx.h        |   6 +-
  3 files changed, 208 insertions(+), 101 deletions(-)

drivers/media/usb/em28xx/em28xx-camera.c +2 -2

index d1e66b503f4d..436c5a8cbbb6 100644
@@ -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;

drivers/media/usb/em28xx/em28xx-video.c +203 -96

index 6b84c3413e83..6bc6baf88644 100644
@@ -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,9 +2197,10 @@ 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);
  	}
 
+	v4l2_device_get(v4l2->v4l2_dev);
  	kref_get(&dev->ref);
  	kref_get(&v4l2->ref);
  	v4l2->users++;

@@ -2222,7 +2236,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 +2252,14 @@ 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_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);

@@ -2322,6 +2336,7 @@ static int em28xx_v4l2_close(struct file *filp)
  	}
 
  exit:
+	v4l2_device_put(v4l2->v4l2_dev);
  	v4l2->users--;
  	kref_put(&v4l2->ref, em28xx_free_v4l2);
  	mutex_unlock(&dev->lock);

@@ -2330,6 +2345,17 @@ static int em28xx_v4l2_close(struct file *filp)
  	return 0;
  }
 
+static 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 +2413,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 +2471,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 +2485,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 +2543,22 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
  	v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
  }
 
+static void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+	struct v4l2_subdev *sd, *next;
+	struct i2c_client *client;
+
+	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) {
+		v4l2_device_unregister_subdev(sd);
+		client = sd->dev_priv;
+		if (client && !client->dev.of_node && !client->dev.fwnode &&
+		    sd->flags & V4L2_SUBDEV_FL_IS_I2C)
+			i2c_unregister_device(client);
+	}
+
+	kfree(v4l2_dev);
+}
+
  static int em28xx_v4l2_init(struct em28xx *dev)
  {
  	u8 val;

@@ -2524,6 +2566,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  	unsigned int maxw;
  	struct v4l2_ctrl_handler *hdl;
  	struct em28xx_v4l2 *v4l2;
+	struct v4l2_subdev *sd;
 
  	if (dev->is_audio_only) {
  		/* Shouldn't initialize IR for this interface */

@@ -2541,26 +2584,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;

@@ -2574,25 +2626,49 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 
  	/* request some modules */
 
-	if (dev->has_msp34xx)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "msp3400", 0, msp3400_addrs);
+	if (dev->has_msp34xx) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "msp3400", 0, msp3400_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering msp34xx v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.decoder == EM28XX_SAA711X)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "saa7115_auto", 0, saa711x_addrs);
+	if (dev->board.decoder == EM28XX_SAA711X) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "saa7115_auto", 0, saa711x_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_SAA711X v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.decoder == EM28XX_TVP5150)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "tvp5150", 0, tvp5150_addrs);
+	if (dev->board.decoder == EM28XX_TVP5150) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "tvp5150", 0, tvp5150_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_TVP5150 v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.adecoder == EM28XX_TVAUDIO)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "tvaudio", dev->board.tvaudio_addr, NULL);
+	if (dev->board.adecoder == EM28XX_TVAUDIO) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "tvaudio", dev->board.tvaudio_addr, NULL);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_TVAUDIO v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
  	/* Initialize tuner and camera */
 

@@ -2600,33 +2676,55 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  		unsigned short tuner_addr = dev->board.tuner_addr;
  		int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
 
-		if (dev->board.radio.type)
-			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,
-					    &dev->i2c_adap[dev->def_i2c_bus],
-					    "tuner", 0,
-					    v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
+		if (dev->board.radio.type) {
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", dev->board.radio_addr,
+						 NULL);
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name1> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+		}
+
+		if (has_demod) {
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", 0,
+						 v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name2> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+		}
+
  		if (tuner_addr == 0) {
  			enum v4l2_i2c_tuner_type type =
  				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));
-
-			if (sd)
+			if (sd) {
  				tuner_addr = v4l2_i2c_subdev_addr(sd);
+			} else {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name3> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+
  		} else {
-			v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-					    &dev->i2c_adap[dev->def_i2c_bus],
-					    "tuner", tuner_addr, NULL);
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", tuner_addr, NULL);
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name4> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
  		}
 
  		em28xx_tuner_setup(dev, tuner_addr);

@@ -2686,7 +2784,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 +2854,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 +2966,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 +2974,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 +3000,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_subdev_register_error:
+	v4l2_device_disconnect(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;
  }

drivers/media/usb/em28xx/em28xx.h +3 -3

index 6648e11f1271..dbcc297b5a0d 100644
@@ -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
  };

Quoted 30 lines · Igor Torrente

Quoted 17 lines · Hans Verkuil

Hi Igor,

Quoted 5 lines · Igor Matheus Andrade Torrente

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.

Quoted 8 lines

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 deregistration of v4l2 subsystem, the 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.

Quoted 9 lines

This isn't the right approach. This driver is quite old and tried to do life-time management itself (and poorly at that), while today there are better mechanisms, something that your patch tries to use to some extent.

The cleanup for em28xx-video.c has to take place in the release callback of struct v4l2_device. All allocated memory can be cleaned up there. The release callback of the video_device structs can just remains as it is today, i.e. video_device_release_empty.

Quoted 3 lines

Do you mean only the free of v4l2 and v4l2_device structs? Or the release callback should include vb2_video_unregister_device, v4l2_ctrl_handler_free, etc in the release callback?

I meant any freeing of resources, so v4l2_ctrl_handler_free, any kfree()s, etc. The vb2_video_unregister_device() must be called when the device is disconnected, or the probe fails. vb2_video_unregister_device() cancels any streaming video nodes and marks the video device as being unregistered, thus preventing applications from attempting to open it again.

When the last filehandle for these video nodes is closed, then the release callback of v4l2_device is called, and that's where you can free all v4l2 memory and do the final kref_put(&dev->ref).

Regards,

Hans

Quoted 715 lines

Quoted 4 lines

Also, the video_unregister_device calls should be replaced by vb2_video_unregister_device, see include/media/videobuf2-v4l2.h why.

Quoted 8 lines

The v4l2->ref can be removed, since struct v4l2_device takes over that role.

And when the release callback of v4l2_device is called, then you can call kref_get(&dev->ref). That reference count really just needs to be incremented once in em28xx_v4l2_init and decremented once in the v4l2_device release callback, and not for every open and close.

I hope I haven't forgotten anything

Thanks for the review,
---
Igor M. A. Torrente

Quoted 697 lines

Regards,

Hans

Quoted 690 lines

Reported-by kernel test robot <[email protected]>

Reported-and-tested-by [email protected]

Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---

V2: Add v4l2_i2c_new_subdev null check
Deal with v4l2 subdevs dependencies

V3: Fix link error when compiled as a module

V4: Remove duplicated v4l2_device_disconnect

      in the em28xx_v4l2_fini

---
   drivers/media/usb/em28xx/em28xx-camera.c |   4 +-
   drivers/media/usb/em28xx/em28xx-video.c  | 299 +++++++++++++++--------
   drivers/media/usb/em28xx/em28xx.h        |   6 +-
   3 files changed, 208 insertions(+), 101 deletions(-)

drivers/media/usb/em28xx/em28xx-camera.c +2 -2

index d1e66b503f4d..436c5a8cbbb6 100644
@@ -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;

drivers/media/usb/em28xx/em28xx-video.c +203 -96

index 6b84c3413e83..6bc6baf88644 100644
@@ -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,9 +2197,10 @@ 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);
   	}
 
+	v4l2_device_get(v4l2->v4l2_dev);
   	kref_get(&dev->ref);
   	kref_get(&v4l2->ref);
   	v4l2->users++;

@@ -2222,7 +2236,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 +2252,14 @@ 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_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);

@@ -2322,6 +2336,7 @@ static int em28xx_v4l2_close(struct file *filp)
   	}
 
   exit:
+	v4l2_device_put(v4l2->v4l2_dev);
   	v4l2->users--;
   	kref_put(&v4l2->ref, em28xx_free_v4l2);
   	mutex_unlock(&dev->lock);

@@ -2330,6 +2345,17 @@ static int em28xx_v4l2_close(struct file *filp)
   	return 0;
   }
 
+static 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 +2413,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 +2471,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 +2485,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 +2543,22 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
   	v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
   }
 
+static void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+	struct v4l2_subdev *sd, *next;
+	struct i2c_client *client;
+
+	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) {
+		v4l2_device_unregister_subdev(sd);
+		client = sd->dev_priv;
+		if (client && !client->dev.of_node && !client->dev.fwnode &&
+		    sd->flags & V4L2_SUBDEV_FL_IS_I2C)
+			i2c_unregister_device(client);
+	}
+
+	kfree(v4l2_dev);
+}
+
   static int em28xx_v4l2_init(struct em28xx *dev)
   {
   	u8 val;

@@ -2524,6 +2566,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
   	unsigned int maxw;
   	struct v4l2_ctrl_handler *hdl;
   	struct em28xx_v4l2 *v4l2;
+	struct v4l2_subdev *sd;
 
   	if (dev->is_audio_only) {
   		/* Shouldn't initialize IR for this interface */

@@ -2541,26 +2584,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;

@@ -2574,25 +2626,49 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 
   	/* request some modules */
 
-	if (dev->has_msp34xx)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "msp3400", 0, msp3400_addrs);
+	if (dev->has_msp34xx) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "msp3400", 0, msp3400_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering msp34xx v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.decoder == EM28XX_SAA711X)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "saa7115_auto", 0, saa711x_addrs);
+	if (dev->board.decoder == EM28XX_SAA711X) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "saa7115_auto", 0, saa711x_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_SAA711X v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.decoder == EM28XX_TVP5150)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "tvp5150", 0, tvp5150_addrs);
+	if (dev->board.decoder == EM28XX_TVP5150) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "tvp5150", 0, tvp5150_addrs);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_TVP5150 v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
-	if (dev->board.adecoder == EM28XX_TVAUDIO)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-				    &dev->i2c_adap[dev->def_i2c_bus],
-				    "tvaudio", dev->board.tvaudio_addr, NULL);
+	if (dev->board.adecoder == EM28XX_TVAUDIO) {
+		sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+					 &dev->i2c_adap[dev->def_i2c_bus],
+					 "tvaudio", dev->board.tvaudio_addr, NULL);
+		if (!sd) {
+			dev_err(&dev->intf->dev,
+				"Error while registering EM28XX_TVAUDIO v4l2 subdevice!\n");
+			goto v4l2_subdev_register_error;
+		}
+	}
 
   	/* Initialize tuner and camera */
 

@@ -2600,33 +2676,55 @@ static int em28xx_v4l2_init(struct em28xx *dev)
   		unsigned short tuner_addr = dev->board.tuner_addr;
   		int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
 
-		if (dev->board.radio.type)
-			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,
-					    &dev->i2c_adap[dev->def_i2c_bus],
-					    "tuner", 0,
-					    v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
+		if (dev->board.radio.type) {
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", dev->board.radio_addr,
+						 NULL);
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name1> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+		}
+
+		if (has_demod) {
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", 0,
+						 v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name2> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+		}
+
   		if (tuner_addr == 0) {
   			enum v4l2_i2c_tuner_type type =
   				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));
-
-			if (sd)
+			if (sd) {
   				tuner_addr = v4l2_i2c_subdev_addr(sd);
+			} else {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name3> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
+
   		} else {
-			v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
-					    &dev->i2c_adap[dev->def_i2c_bus],
-					    "tuner", tuner_addr, NULL);
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
+						 &dev->i2c_adap[dev->def_i2c_bus],
+						 "tuner", tuner_addr, NULL);
+			if (!sd) {
+				dev_err(&dev->intf->dev,
+					"Error while registering <name4> v4l2 subdevice!\n");
+				goto v4l2_subdev_register_error;
+			}
   		}
 
   		em28xx_tuner_setup(dev, tuner_addr);

@@ -2686,7 +2784,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 +2854,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 +2966,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 +2974,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 +3000,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_subdev_register_error:
+	v4l2_device_disconnect(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;
   }

drivers/media/usb/em28xx/em28xx.h +3 -3

index 6648e11f1271..dbcc297b5a0d 100644
@@ -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
   };

The em28xx struct kref isn't being decreased after an error in the em28xx_ir_init, leading to a possible memory leak.

A kref_put is added to the error handler code.

Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---
  drivers/media/usb/em28xx/em28xx-input.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

drivers/media/usb/em28xx/em28xx-input.c +0 -0

index 5aa15a7a49de..b89527014cad 100644

Hi Igor,

Quoted 10 lines · Igor Matheus Andrade Torrente

The em28xx struct kref isn't being decreased after an error in the em28xx_ir_init, leading to a possible memory leak.

A kref_put is added to the error handler code.

Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---
  drivers/media/usb/em28xx/em28xx-input.c | 7 +++++--

  1 file changed, 5 insertions(+), 2 deletions(-)

drivers/media/usb/em28xx/em28xx-input.c static int em28xx_ir_init(struct em28xx *dev) · line 724

Quoted 11 lines

drivers/media/usb/em28xx/em28xx-input.c +2 -1

index 5aa15a7a49de..b89527014cad 100644
@@ -720,7 +720,8 @@ static int em28xx_ir_init(struct em28xx *dev)
               dev->board.has_ir_i2c = 0;
               dev_warn(&dev->intf->dev,
                    "No i2c IR remote control device found.\n");
-            return -ENODEV;
+            err = -ENODEV;
+            goto ref_put;

This doesn't look right. em28xx_init_buttons() is already happened and em28xx_shutdown_buttons() needs to be done from fini. fini needs to run with this ref. If ref is released here, device might be released before em28xx_shutdown_buttons() can run leading to potential use-after-free

drivers/media/usb/em28xx/em28xx-input.c static int em28xx_ir_init(struct em28xx *dev) · line 738

Quoted 7 lines

drivers/media/usb/em28xx/em28xx-input.c +1 -1

           }
       }

@@ -735,7 +736,7 @@ static int em28xx_ir_init(struct em28xx *dev)
       ir = kzalloc(sizeof(*ir), GFP_KERNEL);
       if (!ir)
-        return -ENOMEM;
+        goto ref_put;

This doesn't look right. Same comment as above. fini accounts for null ir.

      em28xx_shutdown_buttons(dev);

       /* skip detach on non attached boards */
       if (!ir)
               goto ref_put;

drivers/media/usb/em28xx/em28xx-input.c static int em28xx_ir_init(struct em28xx *dev) · line 847

Quoted 12 lines

drivers/media/usb/em28xx/em28xx-input.c +2 -0

       rc = rc_allocate_device(RC_DRIVER_SCANCODE);
       if (!rc)
           goto error;

@@ -839,6 +840,8 @@ static int em28xx_ir_init(struct em28xx *dev)
       dev->ir = NULL;
       rc_free_device(rc);
       kfree(ir);
+ref_put:
+    kref_put(&dev->ref, em28xx_free_device);
       return err;
   }
 

thanks,
-- Shuah