Date: Thu, 26 Sep 2013 14:08:08 -0300
From: Mauro Carvalho Chehab <m.chehab@samsung.com>
To: Alexey Khoroshilov <khoroshilov@ispras.ru>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        ldv-project@linuxtesting.org
Subject: Re: [PATCH] [media] dvb_demux: fix deadlock in
 dmx_section_feed_release_filter()
Message-id: <20130926140808.729bad49@samsung.com>
In-reply-to: <1376772531-8963-1-git-send-email-khoroshilov@ispras.ru>
References: <1376772531-8963-1-git-send-email-khoroshilov@ispras.ru>
MIME-version: 1.0
Content-type: text/plain; charset=US-ASCII
Content-transfer-encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2254
Lines: 62

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/