* [PATCH] dvb-core: Fix several locking related problems. [not found] ` <45EAE894.1010000@linuxtv.org> @ 2007-03-04 17:45 ` Simon Arlott 2007-03-10 1:49 ` Johannes Stezenbach 0 siblings, 1 reply; 3+ messages in thread From: Simon Arlott @ 2007-03-04 17:45 UTC (permalink / raw) To: Andrew Morton Cc: Mauro, Andreas Oberritter, Linux Kernel Mailing List, linux-dvb Fix several instances of dvb-core functions using mutex_lock_interruptible and returning -ERESTARTSYS where the calling function will either never retry or never check the return value. These cause a race condition with dvb_dmxdev_filter_free and dvb_dvr_release, both of which are filesystem release functions whose return value is ignored and will never be retried. When this happens it becomes impossible to open dvr0 again (-EBUSY) since it has not been released properly. Signed-off-by: Simon Arlott <simon@fire.lp0.eu> --- On 04/03/07 15:41, Andreas Oberritter wrote: > please send an updated patch together with > Signed-off-by line to Mauro <mchehab@infradead.org> and ask him to apply > it for inclusion into the -mm tree for further testing. Unless there are other -mm trees I've not heard about, presumably I should just do this myself. Doesn't linux-dvb have it's own development tree this would get better tested in? The dvb_dvr_release change has been working for me for 6 months and the dvb_dmxdev_filter_free (dvb_dmxdev_filter_free) change looks equivalent. See http://www.linuxtv.org/pipermail/linux-dvb/2007-February/016120.html for an example of the bug before and after fixing. All the other changes run ok for me but should have lockdep enabled when testing (if there's a possible deadlock somewhere, using _interruptible will hide it). drivers/media/dvb/dvb-core/dmxdev.c | 12 +++--------- drivers/media/dvb/dvb-core/dvb_demux.c | 21 +++++++-------------- drivers/media/dvb/dvb-core/dvbdev.c | 9 +++------ 3 files changed, 13 insertions(+), 29 deletions(-) diff --git a/drivers/media/dvb/dvb-core/dmxdev.c b/drivers/media/dvb/dvb-core/dmxdev.c index fc77de4..a5c0e1a 100644 --- a/drivers/media/dvb/dvb-core/dmxdev.c +++ b/drivers/media/dvb/dvb-core/dmxdev.c @@ -180,8 +180,7 @@ static int dvb_dvr_release(struct inode *inode, struct file *file) struct dvb_device *dvbdev = file->private_data; struct dmxdev *dmxdev = dvbdev->priv; - if (mutex_lock_interruptible(&dmxdev->mutex)) - return -ERESTARTSYS; + mutex_lock(&dmxdev->mutex); if ((file->f_flags & O_ACCMODE) == O_WRONLY) { dmxdev->demux->disconnect_frontend(dmxdev->demux); @@ -673,13 +672,8 @@ static int dvb_demux_open(struct inode *inode, struct file *file) static int dvb_dmxdev_filter_free(struct dmxdev *dmxdev, struct dmxdev_filter *dmxdevfilter) { - if (mutex_lock_interruptible(&dmxdev->mutex)) - return -ERESTARTSYS; - - if (mutex_lock_interruptible(&dmxdevfilter->mutex)) { - mutex_unlock(&dmxdev->mutex); - return -ERESTARTSYS; - } + mutex_lock(&dmxdev->mutex); + mutex_lock(&dmxdevfilter->mutex); dvb_dmxdev_filter_stop(dmxdevfilter); dvb_dmxdev_filter_reset(dmxdevfilter); diff --git a/drivers/media/dvb/dvb-core/dvb_demux.c b/drivers/media/dvb/dvb-core/dvb_demux.c index fcff5ea..6d8d1c3 100644 --- a/drivers/media/dvb/dvb-core/dvb_demux.c +++ b/drivers/media/dvb/dvb-core/dvb_demux.c @@ -673,8 +673,7 @@ static int dmx_ts_feed_stop_filtering(struct dmx_ts_feed *ts_feed) struct dvb_demux *demux = feed->demux; int ret; - if (mutex_lock_interruptible(&demux->mutex)) - return -ERESTARTSYS; + mutex_lock(&demux->mutex); if (feed->state < DMX_STATE_GO) { mutex_unlock(&demux->mutex); @@ -748,8 +747,7 @@ static int dvbdmx_release_ts_feed(struct dmx_demux *dmx, struct dvb_demux *demux = (struct dvb_demux *)dmx; struct dvb_demux_feed *feed = (struct dvb_demux_feed *)ts_feed; - if (mutex_lock_interruptible(&demux->mutex)) - return -ERESTARTSYS; + mutex_lock(&demux->mutex); if (feed->state == DMX_STATE_FREE) { mutex_unlock(&demux->mutex); @@ -916,8 +914,7 @@ static int dmx_section_feed_stop_filtering(struct dmx_section_feed *feed) struct dvb_demux *dvbdmx = dvbdmxfeed->demux; int ret; - if (mutex_lock_interruptible(&dvbdmx->mutex)) - return -ERESTARTSYS; + mutex_lock(&dvbdmx->mutex); if (!dvbdmx->stop_feed) { mutex_unlock(&dvbdmx->mutex); @@ -942,8 +939,7 @@ static int dmx_section_feed_release_filter(struct dmx_section_feed *feed, struct dvb_demux_feed *dvbdmxfeed = (struct dvb_demux_feed *)feed; struct dvb_demux *dvbdmx = dvbdmxfeed->demux; - if (mutex_lock_interruptible(&dvbdmx->mutex)) - return -ERESTARTSYS; + mutex_lock(&dvbdmx->mutex); if (dvbdmxfilter->feed != dvbdmxfeed) { mutex_unlock(&dvbdmx->mutex); @@ -1016,8 +1012,7 @@ static int dvbdmx_release_section_feed(struct dmx_demux *demux, struct dvb_demux_feed *dvbdmxfeed = (struct dvb_demux_feed *)feed; struct dvb_demux *dvbdmx = (struct dvb_demux *)demux; - if (mutex_lock_interruptible(&dvbdmx->mutex)) - return -ERESTARTSYS; + mutex_lock(&dvbdmx->mutex); if (dvbdmxfeed->state == DMX_STATE_FREE) { mutex_unlock(&dvbdmx->mutex); @@ -1126,8 +1121,7 @@ static int dvbdmx_connect_frontend(struct dmx_demux *demux, if (demux->frontend) return -EINVAL; - if (mutex_lock_interruptible(&dvbdemux->mutex)) - return -ERESTARTSYS; + mutex_lock(&dvbdemux->mutex); demux->frontend = frontend; mutex_unlock(&dvbdemux->mutex); @@ -1138,8 +1132,7 @@ static int dvbdmx_disconnect_frontend(struct dmx_demux *demux) { struct dvb_demux *dvbdemux = (struct dvb_demux *)demux; - if (mutex_lock_interruptible(&dvbdemux->mutex)) - return -ERESTARTSYS; + mutex_lock(&dvbdemux->mutex); demux->frontend = NULL; mutex_unlock(&dvbdemux->mutex); diff --git a/drivers/media/dvb/dvb-core/dvbdev.c b/drivers/media/dvb/dvb-core/dvbdev.c index b4553ae..1e0f01a 100644 --- a/drivers/media/dvb/dvb-core/dvbdev.c +++ b/drivers/media/dvb/dvb-core/dvbdev.c @@ -203,8 +203,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, int id; - if (mutex_lock_interruptible(&dvbdev_register_lock)) - return -ERESTARTSYS; + mutex_lock(&dvbdev_register_lock); if ((id = dvbdev_get_free_id (adap, type)) < 0){ mutex_unlock(&dvbdev_register_lock); @@ -294,8 +293,7 @@ int dvb_register_adapter(struct dvb_adapter *adap, const char *name, struct modu { int num; - if (mutex_lock_interruptible(&dvbdev_register_lock)) - return -ERESTARTSYS; + mutex_lock(&dvbdev_register_lock); if ((num = dvbdev_get_free_adapter_num ()) < 0) { mutex_unlock(&dvbdev_register_lock); @@ -323,8 +321,7 @@ EXPORT_SYMBOL(dvb_register_adapter); int dvb_unregister_adapter(struct dvb_adapter *adap) { - if (mutex_lock_interruptible(&dvbdev_register_lock)) - return -ERESTARTSYS; + mutex_lock(&dvbdev_register_lock); list_del (&adap->list_head); mutex_unlock(&dvbdev_register_lock); return 0; -- 1.5.0.1 -- Simon Arlott ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] dvb-core: Fix several locking related problems. 2007-03-04 17:45 ` [PATCH] dvb-core: Fix several locking related problems Simon Arlott @ 2007-03-10 1:49 ` Johannes Stezenbach 2007-03-10 9:23 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 3+ messages in thread From: Johannes Stezenbach @ 2007-03-10 1:49 UTC (permalink / raw) To: Simon Arlott Cc: Andrew Morton, Mauro, Andreas Oberritter, Linux Kernel Mailing List, linux-dvb On Sun, Mar 04, 2007 at 05:45:54PM +0000, Simon Arlott wrote: > Fix several instances of dvb-core functions using mutex_lock_interruptible > and returning -ERESTARTSYS where the calling function will either never > retry or never check the return value. > > These cause a race condition with dvb_dmxdev_filter_free and > dvb_dvr_release, both of which are filesystem release functions whose > return value is ignored and will never be retried. When this happens it > becomes impossible to open dvr0 again (-EBUSY) since it has not been > released properly. > > Signed-off-by: Simon Arlott <simon@fire.lp0.eu> Acked-By: Johannes Stezenbach <js@linuxtv.org> I can't test this but to me it looks good. Mauro, could you please pick it up and keep it in the linuxtv.org repository for a while for testing? Thanks, Johannes > --- > On 04/03/07 15:41, Andreas Oberritter wrote: > >please send an updated patch together with > >Signed-off-by line to Mauro <mchehab@infradead.org> and ask him to apply > >it for inclusion into the -mm tree for further testing. > > Unless there are other -mm trees I've not heard about, presumably I should > just do this myself. Doesn't linux-dvb have it's own development tree this > would get better tested in? > > The dvb_dvr_release change has been working for me for 6 months and the > dvb_dmxdev_filter_free (dvb_dmxdev_filter_free) change looks equivalent. > See http://www.linuxtv.org/pipermail/linux-dvb/2007-February/016120.html > for an example of the bug before and after fixing. > > All the other changes run ok for me but should have lockdep enabled when > testing (if there's a possible deadlock somewhere, using _interruptible > will hide it). > > drivers/media/dvb/dvb-core/dmxdev.c | 12 +++--------- > drivers/media/dvb/dvb-core/dvb_demux.c | 21 +++++++-------------- > drivers/media/dvb/dvb-core/dvbdev.c | 9 +++------ > 3 files changed, 13 insertions(+), 29 deletions(-) > > diff --git a/drivers/media/dvb/dvb-core/dmxdev.c > b/drivers/media/dvb/dvb-core/dmxdev.c > index fc77de4..a5c0e1a 100644 > --- a/drivers/media/dvb/dvb-core/dmxdev.c > +++ b/drivers/media/dvb/dvb-core/dmxdev.c > @@ -180,8 +180,7 @@ static int dvb_dvr_release(struct inode *inode, struct > file *file) > struct dvb_device *dvbdev = file->private_data; > struct dmxdev *dmxdev = dvbdev->priv; > > - if (mutex_lock_interruptible(&dmxdev->mutex)) > - return -ERESTARTSYS; > + mutex_lock(&dmxdev->mutex); > > if ((file->f_flags & O_ACCMODE) == O_WRONLY) { > dmxdev->demux->disconnect_frontend(dmxdev->demux); > @@ -673,13 +672,8 @@ static int dvb_demux_open(struct inode *inode, struct > file *file) > static int dvb_dmxdev_filter_free(struct dmxdev *dmxdev, > struct dmxdev_filter *dmxdevfilter) > { > - if (mutex_lock_interruptible(&dmxdev->mutex)) > - return -ERESTARTSYS; > - > - if (mutex_lock_interruptible(&dmxdevfilter->mutex)) { > - mutex_unlock(&dmxdev->mutex); > - return -ERESTARTSYS; > - } > + mutex_lock(&dmxdev->mutex); > + mutex_lock(&dmxdevfilter->mutex); > > dvb_dmxdev_filter_stop(dmxdevfilter); > dvb_dmxdev_filter_reset(dmxdevfilter); > diff --git a/drivers/media/dvb/dvb-core/dvb_demux.c > b/drivers/media/dvb/dvb-core/dvb_demux.c > index fcff5ea..6d8d1c3 100644 > --- a/drivers/media/dvb/dvb-core/dvb_demux.c > +++ b/drivers/media/dvb/dvb-core/dvb_demux.c > @@ -673,8 +673,7 @@ static int dmx_ts_feed_stop_filtering(struct > dmx_ts_feed *ts_feed) > struct dvb_demux *demux = feed->demux; > int ret; > > - if (mutex_lock_interruptible(&demux->mutex)) > - return -ERESTARTSYS; > + mutex_lock(&demux->mutex); > > if (feed->state < DMX_STATE_GO) { > mutex_unlock(&demux->mutex); > @@ -748,8 +747,7 @@ static int dvbdmx_release_ts_feed(struct dmx_demux *dmx, > struct dvb_demux *demux = (struct dvb_demux *)dmx; > struct dvb_demux_feed *feed = (struct dvb_demux_feed *)ts_feed; > > - if (mutex_lock_interruptible(&demux->mutex)) > - return -ERESTARTSYS; > + mutex_lock(&demux->mutex); > > if (feed->state == DMX_STATE_FREE) { > mutex_unlock(&demux->mutex); > @@ -916,8 +914,7 @@ static int dmx_section_feed_stop_filtering(struct > dmx_section_feed *feed) > struct dvb_demux *dvbdmx = dvbdmxfeed->demux; > int ret; > > - if (mutex_lock_interruptible(&dvbdmx->mutex)) > - return -ERESTARTSYS; > + mutex_lock(&dvbdmx->mutex); > > if (!dvbdmx->stop_feed) { > mutex_unlock(&dvbdmx->mutex); > @@ -942,8 +939,7 @@ static int dmx_section_feed_release_filter(struct > dmx_section_feed *feed, > struct dvb_demux_feed *dvbdmxfeed = (struct dvb_demux_feed *)feed; > struct dvb_demux *dvbdmx = dvbdmxfeed->demux; > > - if (mutex_lock_interruptible(&dvbdmx->mutex)) > - return -ERESTARTSYS; > + mutex_lock(&dvbdmx->mutex); > > if (dvbdmxfilter->feed != dvbdmxfeed) { > mutex_unlock(&dvbdmx->mutex); > @@ -1016,8 +1012,7 @@ static int dvbdmx_release_section_feed(struct > dmx_demux *demux, > struct dvb_demux_feed *dvbdmxfeed = (struct dvb_demux_feed *)feed; > struct dvb_demux *dvbdmx = (struct dvb_demux *)demux; > > - if (mutex_lock_interruptible(&dvbdmx->mutex)) > - return -ERESTARTSYS; > + mutex_lock(&dvbdmx->mutex); > > if (dvbdmxfeed->state == DMX_STATE_FREE) { > mutex_unlock(&dvbdmx->mutex); > @@ -1126,8 +1121,7 @@ static int dvbdmx_connect_frontend(struct dmx_demux > *demux, > if (demux->frontend) > return -EINVAL; > > - if (mutex_lock_interruptible(&dvbdemux->mutex)) > - return -ERESTARTSYS; > + mutex_lock(&dvbdemux->mutex); > > demux->frontend = frontend; > mutex_unlock(&dvbdemux->mutex); > @@ -1138,8 +1132,7 @@ static int dvbdmx_disconnect_frontend(struct > dmx_demux *demux) > { > struct dvb_demux *dvbdemux = (struct dvb_demux *)demux; > > - if (mutex_lock_interruptible(&dvbdemux->mutex)) > - return -ERESTARTSYS; > + mutex_lock(&dvbdemux->mutex); > > demux->frontend = NULL; > mutex_unlock(&dvbdemux->mutex); > diff --git a/drivers/media/dvb/dvb-core/dvbdev.c > b/drivers/media/dvb/dvb-core/dvbdev.c > index b4553ae..1e0f01a 100644 > --- a/drivers/media/dvb/dvb-core/dvbdev.c > +++ b/drivers/media/dvb/dvb-core/dvbdev.c > @@ -203,8 +203,7 @@ int dvb_register_device(struct dvb_adapter *adap, > struct dvb_device **pdvbdev, > > int id; > > - if (mutex_lock_interruptible(&dvbdev_register_lock)) > - return -ERESTARTSYS; > + mutex_lock(&dvbdev_register_lock); > > if ((id = dvbdev_get_free_id (adap, type)) < 0){ > mutex_unlock(&dvbdev_register_lock); > @@ -294,8 +293,7 @@ int dvb_register_adapter(struct dvb_adapter *adap, > const char *name, struct modu > { > int num; > > - if (mutex_lock_interruptible(&dvbdev_register_lock)) > - return -ERESTARTSYS; > + mutex_lock(&dvbdev_register_lock); > > if ((num = dvbdev_get_free_adapter_num ()) < 0) { > mutex_unlock(&dvbdev_register_lock); > @@ -323,8 +321,7 @@ EXPORT_SYMBOL(dvb_register_adapter); > > int dvb_unregister_adapter(struct dvb_adapter *adap) > { > - if (mutex_lock_interruptible(&dvbdev_register_lock)) > - return -ERESTARTSYS; > + mutex_lock(&dvbdev_register_lock); > list_del (&adap->list_head); > mutex_unlock(&dvbdev_register_lock); > return 0; > -- > 1.5.0.1 > > -- > Simon Arlott > - > 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/ > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] dvb-core: Fix several locking related problems. 2007-03-10 1:49 ` Johannes Stezenbach @ 2007-03-10 9:23 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 3+ messages in thread From: Mauro Carvalho Chehab @ 2007-03-10 9:23 UTC (permalink / raw) To: Johannes Stezenbach Cc: Simon Arlott, Andrew Morton, Andreas Oberritter, Linux Kernel Mailing List, linux-dvb Em Sáb, 2007-03-10 às 02:49 +0100, Johannes Stezenbach escreveu: > On Sun, Mar 04, 2007 at 05:45:54PM +0000, Simon Arlott wrote: > > Fix several instances of dvb-core functions using mutex_lock_interruptible > > and returning -ERESTARTSYS where the calling function will either never > > retry or never check the return value. > > > > These cause a race condition with dvb_dmxdev_filter_free and > > dvb_dvr_release, both of which are filesystem release functions whose > > return value is ignored and will never be retried. When this happens it > > becomes impossible to open dvr0 again (-EBUSY) since it has not been > > released properly. > > > > Signed-off-by: Simon Arlott <simon@fire.lp0.eu> > > Acked-By: Johannes Stezenbach <js@linuxtv.org> > > I can't test this but to me it looks good. > Mauro, could you please pick it up and keep it in the > linuxtv.org repository for a while for testing? Done. Thanks, Johannes. -- Cheers, Mauro ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-03-10 9:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <45DCB3A3.3060705@simon.arlott.org.uk>
[not found] ` <45E08886.50606@linuxtv.org>
[not found] ` <45E08BFD.3080203@simon.arlott.org.uk>
[not found] ` <45E9D923.7040707@simon.arlott.org.uk>
[not found] ` <45EAE894.1010000@linuxtv.org>
2007-03-04 17:45 ` [PATCH] dvb-core: Fix several locking related problems Simon Arlott
2007-03-10 1:49 ` Johannes Stezenbach
2007-03-10 9:23 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox