* [PATCH] media: em28xx: Fix race condition between open and init function
@ 2021-04-08 12:10 ` Igor Matheus Andrade Torrente
0 siblings, 0 replies; 9+ messages in thread
From: Igor Matheus Andrade Torrente @ 2021-04-08 12:10 UTC (permalink / raw)
To: mchehab
Cc: Igor Matheus Andrade Torrente, linux-media, linux-kernel,
linux-kernel-mentees, syzbot+b2391895514ed9ef4a8e
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: syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com
Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>
---
drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
drivers/media/usb/em28xx/em28xx-video.c | 188 ++++++++++++++---------
drivers/media/usb/em28xx/em28xx.h | 6 +-
3 files changed, 123 insertions(+), 75 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
index d1e66b503f4d..436c5a8cbbb6 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_xtal = 4300000;
pdata.xtal = v4l2->sensor_xtal;
if (NULL ==
- v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
+ v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
&mt9v011_info, NULL))
return -ENODEV;
v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
@@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_yres = 480;
subdev =
- v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
+ v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
&ov2640_info, NULL);
if (!subdev)
return -ENODEV;
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 6b84c3413e83..e1febb2bf06b 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
*/
static void em28xx_wake_i2c(struct em28xx *dev)
{
- struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
@@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct em28xx *dev)
struct em28xx_v4l2 *v4l2 = dev->v4l2;
int ret, i;
+ v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
+ if (!v4l2->video_pad) {
+ dev_err(&dev->intf->dev,
+ "failed to allocate video pad memory!\n");
+ v4l2->vdev->entity.num_pads = 0;
+ return;
+ }
+
/* Initialize Video, VBI and Radio pads */
- v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
- ret = media_entity_pads_init(&v4l2->vdev.entity, 1, &v4l2->video_pad);
+ v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
+ ret = media_entity_pads_init(&v4l2->vdev->entity, 1, v4l2->video_pad);
if (ret < 0)
dev_err(&dev->intf->dev,
"failed to initialize video media entity!\n");
@@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
f.type = V4L2_TUNER_RADIO;
else
f.type = V4L2_TUNER_ANALOG_TV;
- v4l2_device_call_all(&v4l2->v4l2_dev,
+ v4l2_device_call_all(v4l2->v4l2_dev,
0, tuner, s_frequency, &f);
/* Enable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
}
v4l2->streaming_users++;
@@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct vb2_queue *vq)
if (v4l2->streaming_users-- == 1) {
/* Disable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
/* Last active user, so shutdown all the URBS */
em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
@@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue *vq)
if (v4l2->streaming_users-- == 1) {
/* Disable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
/* Last active user, so shutdown all the URBS */
em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
@@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)
static void video_mux(struct em28xx *dev, int index)
{
- struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
dev->ctl_input = index;
dev->ctl_ainput = INPUT(index)->amux;
@@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file, void *priv, v4l2_std_id *norm)
{
struct em28xx *dev = video_drvdata(file);
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd, norm);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);
return 0;
}
@@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
&v4l2->hscale, &v4l2->vscale);
em28xx_resolution_set(dev);
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
return 0;
}
@@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void *priv,
p->parm.capture.readbuffers = EM28XX_MIN_BUF;
p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
if (dev->is_webcam) {
- rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
+ rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
video, g_frame_interval, &ival);
if (!rc)
p->parm.capture.timeperframe = ival.interval;
@@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void *priv,
memset(&p->parm, 0, sizeof(p->parm));
p->parm.capture.readbuffers = EM28XX_MIN_BUF;
p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
- rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
+ rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
video, s_frame_interval, &ival);
if (!rc)
p->parm.capture.timeperframe = ival.interval;
@@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file, void *priv,
if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
i->type = V4L2_INPUT_TYPE_TUNER;
- i->std = dev->v4l2->vdev.tvnorms;
+ i->std = dev->v4l2->vdev->tvnorms;
/* webcams do not have the STD API */
if (dev->is_webcam)
i->capabilities = 0;
@@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file, void *priv,
strscpy(t->name, "Tuner", sizeof(t->name));
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
return 0;
}
@@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
if (t->index != 0)
return -EINVAL;
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
return 0;
}
@@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
if (f->tuner != 0)
return -EINVAL;
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
v4l2->frequency = new_freq.frequency;
return 0;
@@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file, void *priv,
strscpy(chip->name, "ac97", sizeof(chip->name));
else
strscpy(chip->name,
- dev->v4l2->v4l2_dev.name, sizeof(chip->name));
+ dev->v4l2->v4l2_dev->name, sizeof(chip->name));
return 0;
}
@@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void *priv,
strscpy(t->name, "Radio", sizeof(t->name));
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
return 0;
}
@@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void *priv,
if (t->index != 0)
return -EINVAL;
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
return 0;
}
@@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
if (mutex_lock_interruptible(&dev->lock))
return -ERESTARTSYS;
+ if (!dev->v4l2) {
+ mutex_unlock(&dev->lock);
+ return -ENODEV;
+ }
+
ret = v4l2_fh_open(filp);
if (ret) {
dev_err(&dev->intf->dev,
@@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
if (vdev->vfl_type == VFL_TYPE_RADIO) {
em28xx_videodbg("video_open: setting radio device\n");
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
}
kref_get(&dev->ref);
@@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
mutex_lock(&dev->lock);
- v4l2_device_disconnect(&v4l2->v4l2_dev);
+ v4l2_device_disconnect(v4l2->v4l2_dev);
em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
@@ -2238,14 +2251,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
video_unregister_device(&v4l2->vbi_dev);
}
- if (video_is_registered(&v4l2->vdev)) {
+ if (video_is_registered(v4l2->vdev)) {
dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
- video_device_node_name(&v4l2->vdev));
- video_unregister_device(&v4l2->vdev);
+ video_device_node_name(v4l2->vdev));
+ video_unregister_device(v4l2->vdev);
}
v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
- v4l2_device_unregister(&v4l2->v4l2_dev);
+ v4l2_device_unregister(v4l2->v4l2_dev);
+ v4l2_device_put(v4l2->v4l2_dev);
kref_put(&v4l2->ref, em28xx_free_v4l2);
@@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
goto exit;
/* Save some power by putting tuner to sleep */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
/* do this before setting alternate! */
em28xx_set_mode(dev, EM28XX_SUSPEND);
@@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
return 0;
}
+void em28xx_vdev_release(struct video_device *vdev)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+ int i;
+
+ for (i = 0; i < vdev->entity.num_pads; i++)
+ kfree(&vdev->entity.pads[i]);
+#endif
+ kfree(vdev);
+}
+
static const struct v4l2_file_operations em28xx_v4l_fops = {
.owner = THIS_MODULE,
.open = em28xx_v4l2_open,
@@ -2387,7 +2412,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
static const struct video_device em28xx_video_template = {
.fops = &em28xx_v4l_fops,
.ioctl_ops = &video_ioctl_ops,
- .release = video_device_release_empty,
+ .release = em28xx_vdev_release,
.tvnorms = V4L2_STD_ALL,
};
@@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
const char *type_name)
{
*vfd = *template;
- vfd->v4l2_dev = &dev->v4l2->v4l2_dev;
+ vfd->v4l2_dev = dev->v4l2->v4l2_dev;
vfd->lock = &dev->lock;
if (dev->is_webcam)
vfd->tvnorms = 0;
@@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
{
struct em28xx_v4l2 *v4l2 = dev->v4l2;
- struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
struct tuner_setup tun_setup;
struct v4l2_frequency f;
@@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
}
+void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+ kfree(v4l2_dev);
+}
+
static int em28xx_v4l2_init(struct em28xx *dev)
{
u8 val;
@@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
if (!v4l2) {
- mutex_unlock(&dev->lock);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto v4l2_error;
}
+
kref_init(&v4l2->ref);
v4l2->dev = dev;
dev->v4l2 = v4l2;
+ v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
+ if (!v4l2->v4l2_dev) {
+ ret = -ENOMEM;
+ goto v4l2_dev_error;
+ }
+
+ v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
+
#ifdef CONFIG_MEDIA_CONTROLLER
- v4l2->v4l2_dev.mdev = dev->media_dev;
+ v4l2->v4l2_dev->mdev = dev->media_dev;
#endif
- ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
+ ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
if (ret < 0) {
dev_err(&dev->intf->dev,
"Call to v4l2_device_register() failed!\n");
- goto err;
+ goto v4l2_device_register_error;
}
hdl = &v4l2->ctrl_handler;
v4l2_ctrl_handler_init(hdl, 8);
- v4l2->v4l2_dev.ctrl_handler = hdl;
+ v4l2->v4l2_dev->ctrl_handler = hdl;
if (dev->is_webcam)
v4l2->progressive = true;
@@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
/* request some modules */
if (dev->has_msp34xx)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"msp3400", 0, msp3400_addrs);
if (dev->board.decoder == EM28XX_SAA711X)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"saa7115_auto", 0, saa711x_addrs);
if (dev->board.decoder == EM28XX_TVP5150)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tvp5150", 0, tvp5150_addrs);
if (dev->board.adecoder == EM28XX_TVAUDIO)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tvaudio", dev->board.tvaudio_addr, NULL);
@@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
if (dev->board.radio.type)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", dev->board.radio_addr,
NULL);
if (has_demod)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", 0,
v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
@@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
struct v4l2_subdev *sd;
- sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", 0,
v4l2_i2c_tuner_addrs(type));
@@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
if (sd)
tuner_addr = v4l2_i2c_subdev_addr(sd);
} else {
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", tuner_addr, NULL);
}
@@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
/* set default norm */
v4l2->norm = V4L2_STD_PAL;
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
/* Analog specific initialization */
@@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
goto unregister_dev;
/* allocate and fill video video_device struct */
- em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
+ v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
+ if (!v4l2->vdev) {
+ ret = -ENOMEM;
+ goto unregister_dev;
+ }
+
+ em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
mutex_init(&v4l2->vb_queue_lock);
mutex_init(&v4l2->vb_vbi_queue_lock);
- v4l2->vdev.queue = &v4l2->vb_vidq;
- v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
- v4l2->vdev.device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
+ v4l2->vdev->queue = &v4l2->vb_vidq;
+ v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
+ v4l2->vdev->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
V4L2_CAP_STREAMING;
if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
- v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
+ v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
if (dev->tuner_type != TUNER_ABSENT)
- v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
-
+ v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
/* disable inapplicable ioctls */
if (dev->is_webcam) {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
} else {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
}
if (dev->tuner_type == TUNER_ABSENT) {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
}
if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
}
/* register v4l2 video video_device */
- ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
+ ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
video_nr[dev->devno]);
if (ret) {
dev_err(&dev->intf->dev,
@@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
dev_info(&dev->intf->dev,
"V4L2 video device registered as %s\n",
- video_device_node_name(&v4l2->vdev));
+ video_device_node_name(v4l2->vdev));
if (video_is_registered(&v4l2->vbi_dev))
dev_info(&dev->intf->dev,
@@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
/* Save some power by putting tuner to sleep */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
/* initialize videobuf2 stuff */
em28xx_vb2_setup(dev);
@@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
video_unregister_device(&v4l2->vbi_dev);
}
- if (video_is_registered(&v4l2->vdev)) {
+ if (video_is_registered(v4l2->vdev)) {
dev_info(&dev->intf->dev,
"V4L2 device %s deregistered\n",
- video_device_node_name(&v4l2->vdev));
- video_unregister_device(&v4l2->vdev);
+ video_device_node_name(v4l2->vdev));
+ video_unregister_device(v4l2->vdev);
}
v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
- v4l2_device_unregister(&v4l2->v4l2_dev);
-err:
+ v4l2_device_unregister(v4l2->v4l2_dev);
+
+v4l2_device_register_error:
+ v4l2_device_put(v4l2->v4l2_dev);
+v4l2_dev_error:
dev->v4l2 = NULL;
kref_put(&v4l2->ref, em28xx_free_v4l2);
+v4l2_error:
mutex_unlock(&dev->lock);
return ret;
}
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 6648e11f1271..dbcc297b5a0d 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -552,10 +552,10 @@ struct em28xx_v4l2 {
struct kref ref;
struct em28xx *dev;
- struct v4l2_device v4l2_dev;
+ struct v4l2_device *v4l2_dev;
struct v4l2_ctrl_handler ctrl_handler;
- struct video_device vdev;
+ struct video_device *vdev;
struct video_device vbi_dev;
struct video_device radio_dev;
@@ -601,7 +601,7 @@ struct em28xx_v4l2 {
unsigned int field_count;
#ifdef CONFIG_MEDIA_CONTROLLER
- struct media_pad video_pad, vbi_pad;
+ struct media_pad *video_pad, vbi_pad;
struct media_entity *decoder;
#endif
};
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] media: em28xx: Fix race condition between open and init function
@ 2021-04-08 12:10 ` Igor Matheus Andrade Torrente
0 siblings, 0 replies; 9+ messages in thread
From: Igor Matheus Andrade Torrente @ 2021-04-08 12:10 UTC (permalink / raw)
To: mchehab
Cc: syzbot+b2391895514ed9ef4a8e, linux-media, linux-kernel-mentees,
linux-kernel, 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.
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: syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com
Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>
---
drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
drivers/media/usb/em28xx/em28xx-video.c | 188 ++++++++++++++---------
drivers/media/usb/em28xx/em28xx.h | 6 +-
3 files changed, 123 insertions(+), 75 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
index d1e66b503f4d..436c5a8cbbb6 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_xtal = 4300000;
pdata.xtal = v4l2->sensor_xtal;
if (NULL ==
- v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
+ v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
&mt9v011_info, NULL))
return -ENODEV;
v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
@@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
v4l2->sensor_yres = 480;
subdev =
- v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
+ v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
&ov2640_info, NULL);
if (!subdev)
return -ENODEV;
diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 6b84c3413e83..e1febb2bf06b 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
*/
static void em28xx_wake_i2c(struct em28xx *dev)
{
- struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
@@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct em28xx *dev)
struct em28xx_v4l2 *v4l2 = dev->v4l2;
int ret, i;
+ v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
+ if (!v4l2->video_pad) {
+ dev_err(&dev->intf->dev,
+ "failed to allocate video pad memory!\n");
+ v4l2->vdev->entity.num_pads = 0;
+ return;
+ }
+
/* Initialize Video, VBI and Radio pads */
- v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
- ret = media_entity_pads_init(&v4l2->vdev.entity, 1, &v4l2->video_pad);
+ v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
+ ret = media_entity_pads_init(&v4l2->vdev->entity, 1, v4l2->video_pad);
if (ret < 0)
dev_err(&dev->intf->dev,
"failed to initialize video media entity!\n");
@@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
f.type = V4L2_TUNER_RADIO;
else
f.type = V4L2_TUNER_ANALOG_TV;
- v4l2_device_call_all(&v4l2->v4l2_dev,
+ v4l2_device_call_all(v4l2->v4l2_dev,
0, tuner, s_frequency, &f);
/* Enable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
}
v4l2->streaming_users++;
@@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct vb2_queue *vq)
if (v4l2->streaming_users-- == 1) {
/* Disable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
/* Last active user, so shutdown all the URBS */
em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
@@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue *vq)
if (v4l2->streaming_users-- == 1) {
/* Disable video stream at TV decoder */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
/* Last active user, so shutdown all the URBS */
em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
@@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)
static void video_mux(struct em28xx *dev, int index)
{
- struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
dev->ctl_input = index;
dev->ctl_ainput = INPUT(index)->amux;
@@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file, void *priv, v4l2_std_id *norm)
{
struct em28xx *dev = video_drvdata(file);
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd, norm);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);
return 0;
}
@@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
&v4l2->hscale, &v4l2->vscale);
em28xx_resolution_set(dev);
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
return 0;
}
@@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void *priv,
p->parm.capture.readbuffers = EM28XX_MIN_BUF;
p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
if (dev->is_webcam) {
- rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
+ rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
video, g_frame_interval, &ival);
if (!rc)
p->parm.capture.timeperframe = ival.interval;
@@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void *priv,
memset(&p->parm, 0, sizeof(p->parm));
p->parm.capture.readbuffers = EM28XX_MIN_BUF;
p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
- rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
+ rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
video, s_frame_interval, &ival);
if (!rc)
p->parm.capture.timeperframe = ival.interval;
@@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file, void *priv,
if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
i->type = V4L2_INPUT_TYPE_TUNER;
- i->std = dev->v4l2->vdev.tvnorms;
+ i->std = dev->v4l2->vdev->tvnorms;
/* webcams do not have the STD API */
if (dev->is_webcam)
i->capabilities = 0;
@@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file, void *priv,
strscpy(t->name, "Tuner", sizeof(t->name));
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
return 0;
}
@@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
if (t->index != 0)
return -EINVAL;
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
return 0;
}
@@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
if (f->tuner != 0)
return -EINVAL;
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
v4l2->frequency = new_freq.frequency;
return 0;
@@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file, void *priv,
strscpy(chip->name, "ac97", sizeof(chip->name));
else
strscpy(chip->name,
- dev->v4l2->v4l2_dev.name, sizeof(chip->name));
+ dev->v4l2->v4l2_dev->name, sizeof(chip->name));
return 0;
}
@@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void *priv,
strscpy(t->name, "Radio", sizeof(t->name));
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
return 0;
}
@@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void *priv,
if (t->index != 0)
return -EINVAL;
- v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
+ v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
return 0;
}
@@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
if (mutex_lock_interruptible(&dev->lock))
return -ERESTARTSYS;
+ if (!dev->v4l2) {
+ mutex_unlock(&dev->lock);
+ return -ENODEV;
+ }
+
ret = v4l2_fh_open(filp);
if (ret) {
dev_err(&dev->intf->dev,
@@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
if (vdev->vfl_type == VFL_TYPE_RADIO) {
em28xx_videodbg("video_open: setting radio device\n");
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
}
kref_get(&dev->ref);
@@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
mutex_lock(&dev->lock);
- v4l2_device_disconnect(&v4l2->v4l2_dev);
+ v4l2_device_disconnect(v4l2->v4l2_dev);
em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
@@ -2238,14 +2251,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
video_unregister_device(&v4l2->vbi_dev);
}
- if (video_is_registered(&v4l2->vdev)) {
+ if (video_is_registered(v4l2->vdev)) {
dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
- video_device_node_name(&v4l2->vdev));
- video_unregister_device(&v4l2->vdev);
+ video_device_node_name(v4l2->vdev));
+ video_unregister_device(v4l2->vdev);
}
v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
- v4l2_device_unregister(&v4l2->v4l2_dev);
+ v4l2_device_unregister(v4l2->v4l2_dev);
+ v4l2_device_put(v4l2->v4l2_dev);
kref_put(&v4l2->ref, em28xx_free_v4l2);
@@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
goto exit;
/* Save some power by putting tuner to sleep */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
/* do this before setting alternate! */
em28xx_set_mode(dev, EM28XX_SUSPEND);
@@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
return 0;
}
+void em28xx_vdev_release(struct video_device *vdev)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+ int i;
+
+ for (i = 0; i < vdev->entity.num_pads; i++)
+ kfree(&vdev->entity.pads[i]);
+#endif
+ kfree(vdev);
+}
+
static const struct v4l2_file_operations em28xx_v4l_fops = {
.owner = THIS_MODULE,
.open = em28xx_v4l2_open,
@@ -2387,7 +2412,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
static const struct video_device em28xx_video_template = {
.fops = &em28xx_v4l_fops,
.ioctl_ops = &video_ioctl_ops,
- .release = video_device_release_empty,
+ .release = em28xx_vdev_release,
.tvnorms = V4L2_STD_ALL,
};
@@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
const char *type_name)
{
*vfd = *template;
- vfd->v4l2_dev = &dev->v4l2->v4l2_dev;
+ vfd->v4l2_dev = dev->v4l2->v4l2_dev;
vfd->lock = &dev->lock;
if (dev->is_webcam)
vfd->tvnorms = 0;
@@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
{
struct em28xx_v4l2 *v4l2 = dev->v4l2;
- struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
+ struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
struct tuner_setup tun_setup;
struct v4l2_frequency f;
@@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
}
+void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
+{
+ kfree(v4l2_dev);
+}
+
static int em28xx_v4l2_init(struct em28xx *dev)
{
u8 val;
@@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
if (!v4l2) {
- mutex_unlock(&dev->lock);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto v4l2_error;
}
+
kref_init(&v4l2->ref);
v4l2->dev = dev;
dev->v4l2 = v4l2;
+ v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
+ if (!v4l2->v4l2_dev) {
+ ret = -ENOMEM;
+ goto v4l2_dev_error;
+ }
+
+ v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
+
#ifdef CONFIG_MEDIA_CONTROLLER
- v4l2->v4l2_dev.mdev = dev->media_dev;
+ v4l2->v4l2_dev->mdev = dev->media_dev;
#endif
- ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
+ ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
if (ret < 0) {
dev_err(&dev->intf->dev,
"Call to v4l2_device_register() failed!\n");
- goto err;
+ goto v4l2_device_register_error;
}
hdl = &v4l2->ctrl_handler;
v4l2_ctrl_handler_init(hdl, 8);
- v4l2->v4l2_dev.ctrl_handler = hdl;
+ v4l2->v4l2_dev->ctrl_handler = hdl;
if (dev->is_webcam)
v4l2->progressive = true;
@@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
/* request some modules */
if (dev->has_msp34xx)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"msp3400", 0, msp3400_addrs);
if (dev->board.decoder == EM28XX_SAA711X)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"saa7115_auto", 0, saa711x_addrs);
if (dev->board.decoder == EM28XX_TVP5150)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tvp5150", 0, tvp5150_addrs);
if (dev->board.adecoder == EM28XX_TVAUDIO)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tvaudio", dev->board.tvaudio_addr, NULL);
@@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
if (dev->board.radio.type)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", dev->board.radio_addr,
NULL);
if (has_demod)
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", 0,
v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
@@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
struct v4l2_subdev *sd;
- sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", 0,
v4l2_i2c_tuner_addrs(type));
@@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
if (sd)
tuner_addr = v4l2_i2c_subdev_addr(sd);
} else {
- v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
+ v4l2_i2c_new_subdev(v4l2->v4l2_dev,
&dev->i2c_adap[dev->def_i2c_bus],
"tuner", tuner_addr, NULL);
}
@@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
/* set default norm */
v4l2->norm = V4L2_STD_PAL;
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
/* Analog specific initialization */
@@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
goto unregister_dev;
/* allocate and fill video video_device struct */
- em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
+ v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
+ if (!v4l2->vdev) {
+ ret = -ENOMEM;
+ goto unregister_dev;
+ }
+
+ em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
mutex_init(&v4l2->vb_queue_lock);
mutex_init(&v4l2->vb_vbi_queue_lock);
- v4l2->vdev.queue = &v4l2->vb_vidq;
- v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
- v4l2->vdev.device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
+ v4l2->vdev->queue = &v4l2->vb_vidq;
+ v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
+ v4l2->vdev->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
V4L2_CAP_STREAMING;
if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
- v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
+ v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
if (dev->tuner_type != TUNER_ABSENT)
- v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
-
+ v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
/* disable inapplicable ioctls */
if (dev->is_webcam) {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
} else {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
}
if (dev->tuner_type == TUNER_ABSENT) {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
}
if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
- v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
+ v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
}
/* register v4l2 video video_device */
- ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
+ ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
video_nr[dev->devno]);
if (ret) {
dev_err(&dev->intf->dev,
@@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
dev_info(&dev->intf->dev,
"V4L2 video device registered as %s\n",
- video_device_node_name(&v4l2->vdev));
+ video_device_node_name(v4l2->vdev));
if (video_is_registered(&v4l2->vbi_dev))
dev_info(&dev->intf->dev,
@@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
/* Save some power by putting tuner to sleep */
- v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
+ v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
/* initialize videobuf2 stuff */
em28xx_vb2_setup(dev);
@@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
video_device_node_name(&v4l2->vbi_dev));
video_unregister_device(&v4l2->vbi_dev);
}
- if (video_is_registered(&v4l2->vdev)) {
+ if (video_is_registered(v4l2->vdev)) {
dev_info(&dev->intf->dev,
"V4L2 device %s deregistered\n",
- video_device_node_name(&v4l2->vdev));
- video_unregister_device(&v4l2->vdev);
+ video_device_node_name(v4l2->vdev));
+ video_unregister_device(v4l2->vdev);
}
v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
- v4l2_device_unregister(&v4l2->v4l2_dev);
-err:
+ v4l2_device_unregister(v4l2->v4l2_dev);
+
+v4l2_device_register_error:
+ v4l2_device_put(v4l2->v4l2_dev);
+v4l2_dev_error:
dev->v4l2 = NULL;
kref_put(&v4l2->ref, em28xx_free_v4l2);
+v4l2_error:
mutex_unlock(&dev->lock);
return ret;
}
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 6648e11f1271..dbcc297b5a0d 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -552,10 +552,10 @@ struct em28xx_v4l2 {
struct kref ref;
struct em28xx *dev;
- struct v4l2_device v4l2_dev;
+ struct v4l2_device *v4l2_dev;
struct v4l2_ctrl_handler ctrl_handler;
- struct video_device vdev;
+ struct video_device *vdev;
struct video_device vbi_dev;
struct video_device radio_dev;
@@ -601,7 +601,7 @@ struct em28xx_v4l2 {
unsigned int field_count;
#ifdef CONFIG_MEDIA_CONTROLLER
- struct media_pad video_pad, vbi_pad;
+ struct media_pad *video_pad, vbi_pad;
struct media_entity *decoder;
#endif
};
--
2.20.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] media: em28xx: Fix race condition between open and init function
2021-04-08 12:10 ` Igor Matheus Andrade Torrente
(?)
@ 2021-04-08 15:38 ` kernel test robot
-1 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-04-08 15:38 UTC (permalink / raw)
To: Igor Matheus Andrade Torrente, mchehab
Cc: kbuild-all, clang-built-linux, Igor Matheus Andrade Torrente,
linux-media, linux-kernel, linux-kernel-mentees,
syzbot+b2391895514ed9ef4a8e
[-- Attachment #1: Type: text/plain, Size: 10319 bytes --]
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 <lkp@intel.com>
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@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 46852 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: em28xx: Fix race condition between open and init function
@ 2021-04-08 15:38 ` kernel test robot
0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-04-08 15:38 UTC (permalink / raw)
To: Igor Matheus Andrade Torrente, mchehab
Cc: kbuild-all, linux-kernel, clang-built-linux,
syzbot+b2391895514ed9ef4a8e, Igor Matheus Andrade Torrente,
linux-kernel-mentees, linux-media
[-- Attachment #1: Type: text/plain, Size: 10319 bytes --]
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 <lkp@intel.com>
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@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 46852 bytes --]
[-- Attachment #3: Type: text/plain, Size: 201 bytes --]
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: em28xx: Fix race condition between open and init function
@ 2021-04-08 15:38 ` kernel test robot
0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-04-08 15:38 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 10579 bytes --]
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 <lkp@intel.com>
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
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 46852 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: em28xx: Fix race condition between open and init function
2021-04-08 12:10 ` Igor Matheus Andrade Torrente
@ 2021-04-08 17:37 ` Shuah Khan
-1 siblings, 0 replies; 9+ messages in thread
From: Shuah Khan @ 2021-04-08 17:37 UTC (permalink / raw)
To: Igor Matheus Andrade Torrente, mchehab
Cc: syzbot+b2391895514ed9ef4a8e, linux-media, linux-kernel-mentees,
linux-kernel, Shuah Khan
On 4/8/21 6:10 AM, Igor Matheus Andrade Torrente wrote:
> Fixes a race condition - for lack of a more precise term - between
> em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
> media_pad and vdev structs from the em28xx_v4l2, and managing the
> lifetime of those objects more dynamicaly.
>
> The race happens when a thread[1] - containing the em28xx_v4l2_init()
> code - calls the v4l2_mc_create_media_graph(), and it return a error,
> if a thread[2] - running v4l2_open() - pass the verification point
> and reaches the em28xx_v4l2_open() before the thread[1] finishes
> the v4l2 subsystem deregistration, thread[1] will free all resources
> before the em28xx_v4l2_open() can process their things,
> because the em28xx_v4l2_init() has the dev->lock. And all this lead
> the thread[2] to cause a user-after-free.
>
Have you tried this patch with em28xx device? You will have to take into
account the dependencies between the subdevs using the v4l2_dev.
Also try rmmod invidual drivers - what happens if you were to rmmod a
subdev driver? With v4l2_dev is not embedded in v4l2, this could open
up memory leaks or user-after-frees.
> Reported-and-tested-by: syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com
> Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>
> ---
> drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
> drivers/media/usb/em28xx/em28xx-video.c | 188 ++++++++++++++---------
> drivers/media/usb/em28xx/em28xx.h | 6 +-
> 3 files changed, 123 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> index d1e66b503f4d..436c5a8cbbb6 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
> v4l2->sensor_xtal = 4300000;
> pdata.xtal = v4l2->sensor_xtal;
> if (NULL ==
> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
> &mt9v011_info, NULL))
> return -ENODEV;
> v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
> @@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
> v4l2->sensor_yres = 480;
>
> subdev =
> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
> &ov2640_info, NULL);
> if (!subdev)
> return -ENODEV;
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 6b84c3413e83..e1febb2bf06b 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
> */
> static void em28xx_wake_i2c(struct em28xx *dev)
> {
> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>
> v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
> v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
> @@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct em28xx *dev)
> struct em28xx_v4l2 *v4l2 = dev->v4l2;
> int ret, i;
>
> + v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
> + if (!v4l2->video_pad) {
> + dev_err(&dev->intf->dev,
> + "failed to allocate video pad memory!\n");
> + v4l2->vdev->entity.num_pads = 0;
> + return;
> + }
> +
> /* Initialize Video, VBI and Radio pads */
> - v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
> - ret = media_entity_pads_init(&v4l2->vdev.entity, 1, &v4l2->video_pad);
> + v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
> + ret = media_entity_pads_init(&v4l2->vdev->entity, 1, v4l2->video_pad);
> if (ret < 0)
> dev_err(&dev->intf->dev,
> "failed to initialize video media entity!\n");
> @@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
> f.type = V4L2_TUNER_RADIO;
> else
> f.type = V4L2_TUNER_ANALOG_TV;
> - v4l2_device_call_all(&v4l2->v4l2_dev,
> + v4l2_device_call_all(v4l2->v4l2_dev,
> 0, tuner, s_frequency, &f);
>
> /* Enable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
> }
>
> v4l2->streaming_users++;
> @@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct vb2_queue *vq)
>
> if (v4l2->streaming_users-- == 1) {
> /* Disable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>
> /* Last active user, so shutdown all the URBS */
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
> @@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue *vq)
>
> if (v4l2->streaming_users-- == 1) {
> /* Disable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>
> /* Last active user, so shutdown all the URBS */
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
> @@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)
>
> static void video_mux(struct em28xx *dev, int index)
> {
> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>
> dev->ctl_input = index;
> dev->ctl_ainput = INPUT(index)->amux;
> @@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file, void *priv, v4l2_std_id *norm)
> {
> struct em28xx *dev = video_drvdata(file);
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd, norm);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);
>
> return 0;
> }
> @@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
> &v4l2->hscale, &v4l2->vscale);
>
> em28xx_resolution_set(dev);
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>
> return 0;
> }
> @@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void *priv,
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> if (dev->is_webcam) {
> - rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
> + rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
> video, g_frame_interval, &ival);
> if (!rc)
> p->parm.capture.timeperframe = ival.interval;
> @@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void *priv,
> memset(&p->parm, 0, sizeof(p->parm));
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> - rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
> + rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
> video, s_frame_interval, &ival);
> if (!rc)
> p->parm.capture.timeperframe = ival.interval;
> @@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file, void *priv,
> if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
> i->type = V4L2_INPUT_TYPE_TUNER;
>
> - i->std = dev->v4l2->vdev.tvnorms;
> + i->std = dev->v4l2->vdev->tvnorms;
> /* webcams do not have the STD API */
> if (dev->is_webcam)
> i->capabilities = 0;
> @@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file, void *priv,
>
> strscpy(t->name, "Tuner", sizeof(t->name));
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> return 0;
> }
>
> @@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
> if (t->index != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> return 0;
> }
>
> @@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
> if (f->tuner != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
> v4l2->frequency = new_freq.frequency;
>
> return 0;
> @@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file, void *priv,
> strscpy(chip->name, "ac97", sizeof(chip->name));
> else
> strscpy(chip->name,
> - dev->v4l2->v4l2_dev.name, sizeof(chip->name));
> + dev->v4l2->v4l2_dev->name, sizeof(chip->name));
> return 0;
> }
>
> @@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void *priv,
>
> strscpy(t->name, "Radio", sizeof(t->name));
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>
> return 0;
> }
> @@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void *priv,
> if (t->index != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>
> return 0;
> }
> @@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
> if (mutex_lock_interruptible(&dev->lock))
> return -ERESTARTSYS;
>
> + if (!dev->v4l2) {
> + mutex_unlock(&dev->lock);
> + return -ENODEV;
> + }
> +
> ret = v4l2_fh_open(filp);
> if (ret) {
> dev_err(&dev->intf->dev,
> @@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
>
> if (vdev->vfl_type == VFL_TYPE_RADIO) {
> em28xx_videodbg("video_open: setting radio device\n");
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
> }
>
> kref_get(&dev->ref);
> @@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>
> mutex_lock(&dev->lock);
>
> - v4l2_device_disconnect(&v4l2->v4l2_dev);
> + v4l2_device_disconnect(v4l2->v4l2_dev);
>
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>
> @@ -2238,14 +2251,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
> video_unregister_device(&v4l2->vbi_dev);
> }
> - if (video_is_registered(&v4l2->vdev)) {
> + if (video_is_registered(v4l2->vdev)) {
> dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
> - video_device_node_name(&v4l2->vdev));
> - video_unregister_device(&v4l2->vdev);
> + video_device_node_name(v4l2->vdev));
> + video_unregister_device(v4l2->vdev);
> }
>
> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
> - v4l2_device_unregister(&v4l2->v4l2_dev);
> + v4l2_device_unregister(v4l2->v4l2_dev);
> + v4l2_device_put(v4l2->v4l2_dev);
>
> kref_put(&v4l2->ref, em28xx_free_v4l2);
>
> @@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
> goto exit;
>
> /* Save some power by putting tuner to sleep */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>
> /* do this before setting alternate! */
> em28xx_set_mode(dev, EM28XX_SUSPEND);
> @@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
> return 0;
> }
>
> +void em28xx_vdev_release(struct video_device *vdev)
> +{
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + int i;
> +
> + for (i = 0; i < vdev->entity.num_pads; i++)
> + kfree(&vdev->entity.pads[i]);
> +#endif
> + kfree(vdev);
> +}
> +
> static const struct v4l2_file_operations em28xx_v4l_fops = {
> .owner = THIS_MODULE,
> .open = em28xx_v4l2_open,
> @@ -2387,7 +2412,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
> static const struct video_device em28xx_video_template = {
> .fops = &em28xx_v4l_fops,
> .ioctl_ops = &video_ioctl_ops,
> - .release = video_device_release_empty,
> + .release = em28xx_vdev_release,
> .tvnorms = V4L2_STD_ALL,
> };
>
> @@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
> const char *type_name)
> {
> *vfd = *template;
> - vfd->v4l2_dev = &dev->v4l2->v4l2_dev;
> + vfd->v4l2_dev = dev->v4l2->v4l2_dev;
> vfd->lock = &dev->lock;
> if (dev->is_webcam)
> vfd->tvnorms = 0;
> @@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
> static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
> {
> struct em28xx_v4l2 *v4l2 = dev->v4l2;
> - struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
> struct tuner_setup tun_setup;
> struct v4l2_frequency f;
>
> @@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
> v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
> }
>
> +void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> +{
> + kfree(v4l2_dev);
> +}
> +
> static int em28xx_v4l2_init(struct em28xx *dev)
> {
> u8 val;
> @@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
> if (!v4l2) {
> - mutex_unlock(&dev->lock);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto v4l2_error;
> }
> +
> kref_init(&v4l2->ref);
> v4l2->dev = dev;
> dev->v4l2 = v4l2;
>
> + v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
> + if (!v4l2->v4l2_dev) {
> + ret = -ENOMEM;
> + goto v4l2_dev_error;
> + }
> +
> + v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
> +
> #ifdef CONFIG_MEDIA_CONTROLLER
> - v4l2->v4l2_dev.mdev = dev->media_dev;
> + v4l2->v4l2_dev->mdev = dev->media_dev;
> #endif
> - ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
> + ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
> if (ret < 0) {
> dev_err(&dev->intf->dev,
> "Call to v4l2_device_register() failed!\n");
> - goto err;
> + goto v4l2_device_register_error;
> }
>
> hdl = &v4l2->ctrl_handler;
> v4l2_ctrl_handler_init(hdl, 8);
> - v4l2->v4l2_dev.ctrl_handler = hdl;
> + v4l2->v4l2_dev->ctrl_handler = hdl;
>
> if (dev->is_webcam)
> v4l2->progressive = true;
> @@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> /* request some modules */
>
> if (dev->has_msp34xx)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "msp3400", 0, msp3400_addrs);
>
> if (dev->board.decoder == EM28XX_SAA711X)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "saa7115_auto", 0, saa711x_addrs);
>
> if (dev->board.decoder == EM28XX_TVP5150)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tvp5150", 0, tvp5150_addrs);
>
> if (dev->board.adecoder == EM28XX_TVAUDIO)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tvaudio", dev->board.tvaudio_addr, NULL);
>
> @@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
>
> if (dev->board.radio.type)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", dev->board.radio_addr,
> NULL);
Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.
>
> if (has_demod)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", 0,
> v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
Same here:
Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.
> @@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
> struct v4l2_subdev *sd;
>
> - sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", 0,
> v4l2_i2c_tuner_addrs(type));
> @@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> if (sd)
> tuner_addr = v4l2_i2c_subdev_addr(sd);
Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.
> } else {
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", tuner_addr, NULL);
Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.
> }
> @@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> /* set default norm */
> v4l2->norm = V4L2_STD_PAL;
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
>
> /* Analog specific initialization */
> @@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> goto unregister_dev;
>
> /* allocate and fill video video_device struct */
> - em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
> + v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
> + if (!v4l2->vdev) {
> + ret = -ENOMEM;
> + goto unregister_dev;
> + }
> +
> + em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
> mutex_init(&v4l2->vb_queue_lock);
> mutex_init(&v4l2->vb_vbi_queue_lock);
> - v4l2->vdev.queue = &v4l2->vb_vidq;
> - v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
> - v4l2->vdev.device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
> + v4l2->vdev->queue = &v4l2->vb_vidq;
> + v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
> + v4l2->vdev->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
> V4L2_CAP_STREAMING;
> if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
> - v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
> + v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
> if (dev->tuner_type != TUNER_ABSENT)
> - v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
> -
> + v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
>
> /* disable inapplicable ioctls */
> if (dev->is_webcam) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
> } else {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
> }
> if (dev->tuner_type == TUNER_ABSENT) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
> }
> if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
> }
>
> /* register v4l2 video video_device */
> - ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
> + ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
> video_nr[dev->devno]);
> if (ret) {
> dev_err(&dev->intf->dev,
> @@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> dev_info(&dev->intf->dev,
> "V4L2 video device registered as %s\n",
> - video_device_node_name(&v4l2->vdev));
> + video_device_node_name(v4l2->vdev));
>
> if (video_is_registered(&v4l2->vbi_dev))
> dev_info(&dev->intf->dev,
> @@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
>
> /* Save some power by putting tuner to sleep */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>
> /* initialize videobuf2 stuff */
> em28xx_vb2_setup(dev);
> @@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
> video_unregister_device(&v4l2->vbi_dev);
> }
> - if (video_is_registered(&v4l2->vdev)) {
> + if (video_is_registered(v4l2->vdev)) {
> dev_info(&dev->intf->dev,
> "V4L2 device %s deregistered\n",
> - video_device_node_name(&v4l2->vdev));
> - video_unregister_device(&v4l2->vdev);
> + video_device_node_name(v4l2->vdev));
> + video_unregister_device(v4l2->vdev);
> }
>
> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
> - v4l2_device_unregister(&v4l2->v4l2_dev);
> -err:
> + v4l2_device_unregister(v4l2->v4l2_dev);
> +
> +v4l2_device_register_error:
> + v4l2_device_put(v4l2->v4l2_dev);
> +v4l2_dev_error:
> dev->v4l2 = NULL;
> kref_put(&v4l2->ref, em28xx_free_v4l2);
> +v4l2_error:
> mutex_unlock(&dev->lock);
> return ret;
> }
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 6648e11f1271..dbcc297b5a0d 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -552,10 +552,10 @@ struct em28xx_v4l2 {
> struct kref ref;
> struct em28xx *dev;
>
> - struct v4l2_device v4l2_dev;
> + struct v4l2_device *v4l2_dev;
> struct v4l2_ctrl_handler ctrl_handler;
>
> - struct video_device vdev;
> + struct video_device *vdev;
> struct video_device vbi_dev;
> struct video_device radio_dev;
>
> @@ -601,7 +601,7 @@ struct em28xx_v4l2 {
> unsigned int field_count;
>
> #ifdef CONFIG_MEDIA_CONTROLLER
> - struct media_pad video_pad, vbi_pad;
> + struct media_pad *video_pad, vbi_pad;
> struct media_entity *decoder;
> #endif
> };
>
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: em28xx: Fix race condition between open and init function
@ 2021-04-08 17:37 ` Shuah Khan
0 siblings, 0 replies; 9+ messages in thread
From: Shuah Khan @ 2021-04-08 17:37 UTC (permalink / raw)
To: Igor Matheus Andrade Torrente, mchehab
Cc: linux-media, linux-kernel-mentees, linux-kernel,
syzbot+b2391895514ed9ef4a8e
On 4/8/21 6:10 AM, Igor Matheus Andrade Torrente wrote:
> Fixes a race condition - for lack of a more precise term - between
> em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
> media_pad and vdev structs from the em28xx_v4l2, and managing the
> lifetime of those objects more dynamicaly.
>
> The race happens when a thread[1] - containing the em28xx_v4l2_init()
> code - calls the v4l2_mc_create_media_graph(), and it return a error,
> if a thread[2] - running v4l2_open() - pass the verification point
> and reaches the em28xx_v4l2_open() before the thread[1] finishes
> the v4l2 subsystem deregistration, thread[1] will free all resources
> before the em28xx_v4l2_open() can process their things,
> because the em28xx_v4l2_init() has the dev->lock. And all this lead
> the thread[2] to cause a user-after-free.
>
Have you tried this patch with em28xx device? You will have to take into
account the dependencies between the subdevs using the v4l2_dev.
Also try rmmod invidual drivers - what happens if you were to rmmod a
subdev driver? With v4l2_dev is not embedded in v4l2, this could open
up memory leaks or user-after-frees.
> Reported-and-tested-by: syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com
> Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>
> ---
> drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
> drivers/media/usb/em28xx/em28xx-video.c | 188 ++++++++++++++---------
> drivers/media/usb/em28xx/em28xx.h | 6 +-
> 3 files changed, 123 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> index d1e66b503f4d..436c5a8cbbb6 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
> v4l2->sensor_xtal = 4300000;
> pdata.xtal = v4l2->sensor_xtal;
> if (NULL ==
> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
> &mt9v011_info, NULL))
> return -ENODEV;
> v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
> @@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
> v4l2->sensor_yres = 480;
>
> subdev =
> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
> &ov2640_info, NULL);
> if (!subdev)
> return -ENODEV;
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 6b84c3413e83..e1febb2bf06b 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
> */
> static void em28xx_wake_i2c(struct em28xx *dev)
> {
> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>
> v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
> v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
> @@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct em28xx *dev)
> struct em28xx_v4l2 *v4l2 = dev->v4l2;
> int ret, i;
>
> + v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
> + if (!v4l2->video_pad) {
> + dev_err(&dev->intf->dev,
> + "failed to allocate video pad memory!\n");
> + v4l2->vdev->entity.num_pads = 0;
> + return;
> + }
> +
> /* Initialize Video, VBI and Radio pads */
> - v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
> - ret = media_entity_pads_init(&v4l2->vdev.entity, 1, &v4l2->video_pad);
> + v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
> + ret = media_entity_pads_init(&v4l2->vdev->entity, 1, v4l2->video_pad);
> if (ret < 0)
> dev_err(&dev->intf->dev,
> "failed to initialize video media entity!\n");
> @@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
> f.type = V4L2_TUNER_RADIO;
> else
> f.type = V4L2_TUNER_ANALOG_TV;
> - v4l2_device_call_all(&v4l2->v4l2_dev,
> + v4l2_device_call_all(v4l2->v4l2_dev,
> 0, tuner, s_frequency, &f);
>
> /* Enable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
> }
>
> v4l2->streaming_users++;
> @@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct vb2_queue *vq)
>
> if (v4l2->streaming_users-- == 1) {
> /* Disable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>
> /* Last active user, so shutdown all the URBS */
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
> @@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue *vq)
>
> if (v4l2->streaming_users-- == 1) {
> /* Disable video stream at TV decoder */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>
> /* Last active user, so shutdown all the URBS */
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
> @@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)
>
> static void video_mux(struct em28xx *dev, int index)
> {
> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>
> dev->ctl_input = index;
> dev->ctl_ainput = INPUT(index)->amux;
> @@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file, void *priv, v4l2_std_id *norm)
> {
> struct em28xx *dev = video_drvdata(file);
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd, norm);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);
>
> return 0;
> }
> @@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void *priv, v4l2_std_id norm)
> &v4l2->hscale, &v4l2->vscale);
>
> em28xx_resolution_set(dev);
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>
> return 0;
> }
> @@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void *priv,
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> if (dev->is_webcam) {
> - rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
> + rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
> video, g_frame_interval, &ival);
> if (!rc)
> p->parm.capture.timeperframe = ival.interval;
> @@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void *priv,
> memset(&p->parm, 0, sizeof(p->parm));
> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> - rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
> + rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
> video, s_frame_interval, &ival);
> if (!rc)
> p->parm.capture.timeperframe = ival.interval;
> @@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file, void *priv,
> if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
> i->type = V4L2_INPUT_TYPE_TUNER;
>
> - i->std = dev->v4l2->vdev.tvnorms;
> + i->std = dev->v4l2->vdev->tvnorms;
> /* webcams do not have the STD API */
> if (dev->is_webcam)
> i->capabilities = 0;
> @@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file, void *priv,
>
> strscpy(t->name, "Tuner", sizeof(t->name));
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> return 0;
> }
>
> @@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file, void *priv,
> if (t->index != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> return 0;
> }
>
> @@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file, void *priv,
> if (f->tuner != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency, &new_freq);
> v4l2->frequency = new_freq.frequency;
>
> return 0;
> @@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file, void *priv,
> strscpy(chip->name, "ac97", sizeof(chip->name));
> else
> strscpy(chip->name,
> - dev->v4l2->v4l2_dev.name, sizeof(chip->name));
> + dev->v4l2->v4l2_dev->name, sizeof(chip->name));
> return 0;
> }
>
> @@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void *priv,
>
> strscpy(t->name, "Radio", sizeof(t->name));
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>
> return 0;
> }
> @@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void *priv,
> if (t->index != 0)
> return -EINVAL;
>
> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>
> return 0;
> }
> @@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
> if (mutex_lock_interruptible(&dev->lock))
> return -ERESTARTSYS;
>
> + if (!dev->v4l2) {
> + mutex_unlock(&dev->lock);
> + return -ENODEV;
> + }
> +
> ret = v4l2_fh_open(filp);
> if (ret) {
> dev_err(&dev->intf->dev,
> @@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
>
> if (vdev->vfl_type == VFL_TYPE_RADIO) {
> em28xx_videodbg("video_open: setting radio device\n");
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
> }
>
> kref_get(&dev->ref);
> @@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>
> mutex_lock(&dev->lock);
>
> - v4l2_device_disconnect(&v4l2->v4l2_dev);
> + v4l2_device_disconnect(v4l2->v4l2_dev);
>
> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>
> @@ -2238,14 +2251,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
> video_unregister_device(&v4l2->vbi_dev);
> }
> - if (video_is_registered(&v4l2->vdev)) {
> + if (video_is_registered(v4l2->vdev)) {
> dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
> - video_device_node_name(&v4l2->vdev));
> - video_unregister_device(&v4l2->vdev);
> + video_device_node_name(v4l2->vdev));
> + video_unregister_device(v4l2->vdev);
> }
>
> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
> - v4l2_device_unregister(&v4l2->v4l2_dev);
> + v4l2_device_unregister(v4l2->v4l2_dev);
> + v4l2_device_put(v4l2->v4l2_dev);
>
> kref_put(&v4l2->ref, em28xx_free_v4l2);
>
> @@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
> goto exit;
>
> /* Save some power by putting tuner to sleep */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>
> /* do this before setting alternate! */
> em28xx_set_mode(dev, EM28XX_SUSPEND);
> @@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
> return 0;
> }
>
> +void em28xx_vdev_release(struct video_device *vdev)
> +{
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + int i;
> +
> + for (i = 0; i < vdev->entity.num_pads; i++)
> + kfree(&vdev->entity.pads[i]);
> +#endif
> + kfree(vdev);
> +}
> +
> static const struct v4l2_file_operations em28xx_v4l_fops = {
> .owner = THIS_MODULE,
> .open = em28xx_v4l2_open,
> @@ -2387,7 +2412,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
> static const struct video_device em28xx_video_template = {
> .fops = &em28xx_v4l_fops,
> .ioctl_ops = &video_ioctl_ops,
> - .release = video_device_release_empty,
> + .release = em28xx_vdev_release,
> .tvnorms = V4L2_STD_ALL,
> };
>
> @@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
> const char *type_name)
> {
> *vfd = *template;
> - vfd->v4l2_dev = &dev->v4l2->v4l2_dev;
> + vfd->v4l2_dev = dev->v4l2->v4l2_dev;
> vfd->lock = &dev->lock;
> if (dev->is_webcam)
> vfd->tvnorms = 0;
> @@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
> static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
> {
> struct em28xx_v4l2 *v4l2 = dev->v4l2;
> - struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
> + struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
> struct tuner_setup tun_setup;
> struct v4l2_frequency f;
>
> @@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx *dev, unsigned short tuner_addr)
> v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
> }
>
> +void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> +{
> + kfree(v4l2_dev);
> +}
> +
> static int em28xx_v4l2_init(struct em28xx *dev)
> {
> u8 val;
> @@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
> if (!v4l2) {
> - mutex_unlock(&dev->lock);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto v4l2_error;
> }
> +
> kref_init(&v4l2->ref);
> v4l2->dev = dev;
> dev->v4l2 = v4l2;
>
> + v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
> + if (!v4l2->v4l2_dev) {
> + ret = -ENOMEM;
> + goto v4l2_dev_error;
> + }
> +
> + v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
> +
> #ifdef CONFIG_MEDIA_CONTROLLER
> - v4l2->v4l2_dev.mdev = dev->media_dev;
> + v4l2->v4l2_dev->mdev = dev->media_dev;
> #endif
> - ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
> + ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
> if (ret < 0) {
> dev_err(&dev->intf->dev,
> "Call to v4l2_device_register() failed!\n");
> - goto err;
> + goto v4l2_device_register_error;
> }
>
> hdl = &v4l2->ctrl_handler;
> v4l2_ctrl_handler_init(hdl, 8);
> - v4l2->v4l2_dev.ctrl_handler = hdl;
> + v4l2->v4l2_dev->ctrl_handler = hdl;
>
> if (dev->is_webcam)
> v4l2->progressive = true;
> @@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> /* request some modules */
>
> if (dev->has_msp34xx)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "msp3400", 0, msp3400_addrs);
>
> if (dev->board.decoder == EM28XX_SAA711X)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "saa7115_auto", 0, saa711x_addrs);
>
> if (dev->board.decoder == EM28XX_TVP5150)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tvp5150", 0, tvp5150_addrs);
>
> if (dev->board.adecoder == EM28XX_TVAUDIO)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tvaudio", dev->board.tvaudio_addr, NULL);
>
> @@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
>
> if (dev->board.radio.type)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", dev->board.radio_addr,
> NULL);
Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.
>
> if (has_demod)
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", 0,
> v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
Same here:
Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.
> @@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
> struct v4l2_subdev *sd;
>
> - sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", 0,
> v4l2_i2c_tuner_addrs(type));
> @@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> if (sd)
> tuner_addr = v4l2_i2c_subdev_addr(sd);
Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.
> } else {
> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
> &dev->i2c_adap[dev->def_i2c_bus],
> "tuner", tuner_addr, NULL);
Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
check error prior to this change to allocating v4l2_dev. Now this has
to be handled as a error leg.
> }
> @@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> /* set default norm */
> v4l2->norm = V4L2_STD_PAL;
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
> v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
>
> /* Analog specific initialization */
> @@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> goto unregister_dev;
>
> /* allocate and fill video video_device struct */
> - em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
> + v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
> + if (!v4l2->vdev) {
> + ret = -ENOMEM;
> + goto unregister_dev;
> + }
> +
> + em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
> mutex_init(&v4l2->vb_queue_lock);
> mutex_init(&v4l2->vb_vbi_queue_lock);
> - v4l2->vdev.queue = &v4l2->vb_vidq;
> - v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
> - v4l2->vdev.device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
> + v4l2->vdev->queue = &v4l2->vb_vidq;
> + v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
> + v4l2->vdev->device_caps = V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE |
> V4L2_CAP_STREAMING;
> if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
> - v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
> + v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
> if (dev->tuner_type != TUNER_ABSENT)
> - v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
> -
> + v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
>
> /* disable inapplicable ioctls */
> if (dev->is_webcam) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
> } else {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
> }
> if (dev->tuner_type == TUNER_ABSENT) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
> }
> if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
> }
>
> /* register v4l2 video video_device */
> - ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
> + ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
> video_nr[dev->devno]);
> if (ret) {
> dev_err(&dev->intf->dev,
> @@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>
> dev_info(&dev->intf->dev,
> "V4L2 video device registered as %s\n",
> - video_device_node_name(&v4l2->vdev));
> + video_device_node_name(v4l2->vdev));
>
> if (video_is_registered(&v4l2->vbi_dev))
> dev_info(&dev->intf->dev,
> @@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
>
> /* Save some power by putting tuner to sleep */
> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>
> /* initialize videobuf2 stuff */
> em28xx_vb2_setup(dev);
> @@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> video_device_node_name(&v4l2->vbi_dev));
> video_unregister_device(&v4l2->vbi_dev);
> }
> - if (video_is_registered(&v4l2->vdev)) {
> + if (video_is_registered(v4l2->vdev)) {
> dev_info(&dev->intf->dev,
> "V4L2 device %s deregistered\n",
> - video_device_node_name(&v4l2->vdev));
> - video_unregister_device(&v4l2->vdev);
> + video_device_node_name(v4l2->vdev));
> + video_unregister_device(v4l2->vdev);
> }
>
> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
> - v4l2_device_unregister(&v4l2->v4l2_dev);
> -err:
> + v4l2_device_unregister(v4l2->v4l2_dev);
> +
> +v4l2_device_register_error:
> + v4l2_device_put(v4l2->v4l2_dev);
> +v4l2_dev_error:
> dev->v4l2 = NULL;
> kref_put(&v4l2->ref, em28xx_free_v4l2);
> +v4l2_error:
> mutex_unlock(&dev->lock);
> return ret;
> }
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 6648e11f1271..dbcc297b5a0d 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -552,10 +552,10 @@ struct em28xx_v4l2 {
> struct kref ref;
> struct em28xx *dev;
>
> - struct v4l2_device v4l2_dev;
> + struct v4l2_device *v4l2_dev;
> struct v4l2_ctrl_handler ctrl_handler;
>
> - struct video_device vdev;
> + struct video_device *vdev;
> struct video_device vbi_dev;
> struct video_device radio_dev;
>
> @@ -601,7 +601,7 @@ struct em28xx_v4l2 {
> unsigned int field_count;
>
> #ifdef CONFIG_MEDIA_CONTROLLER
> - struct media_pad video_pad, vbi_pad;
> + struct media_pad *video_pad, vbi_pad;
> struct media_entity *decoder;
> #endif
> };
>
thanks,
-- Shuah
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: em28xx: Fix race condition between open and init function
2021-04-08 17:37 ` Shuah Khan
@ 2021-04-09 13:40 ` Igor Torrente
-1 siblings, 0 replies; 9+ messages in thread
From: Igor Torrente @ 2021-04-09 13:40 UTC (permalink / raw)
To: Shuah Khan, mchehab
Cc: syzbot+b2391895514ed9ef4a8e, linux-media, linux-kernel-mentees,
linux-kernel
On 4/8/21 2:37 PM, Shuah Khan wrote:
> On 4/8/21 6:10 AM, Igor Matheus Andrade Torrente wrote:
>> Fixes a race condition - for lack of a more precise term - between
>> em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
>> media_pad and vdev structs from the em28xx_v4l2, and managing the
>> lifetime of those objects more dynamicaly.
>>
>> The race happens when a thread[1] - containing the em28xx_v4l2_init()
>> code - calls the v4l2_mc_create_media_graph(), and it return a error,
>> if a thread[2] - running v4l2_open() - pass the verification point
>> and reaches the em28xx_v4l2_open() before the thread[1] finishes
>> the v4l2 subsystem deregistration, thread[1] will free all resources
>> before the em28xx_v4l2_open() can process their things,
>> because the em28xx_v4l2_init() has the dev->lock. And all this lead
>> the thread[2] to cause a user-after-free.
>>
>
> Have you tried this patch with em28xx device? You will have to take into
> account the dependencies between the subdevs using the v4l2_dev.
>
No, I didn't because I don't have the device.
I was using the syzbot .c that produces this use-after-free.
I will try to modify the .c to test a complete initialization.
Actually, I forgot to put RFC alongside [PATCH].
> Also try rmmod invidual drivers - what happens if you were to rmmod a
> subdev driver? With v4l2_dev is not embedded in v4l2, this could open
> up memory leaks or user-after-frees.
>
OK. Let me see what I can do.
>> Reported-and-tested-by:
>> syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com
>> Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>
>> ---
>> drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
>> drivers/media/usb/em28xx/em28xx-video.c | 188 ++++++++++++++---------
>> drivers/media/usb/em28xx/em28xx.h | 6 +-
>> 3 files changed, 123 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
>> b/drivers/media/usb/em28xx/em28xx-camera.c
>> index d1e66b503f4d..436c5a8cbbb6 100644
>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>> @@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
>> v4l2->sensor_xtal = 4300000;
>> pdata.xtal = v4l2->sensor_xtal;
>> if (NULL ==
>> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
>> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
>> &mt9v011_info, NULL))
>> return -ENODEV;
>> v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
>> @@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
>> v4l2->sensor_yres = 480;
>> subdev =
>> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
>> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
>> &ov2640_info, NULL);
>> if (!subdev)
>> return -ENODEV;
>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c
>> b/drivers/media/usb/em28xx/em28xx-video.c
>> index 6b84c3413e83..e1febb2bf06b 100644
>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>> @@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
>> */
>> static void em28xx_wake_i2c(struct em28xx *dev)
>> {
>> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
>> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>> v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
>> v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
>> @@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct
>> em28xx *dev)
>> struct em28xx_v4l2 *v4l2 = dev->v4l2;
>> int ret, i;
>> + v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
>> + if (!v4l2->video_pad) {
>> + dev_err(&dev->intf->dev,
>> + "failed to allocate video pad memory!\n");
>> + v4l2->vdev->entity.num_pads = 0;
>> + return;
>> + }
>> +
>> /* Initialize Video, VBI and Radio pads */
>> - v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
>> - ret = media_entity_pads_init(&v4l2->vdev.entity, 1,
>> &v4l2->video_pad);
>> + v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
>> + ret = media_entity_pads_init(&v4l2->vdev->entity, 1,
>> v4l2->video_pad);
>> if (ret < 0)
>> dev_err(&dev->intf->dev,
>> "failed to initialize video media entity!\n");
>> @@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct
>> vb2_queue *vq, unsigned int count)
>> f.type = V4L2_TUNER_RADIO;
>> else
>> f.type = V4L2_TUNER_ANALOG_TV;
>> - v4l2_device_call_all(&v4l2->v4l2_dev,
>> + v4l2_device_call_all(v4l2->v4l2_dev,
>> 0, tuner, s_frequency, &f);
>> /* Enable video stream at TV decoder */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
>> }
>> v4l2->streaming_users++;
>> @@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct
>> vb2_queue *vq)
>> if (v4l2->streaming_users-- == 1) {
>> /* Disable video stream at TV decoder */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>> /* Last active user, so shutdown all the URBS */
>> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>> @@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue
>> *vq)
>> if (v4l2->streaming_users-- == 1) {
>> /* Disable video stream at TV decoder */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>> /* Last active user, so shutdown all the URBS */
>> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>> @@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)
>> static void video_mux(struct em28xx *dev, int index)
>> {
>> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
>> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>> dev->ctl_input = index;
>> dev->ctl_ainput = INPUT(index)->amux;
>> @@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file,
>> void *priv, v4l2_std_id *norm)
>> {
>> struct em28xx *dev = video_drvdata(file);
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd,
>> norm);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);
>> return 0;
>> }
>> @@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void
>> *priv, v4l2_std_id norm)
>> &v4l2->hscale, &v4l2->vscale);
>> em28xx_resolution_set(dev);
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> return 0;
>> }
>> @@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void
>> *priv,
>> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
>> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> if (dev->is_webcam) {
>> - rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
>> + rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
>> video, g_frame_interval, &ival);
>> if (!rc)
>> p->parm.capture.timeperframe = ival.interval;
>> @@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void
>> *priv,
>> memset(&p->parm, 0, sizeof(p->parm));
>> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
>> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> - rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
>> + rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
>> video, s_frame_interval, &ival);
>> if (!rc)
>> p->parm.capture.timeperframe = ival.interval;
>> @@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file,
>> void *priv,
>> if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
>> i->type = V4L2_INPUT_TYPE_TUNER;
>> - i->std = dev->v4l2->vdev.tvnorms;
>> + i->std = dev->v4l2->vdev->tvnorms;
>> /* webcams do not have the STD API */
>> if (dev->is_webcam)
>> i->capabilities = 0;
>> @@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file,
>> void *priv,
>> strscpy(t->name, "Tuner", sizeof(t->name));
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> return 0;
>> }
>> @@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file,
>> void *priv,
>> if (t->index != 0)
>> return -EINVAL;
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> return 0;
>> }
>> @@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file,
>> void *priv,
>> if (f->tuner != 0)
>> return -EINVAL;
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency,
>> &new_freq);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency,
>> &new_freq);
>> v4l2->frequency = new_freq.frequency;
>> return 0;
>> @@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file,
>> void *priv,
>> strscpy(chip->name, "ac97", sizeof(chip->name));
>> else
>> strscpy(chip->name,
>> - dev->v4l2->v4l2_dev.name, sizeof(chip->name));
>> + dev->v4l2->v4l2_dev->name, sizeof(chip->name));
>> return 0;
>> }
>> @@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void
>> *priv,
>> strscpy(t->name, "Radio", sizeof(t->name));
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> return 0;
>> }
>> @@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void
>> *priv,
>> if (t->index != 0)
>> return -EINVAL;
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> return 0;
>> }
>> @@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
>> if (mutex_lock_interruptible(&dev->lock))
>> return -ERESTARTSYS;
>> + if (!dev->v4l2) {
>> + mutex_unlock(&dev->lock);
>> + return -ENODEV;
>> + }
>> +
>> ret = v4l2_fh_open(filp);
>> if (ret) {
>> dev_err(&dev->intf->dev,
>> @@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
>> if (vdev->vfl_type == VFL_TYPE_RADIO) {
>> em28xx_videodbg("video_open: setting radio device\n");
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
>> }
>> kref_get(&dev->ref);
>> @@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>> mutex_lock(&dev->lock);
>> - v4l2_device_disconnect(&v4l2->v4l2_dev);
>> + v4l2_device_disconnect(v4l2->v4l2_dev);
>> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>> @@ -2238,14 +2251,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>> video_device_node_name(&v4l2->vbi_dev));
>> video_unregister_device(&v4l2->vbi_dev);
>> }
>> - if (video_is_registered(&v4l2->vdev)) {
>> + if (video_is_registered(v4l2->vdev)) {
>> dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
>> - video_device_node_name(&v4l2->vdev));
>> - video_unregister_device(&v4l2->vdev);
>> + video_device_node_name(v4l2->vdev));
>> + video_unregister_device(v4l2->vdev);
>> }
>> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
>> - v4l2_device_unregister(&v4l2->v4l2_dev);
>> + v4l2_device_unregister(v4l2->v4l2_dev);
>> + v4l2_device_put(v4l2->v4l2_dev);
>> kref_put(&v4l2->ref, em28xx_free_v4l2);
>> @@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
>> goto exit;
>> /* Save some power by putting tuner to sleep */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>> /* do this before setting alternate! */
>> em28xx_set_mode(dev, EM28XX_SUSPEND);
>> @@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
>> return 0;
>> }
>> +void em28xx_vdev_release(struct video_device *vdev)
>> +{
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> + int i;
>> +
>> + for (i = 0; i < vdev->entity.num_pads; i++)
>> + kfree(&vdev->entity.pads[i]);
>> +#endif
>> + kfree(vdev);
>> +}
>> +
>> static const struct v4l2_file_operations em28xx_v4l_fops = {
>> .owner = THIS_MODULE,
>> .open = em28xx_v4l2_open,
>> @@ -2387,7 +2412,7 @@ static const struct v4l2_ioctl_ops
>> video_ioctl_ops = {
>> static const struct video_device em28xx_video_template = {
>> .fops = &em28xx_v4l_fops,
>> .ioctl_ops = &video_ioctl_ops,
>> - .release = video_device_release_empty,
>> + .release = em28xx_vdev_release,
>> .tvnorms = V4L2_STD_ALL,
>> };
>> @@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
>> const char *type_name)
>> {
>> *vfd = *template;
>> - vfd->v4l2_dev = &dev->v4l2->v4l2_dev;
>> + vfd->v4l2_dev = dev->v4l2->v4l2_dev;
>> vfd->lock = &dev->lock;
>> if (dev->is_webcam)
>> vfd->tvnorms = 0;
>> @@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
>> static void em28xx_tuner_setup(struct em28xx *dev, unsigned short
>> tuner_addr)
>> {
>> struct em28xx_v4l2 *v4l2 = dev->v4l2;
>> - struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
>> + struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
>> struct tuner_setup tun_setup;
>> struct v4l2_frequency f;
>> @@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx
>> *dev, unsigned short tuner_addr)
>> v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
>> }
>> +void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
>> +{
>> + kfree(v4l2_dev);
>> +}
>> +
>> static int em28xx_v4l2_init(struct em28xx *dev)
>> {
>> u8 val;
>> @@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
>> if (!v4l2) {
>> - mutex_unlock(&dev->lock);
>> - return -ENOMEM;
>> + ret = -ENOMEM;
>> + goto v4l2_error;
>> }
>> +
>> kref_init(&v4l2->ref);
>> v4l2->dev = dev;
>> dev->v4l2 = v4l2;
>> + v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
>> + if (!v4l2->v4l2_dev) {
>> + ret = -ENOMEM;
>> + goto v4l2_dev_error;
>> + }
>> +
>> + v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
>> +
>> #ifdef CONFIG_MEDIA_CONTROLLER
>> - v4l2->v4l2_dev.mdev = dev->media_dev;
>> + v4l2->v4l2_dev->mdev = dev->media_dev;
>> #endif
>> - ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
>> + ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
>> if (ret < 0) {
>> dev_err(&dev->intf->dev,
>> "Call to v4l2_device_register() failed!\n");
>> - goto err;
>> + goto v4l2_device_register_error;
>> }
>> hdl = &v4l2->ctrl_handler;
>> v4l2_ctrl_handler_init(hdl, 8);
>> - v4l2->v4l2_dev.ctrl_handler = hdl;
>> + v4l2->v4l2_dev->ctrl_handler = hdl;
>> if (dev->is_webcam)
>> v4l2->progressive = true;
>> @@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> /* request some modules */
>> if (dev->has_msp34xx)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "msp3400", 0, msp3400_addrs);
>> if (dev->board.decoder == EM28XX_SAA711X)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "saa7115_auto", 0, saa711x_addrs);
>> if (dev->board.decoder == EM28XX_TVP5150)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tvp5150", 0, tvp5150_addrs);
>> if (dev->board.adecoder == EM28XX_TVAUDIO)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tvaudio", dev->board.tvaudio_addr, NULL);
>> @@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
>> if (dev->board.radio.type)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tuner", dev->board.radio_addr,
>> NULL);
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
I will add to all bellow.
>
>> if (has_demod)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tuner", 0,
>> v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
>
> Same here:
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
>
>> @@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
>> struct v4l2_subdev *sd;
>> - sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tuner", 0,
>> v4l2_i2c_tuner_addrs(type));
>> @@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> if (sd)
>> tuner_addr = v4l2_i2c_subdev_addr(sd);
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
>
>> } else {
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tuner", tuner_addr, NULL);
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
>
>> }
>> @@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> /* set default norm */
>> v4l2->norm = V4L2_STD_PAL;
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
>> /* Analog specific initialization */
>> @@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> goto unregister_dev;
>> /* allocate and fill video video_device struct */
>> - em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
>> + v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
>> + if (!v4l2->vdev) {
>> + ret = -ENOMEM;
>> + goto unregister_dev;
>> + }
>> +
>> + em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
>> mutex_init(&v4l2->vb_queue_lock);
>> mutex_init(&v4l2->vb_vbi_queue_lock);
>> - v4l2->vdev.queue = &v4l2->vb_vidq;
>> - v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
>> - v4l2->vdev.device_caps = V4L2_CAP_READWRITE |
>> V4L2_CAP_VIDEO_CAPTURE |
>> + v4l2->vdev->queue = &v4l2->vb_vidq;
>> + v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
>> + v4l2->vdev->device_caps = V4L2_CAP_READWRITE |
>> V4L2_CAP_VIDEO_CAPTURE |
>> V4L2_CAP_STREAMING;
>> if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
>> - v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
>> + v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
>> if (dev->tuner_type != TUNER_ABSENT)
>> - v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
>> -
>> + v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
>> /* disable inapplicable ioctls */
>> if (dev->is_webcam) {
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
>> } else {
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
>> }
>> if (dev->tuner_type == TUNER_ABSENT) {
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
>> }
>> if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
>> }
>> /* register v4l2 video video_device */
>> - ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
>> + ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
>> video_nr[dev->devno]);
>> if (ret) {
>> dev_err(&dev->intf->dev,
>> @@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> dev_info(&dev->intf->dev,
>> "V4L2 video device registered as %s\n",
>> - video_device_node_name(&v4l2->vdev));
>> + video_device_node_name(v4l2->vdev));
>> if (video_is_registered(&v4l2->vbi_dev))
>> dev_info(&dev->intf->dev,
>> @@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> video_device_node_name(&v4l2->vbi_dev));
>> /* Save some power by putting tuner to sleep */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>> /* initialize videobuf2 stuff */
>> em28xx_vb2_setup(dev);
>> @@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> video_device_node_name(&v4l2->vbi_dev));
>> video_unregister_device(&v4l2->vbi_dev);
>> }
>> - if (video_is_registered(&v4l2->vdev)) {
>> + if (video_is_registered(v4l2->vdev)) {
>> dev_info(&dev->intf->dev,
>> "V4L2 device %s deregistered\n",
>> - video_device_node_name(&v4l2->vdev));
>> - video_unregister_device(&v4l2->vdev);
>> + video_device_node_name(v4l2->vdev));
>> + video_unregister_device(v4l2->vdev);
>> }
>> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
>> - v4l2_device_unregister(&v4l2->v4l2_dev);
>> -err:
>> + v4l2_device_unregister(v4l2->v4l2_dev);
>> +
>> +v4l2_device_register_error:
>> + v4l2_device_put(v4l2->v4l2_dev);
>> +v4l2_dev_error:
>> dev->v4l2 = NULL;
>> kref_put(&v4l2->ref, em28xx_free_v4l2);
>> +v4l2_error:
>> mutex_unlock(&dev->lock);
>> return ret;
>> }
>> diff --git a/drivers/media/usb/em28xx/em28xx.h
>> b/drivers/media/usb/em28xx/em28xx.h
>> index 6648e11f1271..dbcc297b5a0d 100644
>> --- a/drivers/media/usb/em28xx/em28xx.h
>> +++ b/drivers/media/usb/em28xx/em28xx.h
>> @@ -552,10 +552,10 @@ struct em28xx_v4l2 {
>> struct kref ref;
>> struct em28xx *dev;
>> - struct v4l2_device v4l2_dev;
>> + struct v4l2_device *v4l2_dev;
>> struct v4l2_ctrl_handler ctrl_handler;
>> - struct video_device vdev;
>> + struct video_device *vdev;
>> struct video_device vbi_dev;
>> struct video_device radio_dev;
>> @@ -601,7 +601,7 @@ struct em28xx_v4l2 {
>> unsigned int field_count;
>> #ifdef CONFIG_MEDIA_CONTROLLER
>> - struct media_pad video_pad, vbi_pad;
>> + struct media_pad *video_pad, vbi_pad;
>> struct media_entity *decoder;
>> #endif
>> };
>>
>
> thanks,
> -- Shuah
Thanks,
---
Igor Matheus Andrade Torrente
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: em28xx: Fix race condition between open and init function
@ 2021-04-09 13:40 ` Igor Torrente
0 siblings, 0 replies; 9+ messages in thread
From: Igor Torrente @ 2021-04-09 13:40 UTC (permalink / raw)
To: Shuah Khan, mchehab
Cc: linux-media, linux-kernel-mentees, linux-kernel,
syzbot+b2391895514ed9ef4a8e
On 4/8/21 2:37 PM, Shuah Khan wrote:
> On 4/8/21 6:10 AM, Igor Matheus Andrade Torrente wrote:
>> Fixes a race condition - for lack of a more precise term - between
>> em28xx_v4l2_open and em28xx_v4l2_init, by detaching the v4l2_dev,
>> media_pad and vdev structs from the em28xx_v4l2, and managing the
>> lifetime of those objects more dynamicaly.
>>
>> The race happens when a thread[1] - containing the em28xx_v4l2_init()
>> code - calls the v4l2_mc_create_media_graph(), and it return a error,
>> if a thread[2] - running v4l2_open() - pass the verification point
>> and reaches the em28xx_v4l2_open() before the thread[1] finishes
>> the v4l2 subsystem deregistration, thread[1] will free all resources
>> before the em28xx_v4l2_open() can process their things,
>> because the em28xx_v4l2_init() has the dev->lock. And all this lead
>> the thread[2] to cause a user-after-free.
>>
>
> Have you tried this patch with em28xx device? You will have to take into
> account the dependencies between the subdevs using the v4l2_dev.
>
No, I didn't because I don't have the device.
I was using the syzbot .c that produces this use-after-free.
I will try to modify the .c to test a complete initialization.
Actually, I forgot to put RFC alongside [PATCH].
> Also try rmmod invidual drivers - what happens if you were to rmmod a
> subdev driver? With v4l2_dev is not embedded in v4l2, this could open
> up memory leaks or user-after-frees.
>
OK. Let me see what I can do.
>> Reported-and-tested-by:
>> syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com
>> Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com>
>> ---
>> drivers/media/usb/em28xx/em28xx-camera.c | 4 +-
>> drivers/media/usb/em28xx/em28xx-video.c | 188 ++++++++++++++---------
>> drivers/media/usb/em28xx/em28xx.h | 6 +-
>> 3 files changed, 123 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
>> b/drivers/media/usb/em28xx/em28xx-camera.c
>> index d1e66b503f4d..436c5a8cbbb6 100644
>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>> @@ -340,7 +340,7 @@ int em28xx_init_camera(struct em28xx *dev)
>> v4l2->sensor_xtal = 4300000;
>> pdata.xtal = v4l2->sensor_xtal;
>> if (NULL ==
>> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
>> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
>> &mt9v011_info, NULL))
>> return -ENODEV;
>> v4l2->vinmode = EM28XX_VINMODE_RGB8_GRBG;
>> @@ -394,7 +394,7 @@ int em28xx_init_camera(struct em28xx *dev)
>> v4l2->sensor_yres = 480;
>> subdev =
>> - v4l2_i2c_new_subdev_board(&v4l2->v4l2_dev, adap,
>> + v4l2_i2c_new_subdev_board(v4l2->v4l2_dev, adap,
>> &ov2640_info, NULL);
>> if (!subdev)
>> return -ENODEV;
>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c
>> b/drivers/media/usb/em28xx/em28xx-video.c
>> index 6b84c3413e83..e1febb2bf06b 100644
>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>> @@ -184,7 +184,7 @@ static int em28xx_vbi_supported(struct em28xx *dev)
>> */
>> static void em28xx_wake_i2c(struct em28xx *dev)
>> {
>> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
>> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>> v4l2_device_call_all(v4l2_dev, 0, core, reset, 0);
>> v4l2_device_call_all(v4l2_dev, 0, video, s_routing,
>> @@ -974,9 +974,17 @@ static void em28xx_v4l2_create_entities(struct
>> em28xx *dev)
>> struct em28xx_v4l2 *v4l2 = dev->v4l2;
>> int ret, i;
>> + v4l2->video_pad = kzalloc(sizeof(*v4l2->video_pad), GFP_KERNEL);
>> + if (!v4l2->video_pad) {
>> + dev_err(&dev->intf->dev,
>> + "failed to allocate video pad memory!\n");
>> + v4l2->vdev->entity.num_pads = 0;
>> + return;
>> + }
>> +
>> /* Initialize Video, VBI and Radio pads */
>> - v4l2->video_pad.flags = MEDIA_PAD_FL_SINK;
>> - ret = media_entity_pads_init(&v4l2->vdev.entity, 1,
>> &v4l2->video_pad);
>> + v4l2->video_pad->flags = MEDIA_PAD_FL_SINK;
>> + ret = media_entity_pads_init(&v4l2->vdev->entity, 1,
>> v4l2->video_pad);
>> if (ret < 0)
>> dev_err(&dev->intf->dev,
>> "failed to initialize video media entity!\n");
>> @@ -1132,11 +1140,11 @@ int em28xx_start_analog_streaming(struct
>> vb2_queue *vq, unsigned int count)
>> f.type = V4L2_TUNER_RADIO;
>> else
>> f.type = V4L2_TUNER_ANALOG_TV;
>> - v4l2_device_call_all(&v4l2->v4l2_dev,
>> + v4l2_device_call_all(v4l2->v4l2_dev,
>> 0, tuner, s_frequency, &f);
>> /* Enable video stream at TV decoder */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 1);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 1);
>> }
>> v4l2->streaming_users++;
>> @@ -1157,7 +1165,7 @@ static void em28xx_stop_streaming(struct
>> vb2_queue *vq)
>> if (v4l2->streaming_users-- == 1) {
>> /* Disable video stream at TV decoder */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>> /* Last active user, so shutdown all the URBS */
>> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>> @@ -1192,7 +1200,7 @@ void em28xx_stop_vbi_streaming(struct vb2_queue
>> *vq)
>> if (v4l2->streaming_users-- == 1) {
>> /* Disable video stream at TV decoder */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_stream, 0);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_stream, 0);
>> /* Last active user, so shutdown all the URBS */
>> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>> @@ -1286,7 +1294,7 @@ static int em28xx_vb2_setup(struct em28xx *dev)
>> static void video_mux(struct em28xx *dev, int index)
>> {
>> - struct v4l2_device *v4l2_dev = &dev->v4l2->v4l2_dev;
>> + struct v4l2_device *v4l2_dev = dev->v4l2->v4l2_dev;
>> dev->ctl_input = index;
>> dev->ctl_ainput = INPUT(index)->amux;
>> @@ -1565,7 +1573,7 @@ static int vidioc_querystd(struct file *file,
>> void *priv, v4l2_std_id *norm)
>> {
>> struct em28xx *dev = video_drvdata(file);
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, video, querystd,
>> norm);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, video, querystd, norm);
>> return 0;
>> }
>> @@ -1596,7 +1604,7 @@ static int vidioc_s_std(struct file *file, void
>> *priv, v4l2_std_id norm)
>> &v4l2->hscale, &v4l2->vscale);
>> em28xx_resolution_set(dev);
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> return 0;
>> }
>> @@ -1616,7 +1624,7 @@ static int vidioc_g_parm(struct file *file, void
>> *priv,
>> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
>> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> if (dev->is_webcam) {
>> - rc = v4l2_device_call_until_err(&v4l2->v4l2_dev, 0,
>> + rc = v4l2_device_call_until_err(v4l2->v4l2_dev, 0,
>> video, g_frame_interval, &ival);
>> if (!rc)
>> p->parm.capture.timeperframe = ival.interval;
>> @@ -1648,7 +1656,7 @@ static int vidioc_s_parm(struct file *file, void
>> *priv,
>> memset(&p->parm, 0, sizeof(p->parm));
>> p->parm.capture.readbuffers = EM28XX_MIN_BUF;
>> p->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> - rc = v4l2_device_call_until_err(&dev->v4l2->v4l2_dev, 0,
>> + rc = v4l2_device_call_until_err(dev->v4l2->v4l2_dev, 0,
>> video, s_frame_interval, &ival);
>> if (!rc)
>> p->parm.capture.timeperframe = ival.interval;
>> @@ -1675,7 +1683,7 @@ static int vidioc_enum_input(struct file *file,
>> void *priv,
>> if (INPUT(n)->type == EM28XX_VMUX_TELEVISION)
>> i->type = V4L2_INPUT_TYPE_TUNER;
>> - i->std = dev->v4l2->vdev.tvnorms;
>> + i->std = dev->v4l2->vdev->tvnorms;
>> /* webcams do not have the STD API */
>> if (dev->is_webcam)
>> i->capabilities = 0;
>> @@ -1839,7 +1847,7 @@ static int vidioc_g_tuner(struct file *file,
>> void *priv,
>> strscpy(t->name, "Tuner", sizeof(t->name));
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> return 0;
>> }
>> @@ -1851,7 +1859,7 @@ static int vidioc_s_tuner(struct file *file,
>> void *priv,
>> if (t->index != 0)
>> return -EINVAL;
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> return 0;
>> }
>> @@ -1878,8 +1886,8 @@ static int vidioc_s_frequency(struct file *file,
>> void *priv,
>> if (f->tuner != 0)
>> return -EINVAL;
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_frequency, f);
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, g_frequency,
>> &new_freq);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_frequency, f);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, g_frequency,
>> &new_freq);
>> v4l2->frequency = new_freq.frequency;
>> return 0;
>> @@ -1897,7 +1905,7 @@ static int vidioc_g_chip_info(struct file *file,
>> void *priv,
>> strscpy(chip->name, "ac97", sizeof(chip->name));
>> else
>> strscpy(chip->name,
>> - dev->v4l2->v4l2_dev.name, sizeof(chip->name));
>> + dev->v4l2->v4l2_dev->name, sizeof(chip->name));
>> return 0;
>> }
>> @@ -2095,7 +2103,7 @@ static int radio_g_tuner(struct file *file, void
>> *priv,
>> strscpy(t->name, "Radio", sizeof(t->name));
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, g_tuner, t);
>> return 0;
>> }
>> @@ -2108,7 +2116,7 @@ static int radio_s_tuner(struct file *file, void
>> *priv,
>> if (t->index != 0)
>> return -EINVAL;
>> - v4l2_device_call_all(&dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> + v4l2_device_call_all(dev->v4l2->v4l2_dev, 0, tuner, s_tuner, t);
>> return 0;
>> }
>> @@ -2160,6 +2168,11 @@ static int em28xx_v4l2_open(struct file *filp)
>> if (mutex_lock_interruptible(&dev->lock))
>> return -ERESTARTSYS;
>> + if (!dev->v4l2) {
>> + mutex_unlock(&dev->lock);
>> + return -ENODEV;
>> + }
>> +
>> ret = v4l2_fh_open(filp);
>> if (ret) {
>> dev_err(&dev->intf->dev,
>> @@ -2184,7 +2197,7 @@ static int em28xx_v4l2_open(struct file *filp)
>> if (vdev->vfl_type == VFL_TYPE_RADIO) {
>> em28xx_videodbg("video_open: setting radio device\n");
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, s_radio);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, s_radio);
>> }
>> kref_get(&dev->ref);
>> @@ -2222,7 +2235,7 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>> mutex_lock(&dev->lock);
>> - v4l2_device_disconnect(&v4l2->v4l2_dev);
>> + v4l2_device_disconnect(v4l2->v4l2_dev);
>> em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
>> @@ -2238,14 +2251,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>> video_device_node_name(&v4l2->vbi_dev));
>> video_unregister_device(&v4l2->vbi_dev);
>> }
>> - if (video_is_registered(&v4l2->vdev)) {
>> + if (video_is_registered(v4l2->vdev)) {
>> dev_info(&dev->intf->dev, "V4L2 device %s deregistered\n",
>> - video_device_node_name(&v4l2->vdev));
>> - video_unregister_device(&v4l2->vdev);
>> + video_device_node_name(v4l2->vdev));
>> + video_unregister_device(v4l2->vdev);
>> }
>> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
>> - v4l2_device_unregister(&v4l2->v4l2_dev);
>> + v4l2_device_unregister(v4l2->v4l2_dev);
>> + v4l2_device_put(v4l2->v4l2_dev);
>> kref_put(&v4l2->ref, em28xx_free_v4l2);
>> @@ -2305,7 +2319,7 @@ static int em28xx_v4l2_close(struct file *filp)
>> goto exit;
>> /* Save some power by putting tuner to sleep */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>> /* do this before setting alternate! */
>> em28xx_set_mode(dev, EM28XX_SUSPEND);
>> @@ -2330,6 +2344,17 @@ static int em28xx_v4l2_close(struct file *filp)
>> return 0;
>> }
>> +void em28xx_vdev_release(struct video_device *vdev)
>> +{
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> + int i;
>> +
>> + for (i = 0; i < vdev->entity.num_pads; i++)
>> + kfree(&vdev->entity.pads[i]);
>> +#endif
>> + kfree(vdev);
>> +}
>> +
>> static const struct v4l2_file_operations em28xx_v4l_fops = {
>> .owner = THIS_MODULE,
>> .open = em28xx_v4l2_open,
>> @@ -2387,7 +2412,7 @@ static const struct v4l2_ioctl_ops
>> video_ioctl_ops = {
>> static const struct video_device em28xx_video_template = {
>> .fops = &em28xx_v4l_fops,
>> .ioctl_ops = &video_ioctl_ops,
>> - .release = video_device_release_empty,
>> + .release = em28xx_vdev_release,
>> .tvnorms = V4L2_STD_ALL,
>> };
>> @@ -2445,7 +2470,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
>> const char *type_name)
>> {
>> *vfd = *template;
>> - vfd->v4l2_dev = &dev->v4l2->v4l2_dev;
>> + vfd->v4l2_dev = dev->v4l2->v4l2_dev;
>> vfd->lock = &dev->lock;
>> if (dev->is_webcam)
>> vfd->tvnorms = 0;
>> @@ -2459,7 +2484,7 @@ static void em28xx_vdev_init(struct em28xx *dev,
>> static void em28xx_tuner_setup(struct em28xx *dev, unsigned short
>> tuner_addr)
>> {
>> struct em28xx_v4l2 *v4l2 = dev->v4l2;
>> - struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
>> + struct v4l2_device *v4l2_dev = v4l2->v4l2_dev;
>> struct tuner_setup tun_setup;
>> struct v4l2_frequency f;
>> @@ -2517,6 +2542,11 @@ static void em28xx_tuner_setup(struct em28xx
>> *dev, unsigned short tuner_addr)
>> v4l2_device_call_all(v4l2_dev, 0, tuner, s_frequency, &f);
>> }
>> +void em28xx_v4l2_dev_release(struct v4l2_device *v4l2_dev)
>> +{
>> + kfree(v4l2_dev);
>> +}
>> +
>> static int em28xx_v4l2_init(struct em28xx *dev)
>> {
>> u8 val;
>> @@ -2541,26 +2571,35 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> v4l2 = kzalloc(sizeof(*v4l2), GFP_KERNEL);
>> if (!v4l2) {
>> - mutex_unlock(&dev->lock);
>> - return -ENOMEM;
>> + ret = -ENOMEM;
>> + goto v4l2_error;
>> }
>> +
>> kref_init(&v4l2->ref);
>> v4l2->dev = dev;
>> dev->v4l2 = v4l2;
>> + v4l2->v4l2_dev = kzalloc(sizeof(*v4l2->v4l2_dev), GFP_KERNEL);
>> + if (!v4l2->v4l2_dev) {
>> + ret = -ENOMEM;
>> + goto v4l2_dev_error;
>> + }
>> +
>> + v4l2->v4l2_dev->release = em28xx_v4l2_dev_release;
>> +
>> #ifdef CONFIG_MEDIA_CONTROLLER
>> - v4l2->v4l2_dev.mdev = dev->media_dev;
>> + v4l2->v4l2_dev->mdev = dev->media_dev;
>> #endif
>> - ret = v4l2_device_register(&dev->intf->dev, &v4l2->v4l2_dev);
>> + ret = v4l2_device_register(&dev->intf->dev, v4l2->v4l2_dev);
>> if (ret < 0) {
>> dev_err(&dev->intf->dev,
>> "Call to v4l2_device_register() failed!\n");
>> - goto err;
>> + goto v4l2_device_register_error;
>> }
>> hdl = &v4l2->ctrl_handler;
>> v4l2_ctrl_handler_init(hdl, 8);
>> - v4l2->v4l2_dev.ctrl_handler = hdl;
>> + v4l2->v4l2_dev->ctrl_handler = hdl;
>> if (dev->is_webcam)
>> v4l2->progressive = true;
>> @@ -2575,22 +2614,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> /* request some modules */
>> if (dev->has_msp34xx)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "msp3400", 0, msp3400_addrs);
>> if (dev->board.decoder == EM28XX_SAA711X)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "saa7115_auto", 0, saa711x_addrs);
>> if (dev->board.decoder == EM28XX_TVP5150)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tvp5150", 0, tvp5150_addrs);
>> if (dev->board.adecoder == EM28XX_TVAUDIO)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tvaudio", dev->board.tvaudio_addr, NULL);
>> @@ -2601,13 +2640,13 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> int has_demod = (dev->board.tda9887_conf & TDA9887_PRESENT);
>> if (dev->board.radio.type)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tuner", dev->board.radio_addr,
>> NULL);
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
I will add to all bellow.
>
>> if (has_demod)
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tuner", 0,
>> v4l2_i2c_tuner_addrs(ADDRS_DEMOD));
>
> Same here:
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
>
>> @@ -2616,7 +2655,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> has_demod ? ADDRS_TV_WITH_DEMOD : ADDRS_TV;
>> struct v4l2_subdev *sd;
>> - sd = v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + sd = v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tuner", 0,
>> v4l2_i2c_tuner_addrs(type));
>> @@ -2624,7 +2663,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> if (sd)
>> tuner_addr = v4l2_i2c_subdev_addr(sd);
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
>
>> } else {
>> - v4l2_i2c_new_subdev(&v4l2->v4l2_dev,
>> + v4l2_i2c_new_subdev(v4l2->v4l2_dev,
>> &dev->i2c_adap[dev->def_i2c_bus],
>> "tuner", tuner_addr, NULL);
>
> Add null check for v4l2_i2c_new_subdev() and error handling. It was okay
> check error prior to this change to allocating v4l2_dev. Now this has
> to be handled as a error leg.
>
>> }
>> @@ -2686,7 +2725,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> /* set default norm */
>> v4l2->norm = V4L2_STD_PAL;
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, video, s_std, v4l2->norm);
>> v4l2->interlaced_fieldmode = EM28XX_INTERLACED_DEFAULT;
>> /* Analog specific initialization */
>> @@ -2756,40 +2795,45 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> goto unregister_dev;
>> /* allocate and fill video video_device struct */
>> - em28xx_vdev_init(dev, &v4l2->vdev, &em28xx_video_template, "video");
>> + v4l2->vdev = kzalloc(sizeof(*v4l2->vdev), GFP_KERNEL);
>> + if (!v4l2->vdev) {
>> + ret = -ENOMEM;
>> + goto unregister_dev;
>> + }
>> +
>> + em28xx_vdev_init(dev, v4l2->vdev, &em28xx_video_template, "video");
>> mutex_init(&v4l2->vb_queue_lock);
>> mutex_init(&v4l2->vb_vbi_queue_lock);
>> - v4l2->vdev.queue = &v4l2->vb_vidq;
>> - v4l2->vdev.queue->lock = &v4l2->vb_queue_lock;
>> - v4l2->vdev.device_caps = V4L2_CAP_READWRITE |
>> V4L2_CAP_VIDEO_CAPTURE |
>> + v4l2->vdev->queue = &v4l2->vb_vidq;
>> + v4l2->vdev->queue->lock = &v4l2->vb_queue_lock;
>> + v4l2->vdev->device_caps = V4L2_CAP_READWRITE |
>> V4L2_CAP_VIDEO_CAPTURE |
>> V4L2_CAP_STREAMING;
>> if (dev->int_audio_type != EM28XX_INT_AUDIO_NONE)
>> - v4l2->vdev.device_caps |= V4L2_CAP_AUDIO;
>> + v4l2->vdev->device_caps |= V4L2_CAP_AUDIO;
>> if (dev->tuner_type != TUNER_ABSENT)
>> - v4l2->vdev.device_caps |= V4L2_CAP_TUNER;
>> -
>> + v4l2->vdev->device_caps |= V4L2_CAP_TUNER;
>> /* disable inapplicable ioctls */
>> if (dev->is_webcam) {
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_QUERYSTD);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_STD);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_STD);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_QUERYSTD);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_STD);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_STD);
>> } else {
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_PARM);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_PARM);
>> }
>> if (dev->tuner_type == TUNER_ABSENT) {
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_TUNER);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_TUNER);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_FREQUENCY);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_FREQUENCY);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_TUNER);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_TUNER);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_FREQUENCY);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_FREQUENCY);
>> }
>> if (dev->int_audio_type == EM28XX_INT_AUDIO_NONE) {
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_G_AUDIO);
>> - v4l2_disable_ioctl(&v4l2->vdev, VIDIOC_S_AUDIO);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_G_AUDIO);
>> + v4l2_disable_ioctl(v4l2->vdev, VIDIOC_S_AUDIO);
>> }
>> /* register v4l2 video video_device */
>> - ret = video_register_device(&v4l2->vdev, VFL_TYPE_VIDEO,
>> + ret = video_register_device(v4l2->vdev, VFL_TYPE_VIDEO,
>> video_nr[dev->devno]);
>> if (ret) {
>> dev_err(&dev->intf->dev,
>> @@ -2863,7 +2907,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> dev_info(&dev->intf->dev,
>> "V4L2 video device registered as %s\n",
>> - video_device_node_name(&v4l2->vdev));
>> + video_device_node_name(v4l2->vdev));
>> if (video_is_registered(&v4l2->vbi_dev))
>> dev_info(&dev->intf->dev,
>> @@ -2871,7 +2915,7 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> video_device_node_name(&v4l2->vbi_dev));
>> /* Save some power by putting tuner to sleep */
>> - v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
>> + v4l2_device_call_all(v4l2->v4l2_dev, 0, tuner, standby);
>> /* initialize videobuf2 stuff */
>> em28xx_vb2_setup(dev);
>> @@ -2897,18 +2941,22 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>> video_device_node_name(&v4l2->vbi_dev));
>> video_unregister_device(&v4l2->vbi_dev);
>> }
>> - if (video_is_registered(&v4l2->vdev)) {
>> + if (video_is_registered(v4l2->vdev)) {
>> dev_info(&dev->intf->dev,
>> "V4L2 device %s deregistered\n",
>> - video_device_node_name(&v4l2->vdev));
>> - video_unregister_device(&v4l2->vdev);
>> + video_device_node_name(v4l2->vdev));
>> + video_unregister_device(v4l2->vdev);
>> }
>> v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
>> - v4l2_device_unregister(&v4l2->v4l2_dev);
>> -err:
>> + v4l2_device_unregister(v4l2->v4l2_dev);
>> +
>> +v4l2_device_register_error:
>> + v4l2_device_put(v4l2->v4l2_dev);
>> +v4l2_dev_error:
>> dev->v4l2 = NULL;
>> kref_put(&v4l2->ref, em28xx_free_v4l2);
>> +v4l2_error:
>> mutex_unlock(&dev->lock);
>> return ret;
>> }
>> diff --git a/drivers/media/usb/em28xx/em28xx.h
>> b/drivers/media/usb/em28xx/em28xx.h
>> index 6648e11f1271..dbcc297b5a0d 100644
>> --- a/drivers/media/usb/em28xx/em28xx.h
>> +++ b/drivers/media/usb/em28xx/em28xx.h
>> @@ -552,10 +552,10 @@ struct em28xx_v4l2 {
>> struct kref ref;
>> struct em28xx *dev;
>> - struct v4l2_device v4l2_dev;
>> + struct v4l2_device *v4l2_dev;
>> struct v4l2_ctrl_handler ctrl_handler;
>> - struct video_device vdev;
>> + struct video_device *vdev;
>> struct video_device vbi_dev;
>> struct video_device radio_dev;
>> @@ -601,7 +601,7 @@ struct em28xx_v4l2 {
>> unsigned int field_count;
>> #ifdef CONFIG_MEDIA_CONTROLLER
>> - struct media_pad video_pad, vbi_pad;
>> + struct media_pad *video_pad, vbi_pad;
>> struct media_entity *decoder;
>> #endif
>> };
>>
>
> thanks,
> -- Shuah
Thanks,
---
Igor Matheus Andrade Torrente
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-04-09 13:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08 12:10 [PATCH] media: em28xx: Fix race condition between open and init function Igor Matheus Andrade Torrente
2021-04-08 12:10 ` Igor Matheus Andrade Torrente
2021-04-08 15:38 ` kernel test robot
2021-04-08 15:38 ` kernel test robot
2021-04-08 15:38 ` kernel test robot
2021-04-08 17:37 ` Shuah Khan
2021-04-08 17:37 ` Shuah Khan
2021-04-09 13:40 ` Igor Torrente
2021-04-09 13:40 ` Igor Torrente
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.