Re: [PATCH v5] media: em28xx: Fix race condition between open and init function

[Igor Torrente]

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