Hi Mauro,
Please see the comments below.
On Mon, Mar 28, 2016 at 03:11:03PM -0300, Mauro Carvalho Chehab wrote:
> Remove two nested mutex left-overs at find_entity and make sure
> that the code won't suffer race conditions if the device is
> being removed while ioctl is being handled nor the topology changes,
> by protecting all ioctls with a mutex at media_device_ioctl().
>
> As reported by Laurent, commit c38077d39c7e ("[media] media-device:
> get rid of the spinlock") introduced a deadlock in the
> MEDIA_IOC_ENUM_LINKS ioctl handler:
>
> [ 2760.127749] INFO: task media-ctl:954 blocked for more than 120 seconds.
> [ 2760.131867] Not tainted 4.5.0+ #357
> [ 2760.134622] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables
> this message.
> [ 2760.139310] media-ctl D ffffffc000086bcc 0 954 671
> 0x00000001
> [ 2760.143618] Call trace:
> [ 2760.145601] [<ffffffc000086bcc>] __switch_to+0x90/0xa4
> [ 2760.148941] [<ffffffc0004e6ef0>] __schedule+0x188/0x5b0
> [ 2760.152309] [<ffffffc0004e7354>] schedule+0x3c/0xa0
> [ 2760.155495] [<ffffffc0004e7768>] schedule_preempt_disabled+0x20/0x38
> [ 2760.159423] [<ffffffc0004e8d28>] __mutex_lock_slowpath+0xc4/0x148
> [ 2760.163217] [<ffffffc0004e8df0>] mutex_lock+0x44/0x5c
> [ 2760.166483] [<ffffffc0003e87d4>] find_entity+0x2c/0xac
> [ 2760.169773] [<ffffffc0003e8d34>] __media_device_enum_links+0x20/0x1dc
> [ 2760.173711] [<ffffffc0003e9718>] media_device_ioctl+0x214/0x33c
> [ 2760.177384] [<ffffffc0003e9eec>] media_ioctl+0x24/0x3c
> [ 2760.180671] [<ffffffc0001bee64>] do_vfs_ioctl+0xac/0x758
> [ 2760.184026] [<ffffffc0001bf594>] SyS_ioctl+0x84/0x98
> [ 2760.187196] [<ffffffc000085d30>] el0_svc_naked+0x24/0x28
>
> That's because find_entity() holds the graph_mutex, but both
> MEDIA_IOC_ENUM_LINKS and MEDIA_IOC_SETUP_LINK logic also take
> the mutex.
>
> Reported-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> ---
> drivers/media/media-device.c | 12 ++----------
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 7bfb2b24f644..6cfa890af7b4 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -90,8 +90,6 @@ static struct media_entity *find_entity(struct media_device
> *mdev, u32 id)
>
> id &= ~MEDIA_ENT_ID_FLAG_NEXT;
>
> - mutex_lock(&mdev->graph_mutex);
> -
> media_device_for_each_entity(entity, mdev) {
> if (((media_entity_id(entity) == id) && !next) ||
> ((media_entity_id(entity) > id) && next)) {
> @@ -100,8 +98,6 @@ static struct media_entity *find_entity(struct
> media_device *mdev, u32 id)
> }
> }
>
> - mutex_unlock(&mdev->graph_mutex);
find_entity() will return the entity inside the loop if it's found,
releasing the mutex.
> -
> return NULL;
> }
>
> @@ -431,6 +427,7 @@ static long media_device_ioctl(struct file *filp,
> unsigned int cmd,
> struct media_device *dev = devnode->media_dev;
> long ret;
>
> + mutex_lock(&dev->graph_mutex);
> switch (cmd) {
> case MEDIA_IOC_DEVICE_INFO:
> ret = media_device_get_info(dev,
> @@ -443,29 +440,24 @@ static long media_device_ioctl(struct file *filp,
> unsigned int cmd,
> break;
>
> case MEDIA_IOC_ENUM_LINKS:
> - mutex_lock(&dev->graph_mutex);
> ret = media_device_enum_links(dev,
> (struct media_links_enum __user *)arg);
> - mutex_unlock(&dev->graph_mutex);
> break;
>
> case MEDIA_IOC_SETUP_LINK:
> - mutex_lock(&dev->graph_mutex);
> ret = media_device_setup_link(dev,
> (struct media_link_desc __user *)arg);
> - mutex_unlock(&dev->graph_mutex);
> break;
>
> case MEDIA_IOC_G_TOPOLOGY:
> - mutex_lock(&dev->graph_mutex);
> ret = media_device_get_topology(dev,
> (struct media_v2_topology __user *)arg);
> - mutex_unlock(&dev->graph_mutex);
> break;
>
> default:
> ret = -ENOIOCTLCMD;
> }
> + mutex_unlock(&dev->graph_mutex);
>
> return ret;
> }
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
