Skip to contentSkip to conversation

media: em28xx: Fix race condition between open and init function

PATCH Superseded Igor Matheus Andrade Torrente 3 participants 4 messages Apr 8, 2021 – Apr 9, 2021

Subsystems (2)
Media (V4L/DVB) EM28XX VIDEO4LINUX driver

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Igor Matheus Andrade Torrente

author
#0 lore raw

PATCH 1/1 media: em28xx: Fix race condition between open and init function

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

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

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

472 lines not on this page · raw message

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
 };
Signature · 1 line
2.20.1
build bot
#1 lore raw
Thank you for the patch! Perhaps something to improve:

Hi Igor,

Thank you for the patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Igor-Matheus-Andrade-Torrente/media-em28xx-Fix-race-condition-between-open-and-init-function/20210408-201217
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-a014-20210408 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 56ea2e2fdd691136d5e6631fa0e447173694b82c)

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

If you fix the issue, kindly add following tag as appropriate

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

All warnings (new ones prefixed by >>):

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

drivers/media/usb/em28xx/em28xx-video.c:2545:1: note: declare 'static' if the function is not intended to be used outside of this translation unit void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
^
static
2 warnings generated.

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

2346

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

2546 {
2547 kfree(v4l2_dev);
2548 }
2549

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

#2 lore raw
In reply to Igor Matheus Andrade Torrente
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 9 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 v4l2 subsystem deregistration, thread[1] will free all resources before the em28xx_v4l2_open() can process their things, because the em28xx_v4l2_init() has the dev->lock. And all this lead the thread[2] to cause a user-after-free.

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

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

Quoted 391 lines
Reported-and-tested-by [email protected]
Signed-off-by Igor Matheus Andrade Torrente <[email protected]>
---
  drivers/media/usb/em28xx/em28xx-camera.c |   4 +-
  drivers/media/usb/em28xx/em28xx-video.c  | 188 ++++++++++++++---------
  drivers/media/usb/em28xx/em28xx.h        |   6 +-
  3 files changed, 123 insertions(+), 75 deletions(-)
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 +79 -40
index 6b84c3413e83..e1febb2bf06b 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,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
 
  	if (vdev->vfl_type == VFL_TYPE_RADIO) {
  		em28xx_videodbg("video_open: setting radio device\n");
-		v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
+		v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
  	}
 
  	kref_get(&dev->ref);

@@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
 
  	mutex_lock(&dev->lock);
 
-	v4l2_device_disconnect(&v4l2->v4l2_dev);
+	v4l2_device_disconnect(v4l2->v4l2_dev);
 
  	em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
 

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

@@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
  			goto exit;
 
  		/* Save some power by putting tuner to sleep */
-		v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+		v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
 
  		/* do this before setting alternate! */
  		em28xx_set_mode(dev, EM28XX_SUSPEND);

@@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
  	return 0;
  }
 
+void em28xx_vdev_release(struct video_device *vdev)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+	int i;
+
+	for (i = 0; i < vdev->entity.num_pads; i++)
+		kfree(&vdev->entity.pads[i]);
+#endif
+	kfree(vdev);
+}
+
  static const struct v4l2_file_operations em28xx_v4l_fops = {
  	.owner         = THIS_MODULE,
  	.open          = em28xx_v4l2_open,

@@ -2387,7 +2412,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
  static const struct video_device em28xx_video_template = {
  	.fops		= &em28xx_v4l_fops,
  	.ioctl_ops	= &video_ioctl_ops,
-	.release	= video_device_release_empty,
+	.release	= em28xx_vdev_release,
  	.tvnorms	= V4L2_STD_ALL,
  };
 

@@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
  			     const char *type_name)
  {
  	*vfd		= *template;
-	vfd->v4l2_dev	= &dev->v4l2->v4l2_dev;
+	vfd->v4l2_dev	= dev->v4l2->v4l2_dev;
  	vfd->lock	= &dev->lock;
  	if (dev->is_webcam)
  		vfd->tvnorms = 0;

@@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
  static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
  {
  	struct em28xx_v4l2      *v4l2 = dev->v4l2;
-	struct v4l2_device      *v4l2_dev = &v4l2->v4l2_dev;
+	struct v4l2_device      *v4l2_dev = v4l2->v4l2_dev;
  	struct tuner_setup      tun_setup;
  	struct v4l2_frequency   f;
 

@@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
  	v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
  }
 
+void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+	kfree(v4l2_dev);
+}
+
  static int em28xx_v4l2_init(struct em28xx *dev)
  {
  	u8 val;

@@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 
  	v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
  	if (!v4l2) {
-		mutex_unlock(&dev->lock);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto v4l2_error;
  	}
+
  	kref_init(&v4l2->ref);
  	v4l2->dev = dev;
  	dev->v4l2 = v4l2;
 
+	v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
+	if (!v4l2->v4l2_dev) {
+		ret = -ENOMEM;
+		goto v4l2_dev_error;
+	}
+
+	v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
+
  #ifdef CONFIG_MEDIA_CONTROLLER
-	v4l2->v4l2_dev.mdev = dev->media_dev;
+	v4l2->v4l2_dev->mdev = dev->media_dev;
  #endif
-	ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
+	ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
  	if (ret < 0) {
  		dev_err(&dev->intf->dev,
  			"Call to v4l2_device_register() failed!\n");
-		goto err;
+		goto v4l2_device_register_error;
  	}
 
  	hdl = &v4l2->ctrl_handler;
  	v4l2_ctrl_handler_init(hdl, 8);
-	v4l2->v4l2_dev.ctrl_handler = hdl;
+	v4l2->v4l2_dev->ctrl_handler = hdl;
 
  	if (dev->is_webcam)
  		v4l2->progressive = true;

@@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  	/* request some modules */
 
  	if (dev->has_msp34xx)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+		v4l2_i2c_new_subdev(v4l2->v4l2_dev,
  				    &dev->i2c_adap[dev->def_i2c_bus],
  				    "msp3400", 0, msp3400_addrs);
 
  	if (dev->board.decoder == EM28XX_SAA711X)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+		v4l2_i2c_new_subdev(v4l2->v4l2_dev,
  				    &dev->i2c_adap[dev->def_i2c_bus],
  				    "saa7115_auto", 0, saa711x_addrs);
 
  	if (dev->board.decoder == EM28XX_TVP5150)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+		v4l2_i2c_new_subdev(v4l2->v4l2_dev,
  				    &dev->i2c_adap[dev->def_i2c_bus],
  				    "tvp5150", 0, tvp5150_addrs);
 
  	if (dev->board.adecoder == EM28XX_TVAUDIO)
-		v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+		v4l2_i2c_new_subdev(v4l2->v4l2_dev,
  				    &dev->i2c_adap[dev->def_i2c_bus],
  				    "tvaudio", dev->board.tvaudio_addr, NULL);

drivers/media/usb/em28xx/em28xx-video.c static int em28xx_v4l2_init(struct em28xx *dev) · line 2646

Quoted 8 lines
drivers/media/usb/em28xx/em28xx-video.c +1 -1
@@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  		int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
 
  		if (dev->board.radio.type)
-			v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+			v4l2_i2c_new_subdev(v4l2->v4l2_dev,
  					    &dev->i2c_adap[dev->def_i2c_bus],
  					    "tuner", dev->board.radio_addr,
  					    NULL);

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

drivers/media/usb/em28xx/em28xx-video.c static int em28xx_v4l2_init(struct em28xx *dev) · line 2652

Quoted 7 lines
drivers/media/usb/em28xx/em28xx-video.c +1 -1
  		if (has_demod)
-			v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+			v4l2_i2c_new_subdev(v4l2->v4l2_dev,
  					    &dev->i2c_adap[dev->def_i2c_bus],
  					    "tuner", 0,
  					    v4l2_i2c_tuner_addrs(ADDRS_DEMOD));

Same here:

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

drivers/media/usb/em28xx/em28xx-video.c static int em28xx_v4l2_init(struct em28xx *dev) · line 2664

Quoted 12 lines
drivers/media/usb/em28xx/em28xx-video.c +1 -1
@@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  				has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
  			struct v4l2_subdev *sd;
 
-			sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+			sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
  						 &dev->i2c_adap[dev->def_i2c_bus],
  						 "tuner", 0,
  						 v4l2_i2c_tuner_addrs(type));

@@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  			if (sd)
  				tuner_addr = v4l2_i2c_subdev_addr(sd);

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

drivers/media/usb/em28xx/em28xx-video.c static int em28xx_v4l2_init(struct em28xx *dev) · line 2668

Quoted 5 lines
drivers/media/usb/em28xx/em28xx-video.c +1 -1
  		} else {
-			v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+			v4l2_i2c_new_subdev(v4l2->v4l2_dev,
  					    &dev->i2c_adap[dev->def_i2c_bus],
  					    "tuner", tuner_addr, NULL);

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

Quoted 139 lines
drivers/media/usb/em28xx/em28xx-video.c +35 -26
  		}

@@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 
  	/* set default norm */
  	v4l2->norm = V4L2_STD_PAL;
-	v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
+	v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
  	v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
 
  	/* Analog specific initialization */

@@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  		goto unregister_dev;
 
  	/* allocate and fill video video_device struct */
-	em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
+	v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
+	if (!v4l2->vdev) {
+		ret = -ENOMEM;
+		goto unregister_dev;
+	}
+
+	em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
  	mutex_init(&v4l2->vb_queue_lock);
  	mutex_init(&v4l2->vb_vbi_queue_lock);
-	v4l2->vdev.queue = &v4l2->vb_vidq;
-	v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
-	v4l2->vdev.device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
+	v4l2->vdev->queue = &v4l2->vb_vidq;
+	v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
+	v4l2->vdev->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
  				 V4L2_CAP_STREAMING;
  	if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
-		v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
+		v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
  	if (dev->tuner_type != TUNER_ABSENT)
-		v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
-
+		v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
 
  	/* disable inapplicable ioctls */
  	if (dev->is_webcam) {
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
  	} else {
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
  	}
  	if (dev->tuner_type == TUNER_ABSENT) {
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
  	}
  	if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
-		v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
+		v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
  	}
 
  	/* register v4l2 video video_device */
-	ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
+	ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
  				    video_nr[dev->devno]);
  	if (ret) {
  		dev_err(&dev->intf->dev,

@@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 
  	dev_info(&dev->intf->dev,
  		 "V4L2 video device registered as %s\n",
-		 video_device_node_name(&v4l2->vdev));
+		 video_device_node_name(v4l2->vdev));
 
  	if (video_is_registered(&v4l2->vbi_dev))
  		dev_info(&dev->intf->dev,

@@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  			 video_device_node_name(&v4l2->vbi_dev));
 
  	/* Save some power by putting tuner to sleep */
-	v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+	v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
 
  	/* initialize videobuf2 stuff */
  	em28xx_vb2_setup(dev);

@@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  			 video_device_node_name(&v4l2->vbi_dev));
  		video_unregister_device(&v4l2->vbi_dev);
  	}
-	if (video_is_registered(&v4l2->vdev)) {
+	if (video_is_registered(v4l2->vdev)) {
  		dev_info(&dev->intf->dev,
  			 "V4L2 device %s deregistered\n",
-			 video_device_node_name(&v4l2->vdev));
-		video_unregister_device(&v4l2->vdev);
+			 video_device_node_name(v4l2->vdev));
+		video_unregister_device(v4l2->vdev);
  	}
 
  	v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
-	v4l2_device_unregister(&v4l2->v4l2_dev);
-err:
+	v4l2_device_unregister(v4l2->v4l2_dev);
+
+v4l2_device_register_error:
+	v4l2_device_put(v4l2->v4l2_dev);
+v4l2_dev_error:
  	dev->v4l2 = NULL;
  	kref_put(&v4l2->ref, em28xx_free_v4l2);
+v4l2_error:
  	mutex_unlock(&dev->lock);
  	return ret;
  }
drivers/media/usb/em28xx/em28xx.h +2 -2
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;

drivers/media/usb/em28xx/em28xx.h struct em28xx_v4l2 { · line 607

Quoted 8 lines
drivers/media/usb/em28xx/em28xx.h +1 -1
@@ -601,7 +601,7 @@ struct em28xx_v4l2 {
  	unsigned int field_count;
 
  #ifdef CONFIG_MEDIA_CONTROLLER
-	struct media_pad video_pad, vbi_pad;
+	struct media_pad *video_pad, vbi_pad;
  	struct media_entity *decoder;
  #endif
  };

thanks,
-- Shuah

author
#3 lore raw
Quoted 16 lines · Shuah Khan
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 9 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 v4l2 subsystem deregistration, thread[1] will free all resources before the em28xx_v4l2_open() can process their things, because the em28xx_v4l2_init() has the dev->lock. And all this lead the thread[2] to cause a user-after-free.

Quoted 3 lines

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

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

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

Quoted 4 lines

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

OK. Let me see what I can do.

Quoted 362 lines
Quoted 362 lines
Reported-and-tested-by [email protected]
Signed-off-by Igor Matheus Andrade Torrente <[email protected]>

---
  drivers/media/usb/em28xx/em28xx-camera.c |   4 +- drivers/media/usb/em28xx/em28xx-video.c  | 188 ++++++++++++++--------- drivers/media/usb/em28xx/em28xx.h        |   6 +-

  3 files changed, 123 insertions(+), 75 deletions(-)
drivers/media/usb/em28xx/em28xx-camera.c +0 -0
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;

drivers/media/usb/em28xx/em28xx-camera.c +0 -0
@@ -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 +11 -3
index 6b84c3413e83..e1febb2bf06b 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");

drivers/media/usb/em28xx/em28xx-video.c +0 -0
@@ -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++;

drivers/media/usb/em28xx/em28xx-video.c +6 -6
@@ -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;

drivers/media/usb/em28xx/em28xx-video.c +1 -1
@@ -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;

drivers/media/usb/em28xx/em28xx-video.c +5 -5
@@ -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;

drivers/media/usb/em28xx/em28xx-video.c +0 -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;
  }

drivers/media/usb/em28xx/em28xx-video.c +53 -23
@@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void 
 *priv,
       strscpy(t->name, "Radio", sizeof(t->name));
-    v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
+    v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
       return 0;
   }

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

@@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
       if (mutex_lock_interruptible(&dev->lock))
           return -ERESTARTSYS;
+    if (!dev->v4l2) {
+        mutex_unlock(&dev->lock);
+        return -ENODEV;
+    }
+
       ret = v4l2_fh_open(filp);
       if (ret) {
           dev_err(&dev->intf->dev,

@@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
       if (vdev->vfl_type == VFL_TYPE_RADIO) {
           em28xx_videodbg("video_open: setting radio device\n");
-        v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
+        v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
       }
       kref_get(&dev->ref);

@@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
       mutex_lock(&dev->lock);
-    v4l2_device_disconnect(&v4l2->v4l2_dev);
+    v4l2_device_disconnect(v4l2->v4l2_dev);
       em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);

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

@@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
               goto exit;
           /* Save some power by putting tuner to sleep */
-        v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+        v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
           /* do this before setting alternate! */
           em28xx_set_mode(dev, EM28XX_SUSPEND);

@@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
       return 0;
   }
+void em28xx_vdev_release(struct video_device *vdev)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+    int i;
+
+    for (i = 0; i < vdev->entity.num_pads; i++)
+        kfree(&vdev->entity.pads[i]);
+#endif
+    kfree(vdev);
+}
+
   static const struct v4l2_file_operations em28xx_v4l_fops = {
       .owner         = THIS_MODULE,
       .open          = em28xx_v4l2_open,

@@ -2387,7 +2412,7 @@ static const struct v4l2_ioctl_ops 
 video_ioctl_ops = {
   static const struct video_device em28xx_video_template = {
       .fops        = &em28xx_v4l_fops,
       .ioctl_ops    = &video_ioctl_ops,
-    .release    = video_device_release_empty,
+    .release    = em28xx_vdev_release,
       .tvnorms    = V4L2_STD_ALL,
   };

@@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
                    const char *type_name)
   {
       *vfd        = *template;
-    vfd->v4l2_dev    = &dev->v4l2->v4l2_dev;
+    vfd->v4l2_dev    = dev->v4l2->v4l2_dev;
       vfd->lock    = &dev->lock;
       if (dev->is_webcam)
           vfd->tvnorms = 0;

@@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
   static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
   {
       struct em28xx_v4l2      *v4l2 = dev->v4l2;
-    struct v4l2_device      *v4l2_dev = &v4l2->v4l2_dev;
+    struct v4l2_device      *v4l2_dev = v4l2->v4l2_dev;
       struct tuner_setup      tun_setup;
       struct v4l2_frequency   f;

@@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx 
 *dev, unsigned short tuner_addr)
       v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
   }
+void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+    kfree(v4l2_dev);
+}
+
   static int em28xx_v4l2_init(struct em28xx *dev)
   {
       u8 val;

@@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
       v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
       if (!v4l2) {
-        mutex_unlock(&dev->lock);
-        return -ENOMEM;
+        ret = -ENOMEM;
+        goto v4l2_error;
       }
+
       kref_init(&v4l2->ref);
       v4l2->dev = dev;
       dev->v4l2 = v4l2;
+    v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
+    if (!v4l2->v4l2_dev) {
+        ret = -ENOMEM;
+        goto v4l2_dev_error;
+    }
+
+    v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
+
   #ifdef CONFIG_MEDIA_CONTROLLER
-    v4l2->v4l2_dev.mdev = dev->media_dev;
+    v4l2->v4l2_dev->mdev = dev->media_dev;
   #endif
-    ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
+    ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
       if (ret < 0) {
           dev_err(&dev->intf->dev,
               "Call to v4l2_device_register() failed!\n");
-        goto err;
+        goto v4l2_device_register_error;
       }
       hdl = &v4l2->ctrl_handler;
       v4l2_ctrl_handler_init(hdl, 8);
-    v4l2->v4l2_dev.ctrl_handler = hdl;
+    v4l2->v4l2_dev->ctrl_handler = hdl;
       if (dev->is_webcam)
           v4l2->progressive = true;

@@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
       /* request some modules */
       if (dev->has_msp34xx)
-        v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+        v4l2_i2c_new_subdev(v4l2->v4l2_dev,
                       &dev->i2c_adap[dev->def_i2c_bus],
                       "msp3400", 0, msp3400_addrs);
       if (dev->board.decoder == EM28XX_SAA711X)
-        v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+        v4l2_i2c_new_subdev(v4l2->v4l2_dev,
                       &dev->i2c_adap[dev->def_i2c_bus],
                       "saa7115_auto", 0, saa711x_addrs);
       if (dev->board.decoder == EM28XX_TVP5150)
-        v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+        v4l2_i2c_new_subdev(v4l2->v4l2_dev,
                       &dev->i2c_adap[dev->def_i2c_bus],
                       "tvp5150", 0, tvp5150_addrs);
       if (dev->board.adecoder == EM28XX_TVAUDIO)
-        v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+        v4l2_i2c_new_subdev(v4l2->v4l2_dev,
                       &dev->i2c_adap[dev->def_i2c_bus],
                       "tvaudio", dev->board.tvaudio_addr, NULL);

@@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
           int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
           if (dev->board.radio.type)
-            v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
Quoted 8 lines

drivers/media/usb/em28xx/em28xx-video.c static int em28xx_v4l2_init(struct em28xx *dev) · line 2646

Quoted 4 lines
drivers/media/usb/em28xx/em28xx-video.c +1 -0
+            v4l2_i2c_new_subdev(v4l2->v4l2_dev,
                           &dev->i2c_adap[dev->def_i2c_bus],
                           "tuner", dev->board.radio_addr,
                           NULL);

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

I will add to all bellow.

Quoted 170 lines

drivers/media/usb/em28xx/em28xx-video.c static int em28xx_v4l2_init(struct em28xx *dev) · line 2652

Quoted 7 lines
drivers/media/usb/em28xx/em28xx-video.c +1 -1
           if (has_demod)
-            v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+            v4l2_i2c_new_subdev(v4l2->v4l2_dev,
                           &dev->i2c_adap[dev->def_i2c_bus],
                           "tuner", 0,
                           v4l2_i2c_tuner_addrs(ADDRS_DEMOD));

Same here:

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

drivers/media/usb/em28xx/em28xx-video.c static int em28xx_v4l2_init(struct em28xx *dev) · line 2664

Quoted 11 lines
drivers/media/usb/em28xx/em28xx-video.c +1 -1
@@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
                   has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
               struct v4l2_subdev *sd;
-            sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+            sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
                            &dev->i2c_adap[dev->def_i2c_bus],
                            "tuner", 0,
                            v4l2_i2c_tuner_addrs(type));

@@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
               if (sd)
                   tuner_addr = v4l2_i2c_subdev_addr(sd);

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

drivers/media/usb/em28xx/em28xx-video.c static int em28xx_v4l2_init(struct em28xx *dev) · line 2668

Quoted 5 lines
drivers/media/usb/em28xx/em28xx-video.c +1 -1
           } else {
-            v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+            v4l2_i2c_new_subdev(v4l2->v4l2_dev,
                           &dev->i2c_adap[dev->def_i2c_bus],
                           "tuner", tuner_addr, NULL);

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

Quoted 130 lines
drivers/media/usb/em28xx/em28xx-video.c +8 -5
           }

@@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
       /* set default norm */
       v4l2->norm = V4L2_STD_PAL;
-    v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
+    v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
       v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
       /* Analog specific initialization */

@@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
           goto unregister_dev;
       /* allocate and fill video video_device struct */
-    em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
+    v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
+    if (!v4l2->vdev) {
+        ret = -ENOMEM;
+        goto unregister_dev;
+    }
+
+    em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
       mutex_init(&v4l2->vb_queue_lock);
       mutex_init(&v4l2->vb_vbi_queue_lock);
-    v4l2->vdev.queue = &v4l2->vb_vidq;
-    v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
-    v4l2->vdev.device_caps = V4L2_CAP_READWRITE |

V4L2_CAP_VIDEO_CAPTURE |
+    v4l2->vdev->queue = &v4l2->vb_vidq;
+    v4l2->vdev->queue->lock = &v4l2->vb_queue_lock; +    v4l2->vdev->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
                   V4L2_CAP_STREAMING;
      if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE) -        v4l2->vdev.device_caps |= V4L2_CAP_AUDIO; +        v4l2->vdev->device_caps |= V4L2_CAP_AUDIO; if (dev->tuner_type != TUNER_ABSENT)
-        v4l2->vdev.device_caps |= V4L2_CAP_TUNER; -
+        v4l2->vdev->device_caps |= V4L2_CAP_TUNER; /* disable inapplicable ioctls */
      if (dev->is_webcam) {
-        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD); -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD); -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD); } else {
-        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM); }
      if (dev->tuner_type == TUNER_ABSENT) {
-        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER); -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER); -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY); -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY); }
      if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) { -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO); -        v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO); +        v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO); }
      /* register v4l2 video video_device */
-    ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO, +    ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO, video_nr[dev->devno]);
      if (ret) {
          dev_err(&dev->intf->dev,

drivers/media/usb/em28xx/em28xx-video.c +11 -7
@@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
       dev_info(&dev->intf->dev,
            "V4L2 video device registered as %s\n",
-         video_device_node_name(&v4l2->vdev));
+         video_device_node_name(v4l2->vdev));
       if (video_is_registered(&v4l2->vbi_dev))
           dev_info(&dev->intf->dev,

@@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
                video_device_node_name(&v4l2->vbi_dev));
       /* Save some power by putting tuner to sleep */
-    v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+    v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
       /* initialize videobuf2 stuff */
       em28xx_vb2_setup(dev);

@@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
                video_device_node_name(&v4l2->vbi_dev));
           video_unregister_device(&v4l2->vbi_dev);
       }
-    if (video_is_registered(&v4l2->vdev)) {
+    if (video_is_registered(v4l2->vdev)) {
           dev_info(&dev->intf->dev,
                "V4L2 device %s deregistered\n",
-             video_device_node_name(&v4l2->vdev));
-        video_unregister_device(&v4l2->vdev);
+             video_device_node_name(v4l2->vdev));
+        video_unregister_device(v4l2->vdev);
       }
       v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
-    v4l2_device_unregister(&v4l2->v4l2_dev);
-err:
+    v4l2_device_unregister(v4l2->v4l2_dev);
+
+v4l2_device_register_error:
+    v4l2_device_put(v4l2->v4l2_dev);
+v4l2_dev_error:
       dev->v4l2 = NULL;
       kref_put(&v4l2->ref, em28xx_free_v4l2);
+v4l2_error:
       mutex_unlock(&dev->lock);
       return ret;
   }
drivers/media/usb/em28xx/em28xx.h +2 -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;
Quoted 8 lines

drivers/media/usb/em28xx/em28xx.h struct em28xx_v4l2 {

Quoted 5 lines
drivers/media/usb/em28xx/em28xx.h +1 -0
+    struct media_pad *video_pad, vbi_pad;
       struct media_entity *decoder;
   #endif
   };

thanks,
-- Shuah

Thanks,
---
Igor Matheus Andrade Torrente