[PATCH] [media] dvb_demux: fix deadlock in dmx_section_feed_release_filter()

[PATCH] [media] dvb_demux: fix deadlock in dmx_section_feed_release_filter()

[Alexey Khoroshilov]

dmx_section_feed_release_filter() locks dvbdmx->mutex and
if the feed is still filtering, it calls feed->stop_filtering(feed).
stop_filtering() is implemented by dmx_section_feed_stop_filtering()
that first of all try to lock the same mutex: dvbdmx->mutex.
That leads to a deadlock.

It does not happen often in practice because all callers of
release_filter() stop filtering by themselves.
So the problem can happen in case of race condition only.

The patch proposes to unlock dvbdmx->mutex before call feed->stop_filtering(feed)
and recheck feed->is_filtering after reacquiring mutex.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/media/dvb-core/dvb_demux.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dvb_demux.c b/drivers/media/dvb-core/dvb_demux.c
index 3485655..9d517af 100644
--- a/drivers/media/dvb-core/dvb_demux.c
+++ b/drivers/media/dvb-core/dvb_demux.c
@@ -1027,8 +1027,11 @@ static int dmx_section_feed_release_filter(struct dmx_section_feed *feed,
 		return -EINVAL;
 	}
 
-	if (feed->is_filtering)
+	while (feed->is_filtering) {
+		mutex_unlock(&dvbdmx->mutex);
 		feed->stop_filtering(feed);
+		mutex_lock(&dvbdmx->mutex);
+	}
 
 	spin_lock_irq(&dvbdmx->lock);
 	f = dvbdmxfeed->filter;
-- 
1.8.1.2

Re: [PATCH] [media] dvb_demux: fix deadlock in dmx_section_feed_release_filter()

[Mauro Carvalho Chehab]

Em Sat, 17 Aug 2013 23:48:51 +0300
Alexey Khoroshilov <khoroshilov@ispras.ru> escreveu:

> dmx_section_feed_release_filter() locks dvbdmx->mutex and
> if the feed is still filtering, it calls feed->stop_filtering(feed).
> stop_filtering() is implemented by dmx_section_feed_stop_filtering()
> that first of all try to lock the same mutex: dvbdmx->mutex.
> That leads to a deadlock.
> 
> It does not happen often in practice because all callers of
> release_filter() stop filtering by themselves.
> So the problem can happen in case of race condition only.
> 
> The patch proposes to unlock dvbdmx->mutex before call feed->stop_filtering(feed)
> and recheck feed->is_filtering after reacquiring mutex.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/media/dvb-core/dvb_demux.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_demux.c b/drivers/media/dvb-core/dvb_demux.c
> index 3485655..9d517af 100644
> --- a/drivers/media/dvb-core/dvb_demux.c
> +++ b/drivers/media/dvb-core/dvb_demux.c
> @@ -1027,8 +1027,11 @@ static int dmx_section_feed_release_filter(struct dmx_section_feed *feed,
>  		return -EINVAL;
>  	}
>  
> -	if (feed->is_filtering)
> +	while (feed->is_filtering) {

Changing from if to while here can cause a dead lock, as, if the device
got removed, dmx_section_feed_stop_filtering() won't touch
feed->is_filtering. So, the loop will happen forever.

Except for that the patch looks correct on my eyes.

> +		mutex_unlock(&dvbdmx->mutex);

Please add a small comment about why the mutex needs to be unlocked
there, as this looks ugly, and likely deserve some cleanups in the future
(as both the spinclock and the mutex are taken on both callback and at
the release filter function).

>  		feed->stop_filtering(feed);
> +		mutex_lock(&dvbdmx->mutex);
> +	}
>  
>  	spin_lock_irq(&dvbdmx->lock);
>  	f = dvbdmxfeed->filter;


Thanks!
Mauro

[PATCH v2] [media] dvb_demux: fix deadlock in dmx_section_feed_release_filter()

[Alexey Khoroshilov]

dmx_section_feed_release_filter() locks dvbdmx->mutex and
if the feed is still filtering, it calls feed->stop_filtering(feed).
stop_filtering() is implemented by dmx_section_feed_stop_filtering()
that first of all try to lock the same mutex: dvbdmx->mutex.
That leads to a deadlock.

It does not happen often in practice because all callers of
release_filter() stop filtering by themselves.
So the problem can happen in case of race condition only.

The patch releases dvbdmx->mutex before call to feed->stop_filtering(feed)
and reacquires the mutex after that.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/media/dvb-core/dvb_demux.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dvb_demux.c b/drivers/media/dvb-core/dvb_demux.c
index 3485655..6de3bd0 100644
--- a/drivers/media/dvb-core/dvb_demux.c
+++ b/drivers/media/dvb-core/dvb_demux.c
@@ -1027,8 +1027,13 @@ static int dmx_section_feed_release_filter(struct dmx_section_feed *feed,
 		return -EINVAL;
 	}
 
-	if (feed->is_filtering)
+	if (feed->is_filtering) {
+		/* release dvbdmx->mutex as far as 
+		   it is acquired by stop_filtering() itself */
+		mutex_unlock(&dvbdmx->mutex);
 		feed->stop_filtering(feed);
+		mutex_lock(&dvbdmx->mutex);
+	}
 
 	spin_lock_irq(&dvbdmx->lock);
 	f = dvbdmxfeed->filter;
-- 
1.8.1.2