* [PATCH] dvb-core: Fix potential mutex_unlock without mutex_lock in dvb_dvr_read @ 2009-04-23 17:32 Simon Arlott 2009-04-30 20:18 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Simon Arlott @ 2009-04-23 17:32 UTC (permalink / raw) To: Linux Kernel Mailing List, Mauro Carvalho Chehab; +Cc: Linux DVB dvb_dvr_read may unlock the dmxdev mutex and return -ENODEV, except this function is a file op and will never be called with the mutex held. There's existing mutex_lock and mutex_unlock around the actual read but it's commented out. These should probably be uncommented but the read blocks and this could block another non-blocking reader on the mutex instead. This change comments out the extra mutex_unlock. Signed-off-by: Simon Arlott <simon@fire.lp0.eu> --- This has been on my TODO list for far too long... I did come up with a mutex_trylock/mutex_lock_interruptible version but claiming that it'll block when it may not doesn't make sense (and any blocking read would cause all non-blocking reads to continually return -EWOULDBLOCK until there is data). drivers/media/dvb/dvb-core/dmxdev.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/media/dvb/dvb-core/dmxdev.c b/drivers/media/dvb/dvb-core/dmxdev.c index c35fbb8..d6d098a 100644 --- a/drivers/media/dvb/dvb-core/dmxdev.c +++ b/drivers/media/dvb/dvb-core/dmxdev.c @@ -247,7 +247,7 @@ static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count, int ret; if (dmxdev->exit) { - mutex_unlock(&dmxdev->mutex); + //mutex_unlock(&dmxdev->mutex); return -ENODEV; } -- 1.6.2.2 -- Simon Arlott ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dvb-core: Fix potential mutex_unlock without mutex_lock in dvb_dvr_read 2009-04-23 17:32 [PATCH] dvb-core: Fix potential mutex_unlock without mutex_lock in dvb_dvr_read Simon Arlott @ 2009-04-30 20:18 ` Andrew Morton 2009-04-30 21:42 ` Simon Arlott 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2009-04-30 20:18 UTC (permalink / raw) To: Simon Arlott; +Cc: linux-kernel, mchehab, linux-dvb On Thu, 23 Apr 2009 18:32:13 +0100 Simon Arlott <simon@fire.lp0.eu> wrote: > dvb_dvr_read may unlock the dmxdev mutex and return -ENODEV, > except this function is a file op and will never be called > with the mutex held. > > There's existing mutex_lock and mutex_unlock around the actual > read but it's commented out. These should probably be uncommented > but the read blocks and this could block another non-blocking > reader on the mutex instead. > > This change comments out the extra mutex_unlock. > > Signed-off-by: Simon Arlott <simon@fire.lp0.eu> > --- > This has been on my TODO list for far too long... I did come > up with a mutex_trylock/mutex_lock_interruptible version but > claiming that it'll block when it may not doesn't make sense > (and any blocking read would cause all non-blocking reads to > continually return -EWOULDBLOCK until there is data). > > drivers/media/dvb/dvb-core/dmxdev.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/dvb/dvb-core/dmxdev.c b/drivers/media/dvb/dvb-core/dmxdev.c > index c35fbb8..d6d098a 100644 > --- a/drivers/media/dvb/dvb-core/dmxdev.c > +++ b/drivers/media/dvb/dvb-core/dmxdev.c > @@ -247,7 +247,7 @@ static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count, > int ret; > > if (dmxdev->exit) { > - mutex_unlock(&dmxdev->mutex); > + //mutex_unlock(&dmxdev->mutex); > return -ENODEV; > } Is there any value in retaining all the commented-out lock operations, or can we zap 'em? --- a/drivers/media/dvb/dvb-core/dmxdev.c~dvb-core-fix-potential-mutex_unlock-without-mutex_lock-in-dvb_dvr_read-fix +++ a/drivers/media/dvb/dvb-core/dmxdev.c @@ -244,19 +244,13 @@ static ssize_t dvb_dvr_read(struct file { struct dvb_device *dvbdev = file->private_data; struct dmxdev *dmxdev = dvbdev->priv; - int ret; - if (dmxdev->exit) { - //mutex_unlock(&dmxdev->mutex); + if (dmxdev->exit) return -ENODEV; - } - //mutex_lock(&dmxdev->mutex); - ret = dvb_dmxdev_buffer_read(&dmxdev->dvr_buffer, - file->f_flags & O_NONBLOCK, - buf, count, ppos); - //mutex_unlock(&dmxdev->mutex); - return ret; + return dvb_dmxdev_buffer_read(&dmxdev->dvr_buffer, + file->f_flags & O_NONBLOCK, + buf, count, ppos); } static int dvb_dvr_set_buffer_size(struct dmxdev *dmxdev, _ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dvb-core: Fix potential mutex_unlock without mutex_lock in dvb_dvr_read 2009-04-30 20:18 ` Andrew Morton @ 2009-04-30 21:42 ` Simon Arlott 2009-04-30 21:48 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Simon Arlott @ 2009-04-30 21:42 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, mchehab, linux-media On 30/04/09 21:18, Andrew Morton wrote: > On Thu, 23 Apr 2009 18:32:13 +0100 > Simon Arlott <simon@fire.lp0.eu> wrote: > >> dvb_dvr_read may unlock the dmxdev mutex and return -ENODEV, >> except this function is a file op and will never be called >> with the mutex held. >> >> There's existing mutex_lock and mutex_unlock around the actual >> read but it's commented out. These should probably be uncommented >> but the read blocks and this could block another non-blocking >> reader on the mutex instead. >> >> This change comments out the extra mutex_unlock. >> >> Signed-off-by: Simon Arlott <simon@fire.lp0.eu> >> --- >> This has been on my TODO list for far too long... I did come >> up with a mutex_trylock/mutex_lock_interruptible version but >> claiming that it'll block when it may not doesn't make sense >> (and any blocking read would cause all non-blocking reads to >> continually return -EWOULDBLOCK until there is data). >> >> drivers/media/dvb/dvb-core/dmxdev.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/media/dvb/dvb-core/dmxdev.c b/drivers/media/dvb/dvb-core/dmxdev.c >> index c35fbb8..d6d098a 100644 >> --- a/drivers/media/dvb/dvb-core/dmxdev.c >> +++ b/drivers/media/dvb/dvb-core/dmxdev.c >> @@ -247,7 +247,7 @@ static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count, >> int ret; >> >> if (dmxdev->exit) { >> - mutex_unlock(&dmxdev->mutex); >> + //mutex_unlock(&dmxdev->mutex); >> return -ENODEV; >> } > > Is there any value in retaining all the commented-out lock operations, > or can we zap 'em? I'm assuming they should really be there - it's just not practical because the call to dvb_dmxdev_buffer_read is likely to block waiting for data. > --- a/drivers/media/dvb/dvb-core/dmxdev.c~dvb-core-fix-potential-mutex_unlock-without-mutex_lock-in-dvb_dvr_read-fix > +++ a/drivers/media/dvb/dvb-core/dmxdev.c > @@ -244,19 +244,13 @@ static ssize_t dvb_dvr_read(struct file > { > struct dvb_device *dvbdev = file->private_data; > struct dmxdev *dmxdev = dvbdev->priv; > - int ret; > > - if (dmxdev->exit) { > - //mutex_unlock(&dmxdev->mutex); > + if (dmxdev->exit) > return -ENODEV; > - } > > - //mutex_lock(&dmxdev->mutex); > - ret = dvb_dmxdev_buffer_read(&dmxdev->dvr_buffer, > - file->f_flags & O_NONBLOCK, > - buf, count, ppos); > - //mutex_unlock(&dmxdev->mutex); > - return ret; > + return dvb_dmxdev_buffer_read(&dmxdev->dvr_buffer, > + file->f_flags & O_NONBLOCK, > + buf, count, ppos); > } > > static int dvb_dvr_set_buffer_size(struct dmxdev *dmxdev, > _ > -- Simon Arlott ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dvb-core: Fix potential mutex_unlock without mutex_lock in dvb_dvr_read 2009-04-30 21:42 ` Simon Arlott @ 2009-04-30 21:48 ` Andrew Morton 2009-04-30 21:54 ` Devin Heitmueller 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2009-04-30 21:48 UTC (permalink / raw) To: Simon Arlott; +Cc: linux-kernel, mchehab, linux-media On Thu, 30 Apr 2009 22:42:06 +0100 Simon Arlott <simon@fire.lp0.eu> wrote: > >> diff --git a/drivers/media/dvb/dvb-core/dmxdev.c b/drivers/media/dvb/dvb-core/dmxdev.c > >> index c35fbb8..d6d098a 100644 > >> --- a/drivers/media/dvb/dvb-core/dmxdev.c > >> +++ b/drivers/media/dvb/dvb-core/dmxdev.c > >> @@ -247,7 +247,7 @@ static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count, > >> int ret; > >> > >> if (dmxdev->exit) { > >> - mutex_unlock(&dmxdev->mutex); > >> + //mutex_unlock(&dmxdev->mutex); > >> return -ENODEV; > >> } > > > > Is there any value in retaining all the commented-out lock operations, > > or can we zap 'em? > > I'm assuming they should really be there - it's just not practical > because the call to dvb_dmxdev_buffer_read is likely to block waiting > for data. well.. such infomation is much better communicated via a nice comment, rather than mystery-dead-code? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dvb-core: Fix potential mutex_unlock without mutex_lock in dvb_dvr_read 2009-04-30 21:48 ` Andrew Morton @ 2009-04-30 21:54 ` Devin Heitmueller 0 siblings, 0 replies; 5+ messages in thread From: Devin Heitmueller @ 2009-04-30 21:54 UTC (permalink / raw) To: Andrew Morton; +Cc: Simon Arlott, linux-kernel, mchehab, linux-media On Thu, Apr 30, 2009 at 5:48 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Thu, 30 Apr 2009 22:42:06 +0100 > Simon Arlott <simon@fire.lp0.eu> wrote: > >> >> diff --git a/drivers/media/dvb/dvb-core/dmxdev.c b/drivers/media/dvb/dvb-core/dmxdev.c >> >> index c35fbb8..d6d098a 100644 >> >> --- a/drivers/media/dvb/dvb-core/dmxdev.c >> >> +++ b/drivers/media/dvb/dvb-core/dmxdev.c >> >> @@ -247,7 +247,7 @@ static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count, >> >> int ret; >> >> >> >> if (dmxdev->exit) { >> >> - mutex_unlock(&dmxdev->mutex); >> >> + //mutex_unlock(&dmxdev->mutex); >> >> return -ENODEV; >> >> } >> > >> > Is there any value in retaining all the commented-out lock operations, >> > or can we zap 'em? >> >> I'm assuming they should really be there - it's just not practical >> because the call to dvb_dmxdev_buffer_read is likely to block waiting >> for data. > > well.. such infomation is much better communicated via a nice comment, > rather than mystery-dead-code? I'm doing some review of the locking in dvb core as a result of a race condition I found earlier in the week. I'll take a look at this too when I get a few minutes. Devin -- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-04-30 21:54 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-04-23 17:32 [PATCH] dvb-core: Fix potential mutex_unlock without mutex_lock in dvb_dvr_read Simon Arlott 2009-04-30 20:18 ` Andrew Morton 2009-04-30 21:42 ` Simon Arlott 2009-04-30 21:48 ` Andrew Morton 2009-04-30 21:54 ` Devin Heitmueller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome