Re: [PATCHv13 06/28] v4l2-dev: lock req_queue_mutex

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 08/05/18 12:45, Mauro Carvalho Chehab wrote:
> Em Tue, 8 May 2018 09:45:27 +0200
> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> 
>> On 05/07/2018 07:20 PM, Mauro Carvalho Chehab wrote:
>>> Em Thu,  3 May 2018 16:52:56 +0200
>>> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>>>   
>>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>>
>>>> We need to serialize streamon/off with queueing new requests.
>>>> These ioctls may trigger the cancellation of a streaming
>>>> operation, and that should not be mixed with queuing a new
>>>> request at the same time.
>>>>
>>>> Also TRY/S_EXT_CTRLS needs this lock to correctly serialize
>>>> with MEDIA_REQUEST_IOC_QUEUE.
>>>>
>>>> Finally close() needs this lock since that too can trigger the
>>>> cancellation of a streaming operation.
>>>>
>>>> We take the req_queue_mutex here before any other locks since
>>>> it is a very high-level lock.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>> ---
>>>>  drivers/media/v4l2-core/v4l2-dev.c | 37 +++++++++++++++++++++++++++++-
>>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>>> index 1d0b2208e8fb..b1c9efc0ecc4 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>>> @@ -353,13 +353,36 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>>>  
>>>>  	if (vdev->fops->unlocked_ioctl) {
>>>>  		struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
>>>> +		struct mutex *queue_lock = NULL;
>>>>  
>>>> -		if (lock && mutex_lock_interruptible(lock))
>>>> +		/*
>>>> +		 * We need to serialize streamon/off with queueing new requests.
>>>> +		 * These ioctls may trigger the cancellation of a streaming
>>>> +		 * operation, and that should not be mixed with queueing a new
>>>> +		 * request at the same time.
>>>> +		 *
>>>> +		 * Also TRY/S_EXT_CTRLS needs this lock to correctly serialize
>>>> +		 * with MEDIA_REQUEST_IOC_QUEUE.
>>>> +		 */
>>>> +		if (vdev->v4l2_dev->mdev &&
>>>> +		    (cmd == VIDIOC_STREAMON || cmd == VIDIOC_STREAMOFF ||
>>>> +		     cmd == VIDIOC_S_EXT_CTRLS || cmd == VIDIOC_TRY_EXT_CTRLS))
>>>> +			queue_lock = &vdev->v4l2_dev->mdev->req_queue_mutex;
>>>> +
>>>> +		if (queue_lock && mutex_lock_interruptible(queue_lock))
>>>> +			return -ERESTARTSYS;  
>>>
>>> Taking both locks seems risky. Here you're taking first v4l2 lock, returned
>>> by v4l2_ioctl_get_lock(vdev, cmd), and then you're taking the req_queue lock.  
>>
>> No,  v4l2_ioctl_get_lock() only returns a pointer to a mutex, it doesn't lock
>> anything. I think you got confused there. I'll reorganize the code a bit so
>> the call to  v4l2_ioctl_get_lock() happens after the queue_lock has been taken.
> 
> Yeah, I didn't actually look at the implementation of v4l2_ioctl_get_lock().
> 
> As we're using "_get" along this patch series to increment krefs (with is
> a sort of locking), the name here confused me. IMHO, we should rename it
> to v4l2_ioctl_return_lock() (or similar) on some future, in order to avoid
> confusion.

I think in the near future the v4l2_ioctl_get_lock() function can disappear.
My patch that pushes taking the ioctl serialization lock down into v4l2-ioctl.c
helps, and the gspca patch series removes the disable_locking bitarray.

Once that's all in I think the remaining code of v4l2_ioctl_get_lock can just
be moved to __video_do_ioctl() where it really belongs.

> 
>> I'll also rename queue_lock to req_queue_lock (it's a bit more descriptive).
> 
> Agreed.
> 
>>
>> So we first take the high-level media_device req_queue_mutex if needed, and
>> then the ioctl serialization lock. Doing it the other way around will indeed
>> promptly deadlock (as I very quickly discovered after my initial implementation!).
>>
>> So the order is:
>>
>> 	req_queue_mutex (serialize request state changes from/to IDLE)
>> 	ioctl lock (serialize ioctls)
>> 	request->lock (spinlock)
>>
>> The last is only held for short periods when updating the media_request struct.
>>
>>>
>>> It is possible to call parts of the code that only handles req_queue
>>> or v4l2 lock (for example, by mixing request API calls with non-requests
>>> one). Worse than that, there are parts of the code where the request API
>>> patches get both a mutex and a spin lock.
>>>
>>> I didn't look too closely (nor ran any test), but I'm almost sure that
>>> there are paths where it will end by leading into dead locks.  
>>
>> I've done extensive testing with this and actually been very careful about
>> the lock handling. It's also been tested with the cedrus driver.
> 
> I don't doubt it works using your apps, but real life can be messier:
> people could be issuing ioctls at different orders, programs can abort
> any time, closing file descriptors at random times, threads can be
> used to paralelize ioctls, etc.
> 
> That not discarding the possibility of someone coming with some
> ingenious code meant to cause machine hangups or to exposure some
> other security flaws.

Which is why the best place for this is in the core. I think v15 is more
readable in this respect.

Regards,

	Hans

> 
> 
> 
> Thanks,
> Mauro
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux