Hi Hillf,
On 5/28/21 4:52 AM, Hillf Danton wrote:
On 07/05/2021 21:34, 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
struct from the em28xx_v4l2, and managing the em28xx_v4l2 and v4l2_dev
life-time with the v4l2_dev->release() callback.
This is a bit more complicated than the rare race deserves and IMHO rcu can
help detect it.
The diff below 1) frees em28xx_v4l2 through rcu 2) checks race under rcu lock
on the open side.
Note it is only for idea and thoughts are welcome if it makes sense to you.
I didn't know what was the purpose of rcu, so I took some minutes to
study it.
If I understood correctly it solves the issue more easily and with way
fewer changes in the existing code.
Hans, what do you think?
+++ x/drivers/media/usb/em28xx/em28xx-video.c
@@ -2113,6 +2113,13 @@ static int radio_s_tuner(struct file *fi
return 0;
}
+static void em28xx_v4l2_rcufn(struct rcu_head *r)
+{
+ struct em28xx_v4l2 *v4l2 = container_of(r, struct em28xx_v4l2, rcu);
+
+ kfree(v4l2);
+}
+
/*
* em28xx_free_v4l2() - Free struct em28xx_v4l2
*
@@ -2125,7 +2132,13 @@ static void em28xx_free_v4l2(struct kref
struct em28xx_v4l2 *v4l2 = container_of(ref, struct em28xx_v4l2, ref);
v4l2->dev->v4l2 = NULL;
- kfree(v4l2);
+ call_rcu(&v4l2->rcu, em28xx_v4l2_rcufn);
+}
+
+static void em28xx_put_v4l2(struct em28xx_v4l2 *v4l2)
+{
+ if (v4l2)
+ kref_put(&v4l2->ref, em28xx_free_v4l2);
}
/*
@@ -2136,10 +2149,18 @@ static int em28xx_v4l2_open(struct file
{
struct video_device *vdev = video_devdata(filp);
struct em28xx *dev = video_drvdata(filp);
- struct em28xx_v4l2 *v4l2 = dev->v4l2;
+ struct em28xx_v4l2 *v4l2;
enum v4l2_buf_type fh_type = 0;
int ret;
+ rcu_read_lock();
+ v4l2 = dev->v4l2;
+ ret = v4l2 && kref_get_unless_zero(&v4l2->ref);
+ rcu_read_unlock();
+
+ if (!ret)
+ return -ENODEV;
+
switch (vdev->vfl_type) {
case VFL_TYPE_VIDEO:
fh_type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
@@ -2150,6 +2171,7 @@ static int em28xx_v4l2_open(struct file
case VFL_TYPE_RADIO:
break;
default:
+ em28xx_put_v4l2(v4l2);
return -EINVAL;
}
@@ -2157,8 +2179,10 @@ static int em28xx_v4l2_open(struct file
video_device_node_name(vdev), v4l2_type_names[fh_type],
v4l2->users);
- if (mutex_lock_interruptible(&dev->lock))
+ if (mutex_lock_interruptible(&dev->lock)) {
+ em28xx_put_v4l2(v4l2);
return -ERESTARTSYS;
+ }
ret = v4l2_fh_open(filp);
if (ret) {
@@ -2166,6 +2190,7 @@ static int em28xx_v4l2_open(struct file
"%s: v4l2_fh_open() returned error %d\n",
__func__, ret);
mutex_unlock(&dev->lock);
+ em28xx_put_v4l2(v4l2);
return ret;
}
@@ -2188,7 +2213,6 @@ static int em28xx_v4l2_open(struct file
}
kref_get(&dev->ref);
- kref_get(&v4l2->ref);
v4l2->users++;
mutex_unlock(&dev->lock);
Thanks,
---
Igor M. A. Torrente
