[PATCH] uniform cdrom: fix mutex unlocking in /proc/sys/dev/cdrom/info file

[PATCH] uniform cdrom: fix mutex unlocking in /proc/sys/dev/cdrom/info file

[Davidlohr Bueso]

If the last if condition passes the cdrom_mutex mutex will not be unlocked. 
This patch fixes this and also removes the unnecessary doit label that was only invoked by the done label.

Thanks.


Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 drivers/cdrom/cdrom.c |   51 +++++++++++++++++++++++++-----------------------
 1 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index e3749d0..539b8d0 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -3429,70 +3429,73 @@ static int cdrom_sysctl_info(ctl_table *ctl, int write,
 	pos = sprintf(info, "CD-ROM information, " VERSION "\n");
 	
 	if (cdrom_print_info("\ndrive name:\t", 0, info, &pos, CTL_NAME))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\ndrive speed:\t", 0, info, &pos, CTL_SPEED))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\ndrive # of slots:", 0, info, &pos, CTL_SLOTS))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan close tray:\t",
 				CDC_CLOSE_TRAY, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan open tray:\t",
 				CDC_OPEN_TRAY, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan lock tray:\t",
 				CDC_LOCK, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan change speed:",
 				CDC_SELECT_SPEED, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan select disk:",
 				CDC_SELECT_DISC, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan read multisession:",
 				CDC_MULTI_SESSION, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan read MCN:\t",
 				CDC_MCN, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nReports media changed:",
 				CDC_MEDIA_CHANGED, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan play audio:\t",
 				CDC_PLAY_AUDIO, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan write CD-R:\t",
 				CDC_CD_R, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan write CD-RW:",
 				CDC_CD_RW, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan read DVD:\t",
 				CDC_DVD, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan write DVD-R:",
 				CDC_DVD_R, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan write DVD-RAM:",
 				CDC_DVD_RAM, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan read MRW:\t",
 				CDC_MRW, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan write MRW:\t",
 				CDC_MRW_W, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (cdrom_print_info("\nCan write RAM:\t",
 				CDC_RAM, info, &pos, CTL_CAPABILITY))
-		goto done;
+		goto small;
 	if (!scnprintf(info + pos, max_size - pos, "\n\n"))
-		goto done;
-doit:
+		goto small;
+	
+	goto ret;
+
+ret:
 	mutex_unlock(&cdrom_mutex);
 	return proc_dostring(ctl, write, buffer, lenp, ppos);
-done:
+small:
 	printk(KERN_INFO "cdrom: info buffer too small\n");
-	goto doit;
+	goto ret;
 }
 
 /* Unfortunately, per device settings are not implemented through
-- 
1.7.0.4

Re: [PATCH] uniform cdrom: fix mutex unlocking in /proc/sys/dev/cdrom/info file

[Andrew Morton]

On Wed, 09 Jun 2010 12:56:50 -0400
Davidlohr Bueso <dave.bueso@gmail.com> wrote:

> If the last if condition passes the cdrom_mutex mutex will not be unlocked. 
> This patch fixes this and also removes the unnecessary doit label that was only invoked by the done label.
> 

Confused.  There is no way in which cdrom_sysctl_info() can forget to
unlock cdrom_mutex.

Re: [PATCH] uniform cdrom: fix mutex unlocking in /proc/sys/dev/cdrom/info file

[Davidlohr Bueso]

On Mon, 2010-06-14 at 12:34 -0700, Andrew Morton wrote:
> On Wed, 09 Jun 2010 12:56:50 -0400
> Davidlohr Bueso <dave.bueso@gmail.com> wrote:
> 
> > If the last if condition passes the cdrom_mutex mutex will not be unlocked. 
> > This patch fixes this and also removes the unnecessary doit label that was only invoked by the done label.
> > 
> 
> Confused.  There is no way in which cdrom_sysctl_info() can forget to
> unlock cdrom_mutex.
> 

Ok, I had misread the code then, one of the reasons I don't like goto
labels, but that's a different story.

Regarding cdrom_sysctl_info(), I have noticed in several netbooks
(haven't
looked in other machines with no cdrom drive) that the
/proc/sys/dev/cdrom/info file shows no values for the listed entries,
ie:

CD-ROM information, Id: cdrom.c 3.20 2003/12/17

drive name:
drive speed:
drive # of slots:
Can close tray:
Can open tray:
...

Now, is this expected behavior? If not, should it be changed to
something
more descriptive? Perhaps it would affect user space applications that
read
the file?

Thanks,
Davidlohr

Re: [PATCH] uniform cdrom: fix mutex unlocking in /proc/sys/dev/cdrom/info file

[Andrew Morton]

On Tue, 15 Jun 2010 14:40:54 -0400
Davidlohr Bueso <dave.bueso@gmail.com> wrote:

> Regarding cdrom_sysctl_info(), I have noticed in several netbooks
> (haven't
> looked in other machines with no cdrom drive) that the
> /proc/sys/dev/cdrom/info file shows no values for the listed entries,
> ie:
> 
> CD-ROM information, Id: cdrom.c 3.20 2003/12/17
> 
> drive name:
> drive speed:
> drive # of slots:
> Can close tray:
> Can open tray:
> ...
> 
> Now, is this expected behavior? If not, should it be changed to
> something
> more descriptive? Perhaps it would affect user space applications that
> read
> the file?

It works for me.

Does your kernel build have CONFIG_SYSCTL=n?  If so, then all that code
is disabled.  That seems a bit bogus to me and perhaps we should enable
at least some of that code by CONFIG_PROCFS instead.