dvb-core: Fix potential mutex_unlock without mutex_lock in dvb_dvr_read
PATCH 1/1 dvb-core: Fix potential mutex_unlock without mutex_lock in dvb_dvr_read
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.
---
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(-)
drivers/media/dvb/dvb-core/dmxdev.c +1 -1
@@ -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 ; }
Signature · 4 lines
1.6.2.2 -- Simon Arlott
Quoted 27 lines · On Thu, 23 Apr 2009 18:32:13 +0100 Simon Arlott <[email protected]> wrote:
dvb_dvr_readmay 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_lockandmutex_unlockaround 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 <[email protected]>---
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-EWOULDBLOCKuntil there is data).drivers/media/dvb/dvb-core/dmxdev.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/media/dvb/dvb-core/dmxdev.c +0
-0
drivers/media/dvb/dvb-core/dmxdev.c static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count, · line 252
Quoted 7 lines
drivers/media/dvb/dvb-core/dmxdev.c +1
-1@@ -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?
drivers/media/dvb/dvb-core/dmxdev.c renamed from drivers/media/dvb/dvb-core/dmxdev.c~dvb-core-fix-potential-mutex_unlock-without-mutex_lock-in-dvb_dvr_read-fix +4 -10
@@ -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,
_
Quoted 32 lines · Andrew Morton
Quoted 29 lines · On Thu, 23 Apr 2009 18:32:13 +0100 Simon Arlott <[email protected]> wrote:
dvb_dvr_readmay 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_lockandmutex_unlockaround 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 <[email protected]>---
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-EWOULDBLOCKuntil there is data).drivers/media/dvb/dvb-core/dmxdev.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/media/dvb/dvb-core/dmxdev.c +0
-0@@ -247,7 +247,7 @@ static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count,int ret;
Quoted 8 lines
drivers/media/dvb/dvb-core/dmxdev.c static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count, · line 252
Quoted 5 lines
drivers/media/dvb/dvb-core/dmxdev.c +1
-1if (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.
Quoted 28 lines
drivers/media/dvb/dvb-core/dmxdev.c renamed from drivers/media/dvb/dvb-core/dmxdev.c~dvb-core-fix-potential-mutex_unlock-without-mutex_lock-in-dvb_dvr_read-fix +4
-10@@ -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,_
Signature · 1 line
Simon Arlott
Quoted 16 lines · On Thu, 30 Apr 2009 22:42:06 +0100 Simon Arlott <[email protected]> wrote:
Quoted 7 lines
Quoted 7 lines
drivers/media/dvb/dvb-core/dmxdev.c +0
-0@@ -247,7 +247,7 @@ static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count,int ret;Quoted 8 lines
drivers/media/dvb/dvb-core/dmxdev.c static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count, · line 252
Quoted 5 lines
drivers/media/dvb/dvb-core/dmxdev.c +1
-1if (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?
Quoted 3 lines
I'm assuming they should really be there - it's just not practical because the call to
dvb_dmxdev_buffer_readis likely to block waiting for data.
well.. such infomation is much better communicated via a nice comment, rather than mystery-dead-code?
Quoted 19 lines · Andrew Morton
Quoted 16 lines · On Thu, 30 Apr 2009 22:42:06 +0100 Simon Arlott <[email protected]> wrote:
Quoted 15 lines
drivers/media/dvb/dvb-core/dmxdev.c static ssize_t dvb_dvr_read(struct file *file, char __user *buf, size_t count, · line 252
Quoted 12 lines
drivers/media/dvb/dvb-core/dmxdev.c +1
-1@@ -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?
Quoted 6 lines
Quoted 3 lines
I'm assuming they should really be there - it's just not practical because the call to
dvb_dmxdev_buffer_readis 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
Signature · 3 lines
Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller