Hi,
Fix the cdrom_sysctl_info possible buffer overwrite bug. Somd codingstyle fixes are included as well.
diff based on 2.6.22-rc4
Signed-off-by: Dave Young <[email protected]>
---
drivers/cdrom/cdrom.c | 186 +-
1 files changed, 102 insertions(+), 84 deletions(-)
diff -upr linux/drivers/cdrom/cdrom.c linux.new/drivers/cdrom/cdrom.c
--- linux/drivers/cdrom/cdrom.c 2007-06-14 14:05:04.000000000 +0000
+++ linux.new/drivers/cdrom/cdrom.c 2007-06-14 14:11:58.000000000 +0000
@@ -3290,102 +3290,120 @@ static struct cdrom_sysctl_settings {
} cdrom_sysctl_settings;
static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
- void __user *buffer, size_t *lenp, loff_t *ppos)
+ void __user *buffer, size_t *lenp, loff_t *ppos)
{
- int pos;
+ int pos;
struct cdrom_device_info *cdi;
char *info = cdrom_sysctl_settings.info;
+ int size = sizeof(cdrom_sysctl_settings.info);
if (!*lenp || (*ppos && !write)) {
*lenp = 0;
return 0;
}
- pos = sprintf(info, "CD-ROM information, " VERSION "\n");
+ pos = scnprintf(info, size, "CD-ROM information, " VERSION "\n");
- pos += sprintf(info+pos, "\ndrive name:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%s", cdi->name);
-
- pos += sprintf(info+pos, "\ndrive speed:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", cdi->speed);
-
- pos += sprintf(info+pos, "\ndrive # of slots:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", cdi->capacity);
-
- pos += sprintf(info+pos, "\nCan close tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
-
- pos += sprintf(info+pos, "\nCan open tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
-
- pos += sprintf(info+pos, "\nCan lock tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
-
- pos += sprintf(info+pos, "\nCan change speed:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
-
- pos += sprintf(info+pos, "\nCan select disk:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
-
- pos += sprintf(info+pos, "\nCan read multisession:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
-
- pos += sprintf(info+pos, "\nCan read MCN:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
-
- pos += sprintf(info+pos, "\nReports media changed:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
-
- pos += sprintf(info+pos, "\nCan play audio:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
-
- pos += sprintf(info+pos, "\nCan write CD-R:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
-
- pos += sprintf(info+pos, "\nCan write CD-RW:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
-
- pos += sprintf(info+pos, "\nCan read DVD:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
-
- pos += sprintf(info+pos, "\nCan write DVD-R:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
-
- pos += sprintf(info+pos, "\nCan write DVD-RAM:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
-
- pos += sprintf(info+pos, "\nCan read MRW:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
-
- pos += sprintf(info+pos, "\nCan write MRW:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
-
- pos += sprintf(info+pos, "\nCan write RAM:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
+ pos += scnprintf(info + pos, size - pos, "\ndrive name:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%s", cdi->name);
+
+ pos += scnprintf(info + pos, size - pos ,"\ndrive speed:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d", cdi->speed);
+
+ pos += scnprintf(info + pos, size - pos, "\ndrive # of slots:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d", cdi->capacity);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan close tray:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_CLOSE_TRAY) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan open tray:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_OPEN_TRAY) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan lock tray:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_LOCK) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan change speed:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_SELECT_SPEED) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan select disk:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_SELECT_DISC) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan read multisession:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MULTI_SESSION) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan read MCN:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MCN) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nReports media changed:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan play audio:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_PLAY_AUDIO) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write CD-R:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_CD_R) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write CD-RW:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_CD_RW) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan read DVD:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_DVD) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write DVD-R:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_DVD_R) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write DVD-RAM:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_DVD_RAM) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan read MRW:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MRW) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write MRW:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MRW_W) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write RAM:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_RAM) != 0);
- strcpy(info+pos,"\n\n");
+ scnprintf(info + pos, size - pos, "\n\n");
- return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+ return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
}
/* Unfortunately, per device settings are not implemented through
Regards
dave
Hi Andrew,
Sorry for reply to myself, does the Jens Axboe's email is outdated?
which one is the latest?
And Jens, could you please update your email address?
On Thu, 14 Jun 2007 06:40:49 +0000 dave young wrote:
> Hi Andrew,
>
> Sorry for reply to myself, does the Jens Axboe's email is outdated?
> which one is the latest?
>
> And Jens, could you please update your email address?
Better to use the email address in the MAINTAINERS file than
the one in the driver source file.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
Hi,
> Better to use the email address in the MAINTAINERS file than
> the one in the driver source file.
Really? I searched the list, found axboe use the address
[email protected], same as what andrew said. does the MAINTAINERS
file be updated?
Regards
dave
dave young wrote:
> Hi,
>> Better to use the email address in the MAINTAINERS file than
>> the one in the driver source file.
>
> Really? I searched the list, found axboe use the address
> [email protected], same as what andrew said. does the MAINTAINERS
> file be updated?
Could be, but that's up to Jens.
--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
On Fri, Jun 15 2007, dave young wrote:
> Hi,
> >Better to use the email address in the MAINTAINERS file than
> >the one in the driver source file.
>
> Really? I searched the list, found axboe use the address
> [email protected], same as what andrew said. does the MAINTAINERS
> file be updated?
Hey, no point in CC'ing me twice! [email protected] works just fine as
well.
--
Jens Axboe
Hi,
>On Fri, Jun 15, 2007 at 07:58:47AM +0200, Jens Axboe wrote:
> Hey, no point in CC'ing me twice! [email protected] works just fine as
> well.
>
I'm sorry for it , jens. Now I resend the patch after remove some checkpatch.pl warnings(line breaking trailing space).
If user have many cdrom drives, then the buffer is not enough, so I write this patch along with a little coding style fixing. Do you think I need to rewrite to two patches?
Regards
dave
----------------------
From: Dave Young <[email protected]>
cdrom_sysctl_info may cause buffer overwrite.
Signed-off-by: Dave Young <[email protected]>
---
diff -upr linux/drivers/cdrom/cdrom.c linux.new/drivers/cdrom/cdrom.c
--- linux/drivers/cdrom/cdrom.c 2007-06-14 14:05:04.000000000 +0000
+++ linux.new/drivers/cdrom/cdrom.c 2007-06-15 13:24:10.000000000 +0000
@@ -3290,102 +3290,120 @@ static struct cdrom_sysctl_settings {
} cdrom_sysctl_settings;
static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
- void __user *buffer, size_t *lenp, loff_t *ppos)
+ void __user *buffer, size_t *lenp, loff_t *ppos)
{
- int pos;
+ int pos;
struct cdrom_device_info *cdi;
char *info = cdrom_sysctl_settings.info;
+ int size = sizeof(cdrom_sysctl_settings.info);
if (!*lenp || (*ppos && !write)) {
*lenp = 0;
return 0;
}
- pos = sprintf(info, "CD-ROM information, " VERSION "\n");
+ pos = scnprintf(info, size, "CD-ROM information, " VERSION "\n");
- pos += sprintf(info+pos, "\ndrive name:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%s", cdi->name);
-
- pos += sprintf(info+pos, "\ndrive speed:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", cdi->speed);
-
- pos += sprintf(info+pos, "\ndrive # of slots:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", cdi->capacity);
-
- pos += sprintf(info+pos, "\nCan close tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
-
- pos += sprintf(info+pos, "\nCan open tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
-
- pos += sprintf(info+pos, "\nCan lock tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
-
- pos += sprintf(info+pos, "\nCan change speed:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
-
- pos += sprintf(info+pos, "\nCan select disk:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
-
- pos += sprintf(info+pos, "\nCan read multisession:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
-
- pos += sprintf(info+pos, "\nCan read MCN:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
-
- pos += sprintf(info+pos, "\nReports media changed:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
-
- pos += sprintf(info+pos, "\nCan play audio:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
-
- pos += sprintf(info+pos, "\nCan write CD-R:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
-
- pos += sprintf(info+pos, "\nCan write CD-RW:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
-
- pos += sprintf(info+pos, "\nCan read DVD:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
-
- pos += sprintf(info+pos, "\nCan write DVD-R:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
-
- pos += sprintf(info+pos, "\nCan write DVD-RAM:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
-
- pos += sprintf(info+pos, "\nCan read MRW:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
-
- pos += sprintf(info+pos, "\nCan write MRW:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
-
- pos += sprintf(info+pos, "\nCan write RAM:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
+ pos += scnprintf(info + pos, size - pos, "\ndrive name:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%s", cdi->name);
+
+ pos += scnprintf(info + pos, size - pos, "\ndrive speed:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d", cdi->speed);
+
+ pos += scnprintf(info + pos, size - pos, "\ndrive # of slots:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d", cdi->capacity);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan close tray:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_CLOSE_TRAY) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan open tray:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_OPEN_TRAY) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan lock tray:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_LOCK) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan change speed:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_SELECT_SPEED) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan select disk:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_SELECT_DISC) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan read multisession:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MULTI_SESSION) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan read MCN:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MCN) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nReports media changed:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan play audio:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_PLAY_AUDIO) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write CD-R:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_CD_R) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write CD-RW:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_CD_RW) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan read DVD:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_DVD) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write DVD-R:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_DVD_R) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write DVD-RAM:");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_DVD_RAM) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan read MRW:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MRW) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write MRW:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_MRW_W) != 0);
+
+ pos += scnprintf(info + pos, size - pos, "\nCan write RAM:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, size - pos, "\t%d",
+ CDROM_CAN(CDC_RAM) != 0);
- strcpy(info+pos,"\n\n");
+ scnprintf(info + pos, size - pos, "\n\n");
- return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+ return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
}
/* Unfortunately, per device settings are not implemented through
On Thu, Jun 14 2007, Dave Young wrote:
> Hi,
>
> Fix the cdrom_sysctl_info possible buffer overwrite bug. Somd
> codingstyle fixes are included as well.
How about something like this? The current code is actually racy,
because there's no protection against adding/removing a cdrom while it
runs. This patch is largely based on yours, just abstracted the printing
and looping into a function and bail when we run out of room (and print
a message to that effect).
If you could test it and verify that it does the right thing (eg prints
the same as before), that would be great!
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 3625a05..ae5a475 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -302,7 +302,7 @@ module_param(lockdoor, bool, 0);
module_param(check_media_type, bool, 0);
module_param(mrw_format_restart, bool, 0);
-static DEFINE_SPINLOCK(cdrom_lock);
+static DEFINE_MUTEX(cdrom_mutex);
static const char *mrw_format_status[] = {
"not mrw",
@@ -438,10 +438,10 @@ int register_cdrom(struct cdrom_device_info *cdi)
cdo->generic_packet = cdrom_dummy_generic_packet;
cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
- spin_lock(&cdrom_lock);
+ mutex_lock(&cdrom_mutex);
cdi->next = topCdromPtr;
topCdromPtr = cdi;
- spin_unlock(&cdrom_lock);
+ mutex_unlock(&cdrom_mutex);
return 0;
}
#undef ENSURE
@@ -452,7 +452,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
cdinfo(CD_OPEN, "entering unregister_cdrom\n");
prev = NULL;
- spin_lock(&cdrom_lock);
+ mutex_lock(&cdrom_mutex);
cdi = topCdromPtr;
while (cdi && cdi != unreg) {
prev = cdi;
@@ -460,7 +460,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
}
if (cdi == NULL) {
- spin_unlock(&cdrom_lock);
+ mutex_unlock(&cdrom_mutex);
return -2;
}
if (prev)
@@ -468,7 +468,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
else
topCdromPtr = cdi->next;
- spin_unlock(&cdrom_lock);
+ mutex_unlock(&cdrom_mutex);
if (cdi->exit)
cdi->exit(cdi);
@@ -3289,103 +3289,112 @@ static struct cdrom_sysctl_settings {
int check; /* check media type */
} cdrom_sysctl_settings;
+static int cdrom_print_int(const char *header, int val, char *info, int *pos)
+{
+ const int max_size = sizeof(cdrom_sysctl_settings.info);
+ struct cdrom_device_info *cdi;
+ int ret;
+
+ ret = scnprintf(info + *pos, max_size, header);
+ if (!ret)
+ return 1;
+
+ *pos += ret;
+
+ for (cdi = topCdromPtr; cdi; cdi = cdi->next) {
+ ret = scnprintf(info + *pos, max_size, "\t%d", val);
+ if (!ret)
+ return 1;
+ *pos += ret;
+ }
+
+ return 0;
+}
+
static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
- int pos;
+ int pos;
struct cdrom_device_info *cdi;
char *info = cdrom_sysctl_settings.info;
+ const int max_size = sizeof(cdrom_sysctl_settings.info);
if (!*lenp || (*ppos && !write)) {
*lenp = 0;
return 0;
}
+ mutex_lock(&cdrom_mutex);
+
pos = sprintf(info, "CD-ROM information, " VERSION "\n");
- pos += sprintf(info+pos, "\ndrive name:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%s", cdi->name);
-
- pos += sprintf(info+pos, "\ndrive speed:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", cdi->speed);
-
- pos += sprintf(info+pos, "\ndrive # of slots:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", cdi->capacity);
-
- pos += sprintf(info+pos, "\nCan close tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
-
- pos += sprintf(info+pos, "\nCan open tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
-
- pos += sprintf(info+pos, "\nCan lock tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
-
- pos += sprintf(info+pos, "\nCan change speed:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
-
- pos += sprintf(info+pos, "\nCan select disk:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
-
- pos += sprintf(info+pos, "\nCan read multisession:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
-
- pos += sprintf(info+pos, "\nCan read MCN:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
-
- pos += sprintf(info+pos, "\nReports media changed:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
-
- pos += sprintf(info+pos, "\nCan play audio:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
-
- pos += sprintf(info+pos, "\nCan write CD-R:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
-
- pos += sprintf(info+pos, "\nCan write CD-RW:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
-
- pos += sprintf(info+pos, "\nCan read DVD:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
-
- pos += sprintf(info+pos, "\nCan write DVD-R:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
-
- pos += sprintf(info+pos, "\nCan write DVD-RAM:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
-
- pos += sprintf(info+pos, "\nCan read MRW:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
-
- pos += sprintf(info+pos, "\nCan write MRW:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
-
- pos += sprintf(info+pos, "\nCan write RAM:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
-
- strcpy(info+pos,"\n\n");
-
- return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+ pos += scnprintf(info + pos, max_size - pos, "\ndrive name:\t");
+ for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+ pos += scnprintf(info + pos, max_size - pos, "\t%s", cdi->name);
+
+ if (cdrom_print_int("\ndrive speed:\t", cdi->speed, info, &pos))
+ goto done;
+ if (cdrom_print_int("\ndrive # of slots:", cdi->capacity, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan close tray:\t",
+ CDROM_CAN(CDC_CLOSE_TRAY) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan open tray:\t",
+ CDROM_CAN(CDC_OPEN_TRAY) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan lock tray:\t",
+ CDROM_CAN(CDC_LOCK) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan change speed:\t",
+ CDROM_CAN(CDC_SELECT_SPEED) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan select disk:\t",
+ CDROM_CAN(CDC_SELECT_DISC) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan read multisession:",
+ CDROM_CAN(CDC_MULTI_SESSION) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan read MCN:",
+ CDROM_CAN(CDC_MCN) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nReports media changed:",
+ CDROM_CAN(CDC_MEDIA_CHANGED) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan play audio:",
+ CDROM_CAN(CDC_PLAY_AUDIO) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan write CD-R:\t",
+ CDROM_CAN(CDC_CD_R) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan write CD-RW:\t",
+ CDROM_CAN(CDC_CD_RW) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan read DVD:\t",
+ CDROM_CAN(CDC_DVD) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan write DVD-R:\t",
+ CDROM_CAN(CDC_DVD_R) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan write DVD-RAM:\t",
+ CDROM_CAN(CDC_DVD_RAM) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan read MRW:\t",
+ CDROM_CAN(CDC_MRW) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan write MRW:\t",
+ CDROM_CAN(CDC_MRW_W) != 0, info, &pos))
+ goto done;
+ if (cdrom_print_int("\nCan write RAM:\t",
+ CDROM_CAN(CDC_RAM) != 0, info, &pos))
+ goto done;
+ if (!scnprintf(info + pos, max_size - pos, "\n\n"))
+ goto done;
+doit:
+ mutex_unlock(&cdrom_mutex);
+ return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+done:
+ printk(KERN_INFO "cdrom: info buffer too small\n");
+ goto doit;
}
/* Unfortunately, per device settings are not implemented through
--
Jens Axboe
Hi jens,
Thanks for rework this patch.But it has some bugs, results kernel oops.
On Fri, Jun 15, 2007 at 03:26:57PM +0200, Jens Axboe wrote:
> On Thu, Jun 14 2007, Dave Young wrote:
> > Hi,
> >
> > Fix the cdrom_sysctl_info possible buffer overwrite bug. Somd
> > codingstyle fixes are included as well.
>
> How about something like this? The current code is actually racy,
> because there's no protection against adding/removing a cdrom while it
> runs. This patch is largely based on yours, just abstracted the printing
> and looping into a function and bail when we run out of room (and print
> a message to that effect).
You are right
>
> If you could test it and verify that it does the right thing (eg prints
> the same as before), that would be great!
>
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 3625a05..ae5a475 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -302,7 +302,7 @@ module_param(lockdoor, bool, 0);
> module_param(check_media_type, bool, 0);
> module_param(mrw_format_restart, bool, 0);
>
> -static DEFINE_SPINLOCK(cdrom_lock);
> +static DEFINE_MUTEX(cdrom_mutex);
>
> static const char *mrw_format_status[] = {
> "not mrw",
> @@ -438,10 +438,10 @@ int register_cdrom(struct cdrom_device_info *cdi)
> cdo->generic_packet = cdrom_dummy_generic_packet;
>
> cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
> - spin_lock(&cdrom_lock);
> + mutex_lock(&cdrom_mutex);
> cdi->next = topCdromPtr;
> topCdromPtr = cdi;
> - spin_unlock(&cdrom_lock);
> + mutex_unlock(&cdrom_mutex);
> return 0;
> }
> #undef ENSURE
> @@ -452,7 +452,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
> cdinfo(CD_OPEN, "entering unregister_cdrom\n");
>
> prev = NULL;
> - spin_lock(&cdrom_lock);
> + mutex_lock(&cdrom_mutex);
> cdi = topCdromPtr;
> while (cdi && cdi != unreg) {
> prev = cdi;
> @@ -460,7 +460,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
> }
>
> if (cdi == NULL) {
> - spin_unlock(&cdrom_lock);
> + mutex_unlock(&cdrom_mutex);
> return -2;
> }
> if (prev)
> @@ -468,7 +468,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
> else
> topCdromPtr = cdi->next;
>
> - spin_unlock(&cdrom_lock);
> + mutex_unlock(&cdrom_mutex);
>
> if (cdi->exit)
> cdi->exit(cdi);
> @@ -3289,103 +3289,112 @@ static struct cdrom_sysctl_settings {
> int check; /* check media type */
> } cdrom_sysctl_settings;
>
> +static int cdrom_print_int(const char *header, int val, char *info, int *pos)
> +{
The val is diffrent for every devices, isn't it?
> + const int max_size = sizeof(cdrom_sysctl_settings.info);
> + struct cdrom_device_info *cdi;
> + int ret;
> +
> + ret = scnprintf(info + *pos, max_size, header);
> + if (!ret)
> + return 1;
> +
> + *pos += ret;
> +
> + for (cdi = topCdromPtr; cdi; cdi = cdi->next) {
> + ret = scnprintf(info + *pos, max_size, "\t%d", val);
The max_size should be max_size - *pos
> + if (!ret)
> + return 1;
> + *pos += ret;
> + }
> +
> + return 0;
> +}
> +
> static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
> void __user *buffer, size_t *lenp, loff_t *ppos)
> {
> - int pos;
> + int pos;
> struct cdrom_device_info *cdi;
> char *info = cdrom_sysctl_settings.info;
> + const int max_size = sizeof(cdrom_sysctl_settings.info);
>
> if (!*lenp || (*ppos && !write)) {
> *lenp = 0;
> return 0;
> }
>
> + mutex_lock(&cdrom_mutex);
> +
> pos = sprintf(info, "CD-ROM information, " VERSION "\n");
>
> - pos += sprintf(info+pos, "\ndrive name:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%s", cdi->name);
> -
> - pos += sprintf(info+pos, "\ndrive speed:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", cdi->speed);
> -
> - pos += sprintf(info+pos, "\ndrive # of slots:");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", cdi->capacity);
> -
> - pos += sprintf(info+pos, "\nCan close tray:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
> -
> - pos += sprintf(info+pos, "\nCan open tray:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
> -
> - pos += sprintf(info+pos, "\nCan lock tray:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
> -
> - pos += sprintf(info+pos, "\nCan change speed:");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
> -
> - pos += sprintf(info+pos, "\nCan select disk:");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
> -
> - pos += sprintf(info+pos, "\nCan read multisession:");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
> -
> - pos += sprintf(info+pos, "\nCan read MCN:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
> -
> - pos += sprintf(info+pos, "\nReports media changed:");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
> -
> - pos += sprintf(info+pos, "\nCan play audio:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
> -
> - pos += sprintf(info+pos, "\nCan write CD-R:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
> -
> - pos += sprintf(info+pos, "\nCan write CD-RW:");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
> -
> - pos += sprintf(info+pos, "\nCan read DVD:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
> -
> - pos += sprintf(info+pos, "\nCan write DVD-R:");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
> -
> - pos += sprintf(info+pos, "\nCan write DVD-RAM:");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
> -
> - pos += sprintf(info+pos, "\nCan read MRW:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
> -
> - pos += sprintf(info+pos, "\nCan write MRW:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
> -
> - pos += sprintf(info+pos, "\nCan write RAM:\t");
> - for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> - pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
> -
> - strcpy(info+pos,"\n\n");
> -
> - return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
> + pos += scnprintf(info + pos, max_size - pos, "\ndrive name:\t");
> + for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
> + pos += scnprintf(info + pos, max_size - pos, "\t%s", cdi->name);
> +
> + if (cdrom_print_int("\ndrive speed:\t", cdi->speed, info, &pos))
Now the cdi is NULL
> + goto done;
> + if (cdrom_print_int("\ndrive # of slots:", cdi->capacity, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan close tray:\t",
> + CDROM_CAN(CDC_CLOSE_TRAY) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan open tray:\t",
> + CDROM_CAN(CDC_OPEN_TRAY) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan lock tray:\t",
> + CDROM_CAN(CDC_LOCK) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan change speed:\t",
> + CDROM_CAN(CDC_SELECT_SPEED) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan select disk:\t",
> + CDROM_CAN(CDC_SELECT_DISC) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan read multisession:",
> + CDROM_CAN(CDC_MULTI_SESSION) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan read MCN:",
> + CDROM_CAN(CDC_MCN) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nReports media changed:",
> + CDROM_CAN(CDC_MEDIA_CHANGED) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan play audio:",
> + CDROM_CAN(CDC_PLAY_AUDIO) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan write CD-R:\t",
> + CDROM_CAN(CDC_CD_R) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan write CD-RW:\t",
> + CDROM_CAN(CDC_CD_RW) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan read DVD:\t",
> + CDROM_CAN(CDC_DVD) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan write DVD-R:\t",
> + CDROM_CAN(CDC_DVD_R) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan write DVD-RAM:\t",
> + CDROM_CAN(CDC_DVD_RAM) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan read MRW:\t",
> + CDROM_CAN(CDC_MRW) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan write MRW:\t",
> + CDROM_CAN(CDC_MRW_W) != 0, info, &pos))
> + goto done;
> + if (cdrom_print_int("\nCan write RAM:\t",
> + CDROM_CAN(CDC_RAM) != 0, info, &pos))
> + goto done;
> + if (!scnprintf(info + pos, max_size - pos, "\n\n"))
> + goto done;
> +doit:
> + mutex_unlock(&cdrom_mutex);
> + return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
> +done:
> + printk(KERN_INFO "cdrom: info buffer too small\n");
> + goto doit;
> }
>
> /* Unfortunately, per device settings are not implemented through
>
> --
> Jens Axboe
And there's several more '\t' in the printed strings. so the result not aligned right.
I rewite the patch according to yours, please help to check. Pass the cdrom_print_info function one more argument for diffrent type printing, do you have better solutions for this? thanks for replying :)
Signed-off-by: Dave Young <[email protected]>
---
diff -upr linux/drivers/cdrom/cdrom.c linux.new/drivers/cdrom/cdrom.c
--- linux/drivers/cdrom/cdrom.c 2007-06-18 12:36:40.000000000 +0000
+++ linux.new/drivers/cdrom/cdrom.c 2007-06-18 12:38:51.000000000 +0000
@@ -302,7 +302,7 @@ module_param(lockdoor, bool, 0);
module_param(check_media_type, bool, 0);
module_param(mrw_format_restart, bool, 0);
-static DEFINE_SPINLOCK(cdrom_lock);
+static DEFINE_MUTEX(cdrom_mutex);
static const char *mrw_format_status[] = {
"not mrw",
@@ -438,10 +438,10 @@ int register_cdrom(struct cdrom_device_i
cdo->generic_packet = cdrom_dummy_generic_packet;
cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
- spin_lock(&cdrom_lock);
+ mutex_lock(&cdrom_mutex);
cdi->next = topCdromPtr;
topCdromPtr = cdi;
- spin_unlock(&cdrom_lock);
+ mutex_unlock(&cdrom_mutex);
return 0;
}
#undef ENSURE
@@ -452,7 +452,7 @@ int unregister_cdrom(struct cdrom_device
cdinfo(CD_OPEN, "entering unregister_cdrom\n");
prev = NULL;
- spin_lock(&cdrom_lock);
+ mutex_lock(&cdrom_mutex);
cdi = topCdromPtr;
while (cdi && cdi != unreg) {
prev = cdi;
@@ -460,7 +460,7 @@ int unregister_cdrom(struct cdrom_device
}
if (cdi == NULL) {
- spin_unlock(&cdrom_lock);
+ mutex_unlock(&cdrom_mutex);
return -2;
}
if (prev)
@@ -468,7 +468,7 @@ int unregister_cdrom(struct cdrom_device
else
topCdromPtr = cdi->next;
- spin_unlock(&cdrom_lock);
+ mutex_unlock(&cdrom_mutex);
if (cdi->exit)
cdi->exit(cdi);
@@ -3289,103 +3289,137 @@ static struct cdrom_sysctl_settings {
int check; /* check media type */
} cdrom_sysctl_settings;
+enum cdrom_print_option {
+ CTL_NAME,
+ CTL_SPEED,
+ CTL_SLOTS,
+ CTL_CAPABILITY
+};
+
+static int cdrom_print_info(const char *header, int val, char *info,
+ int *pos, enum cdrom_print_option option)
+{
+ const int max_size = sizeof(cdrom_sysctl_settings.info);
+ struct cdrom_device_info *cdi;
+ int ret;
+
+ ret = scnprintf(info + *pos, max_size - *pos, header);
+ if (!ret)
+ return 1;
+
+ *pos += ret;
+
+ for (cdi = topCdromPtr; cdi; cdi = cdi->next) {
+ switch (option) {
+ case CTL_NAME:
+ ret = scnprintf(info + *pos, max_size - *pos,
+ "\t%s", cdi->name);
+ break;
+ case CTL_SPEED:
+ ret = scnprintf(info + *pos, max_size - *pos,
+ "\t%d", cdi->speed);
+ break;
+ case CTL_SLOTS:
+ ret = scnprintf(info + *pos, max_size - *pos,
+ "\t%d", cdi->capacity);
+ break;
+ case CTL_CAPABILITY:
+ ret = scnprintf(info + *pos, max_size - *pos,
+ "\t%d", CDROM_CAN(val) != 0);
+ break;
+ default:
+ printk(KERN_INFO "cdrom: invalid option%d\n", option);
+ return 1;
+ }
+ if (!ret)
+ return 1;
+ *pos += ret;
+ }
+
+ return 0;
+}
+
static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
- int pos;
- struct cdrom_device_info *cdi;
+ int pos;
char *info = cdrom_sysctl_settings.info;
+ const int max_size = sizeof(cdrom_sysctl_settings.info);
if (!*lenp || (*ppos && !write)) {
*lenp = 0;
return 0;
}
+ mutex_lock(&cdrom_mutex);
+
pos = sprintf(info, "CD-ROM information, " VERSION "\n");
- pos += sprintf(info+pos, "\ndrive name:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%s", cdi->name);
-
- pos += sprintf(info+pos, "\ndrive speed:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", cdi->speed);
-
- pos += sprintf(info+pos, "\ndrive # of slots:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", cdi->capacity);
-
- pos += sprintf(info+pos, "\nCan close tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
-
- pos += sprintf(info+pos, "\nCan open tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
-
- pos += sprintf(info+pos, "\nCan lock tray:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
-
- pos += sprintf(info+pos, "\nCan change speed:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
-
- pos += sprintf(info+pos, "\nCan select disk:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
-
- pos += sprintf(info+pos, "\nCan read multisession:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
-
- pos += sprintf(info+pos, "\nCan read MCN:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
-
- pos += sprintf(info+pos, "\nReports media changed:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
-
- pos += sprintf(info+pos, "\nCan play audio:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
-
- pos += sprintf(info+pos, "\nCan write CD-R:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
-
- pos += sprintf(info+pos, "\nCan write CD-RW:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
-
- pos += sprintf(info+pos, "\nCan read DVD:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
-
- pos += sprintf(info+pos, "\nCan write DVD-R:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
-
- pos += sprintf(info+pos, "\nCan write DVD-RAM:");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
-
- pos += sprintf(info+pos, "\nCan read MRW:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
-
- pos += sprintf(info+pos, "\nCan write MRW:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
-
- pos += sprintf(info+pos, "\nCan write RAM:\t");
- for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
- pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
-
- strcpy(info+pos,"\n\n");
-
- return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+ if (cdrom_print_info("\ndrive name:\t", 0, info, &pos, CTL_NAME))
+ goto done;
+ if (cdrom_print_info("\ndrive speed:\t", 0, info, &pos, CTL_SPEED))
+ goto done;
+ if (cdrom_print_info("\ndrive # of slots:", 0, info, &pos, CTL_SLOTS))
+ goto done;
+ if (cdrom_print_info("\nCan close tray:\t",
+ CDC_CLOSE_TRAY, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan open tray:\t",
+ CDC_OPEN_TRAY, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan lock tray:\t",
+ CDC_LOCK, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan change speed:",
+ CDC_SELECT_SPEED, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan select disk:",
+ CDC_SELECT_DISC, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan read multisession:",
+ CDC_MULTI_SESSION, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan read MCN:\t",
+ CDC_MCN, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nReports media changed:",
+ CDC_MEDIA_CHANGED, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan play audio:\t",
+ CDC_PLAY_AUDIO, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan write CD-R:\t",
+ CDC_CD_R, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan write CD-RW:",
+ CDC_CD_RW, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan read DVD:\t",
+ CDC_DVD, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan write DVD-R:",
+ CDC_DVD_R, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan write DVD-RAM:",
+ CDC_DVD_RAM, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan read MRW:\t",
+ CDC_MRW, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan write MRW:\t",
+ CDC_MRW_W, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (cdrom_print_info("\nCan write RAM:\t",
+ CDC_RAM, info, &pos, CTL_CAPABILITY))
+ goto done;
+ if (!scnprintf(info + pos, max_size - pos, "\n\n"))
+ goto done;
+doit:
+ mutex_unlock(&cdrom_mutex);
+ return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+done:
+ printk(KERN_INFO "cdrom: info buffer too small\n");
+ goto doit;
}
/* Unfortunately, per device settings are not implemented through
Only in linux/drivers/cdrom: cdrom.old
--
Regards
dave
On Mon, Jun 18 2007, Dave Young wrote:
> I rewite the patch according to yours, please help to check. Pass the
> cdrom_print_info function one more argument for diffrent type
> printing, do you have better solutions for this? thanks for replying
> :)
Yep that looks much better! I trust you booted and tested this and the
output looks correct?
--
Jens Axboe
Hi,
> Yep that looks much better! I trust you booted and tested this and the
> output looks correct?
Yes, I tested and the output is correct.
BTW, another problem, I can't find the CONFIG_CDROM option in kernel
config menu. Is it deperacated? If it is true, is the module part
still necessary?
Regards
dave
On Mon, Jun 18 2007, dave young wrote:
> Hi,
> >Yep that looks much better! I trust you booted and tested this and the
> >output looks correct?
> Yes, I tested and the output is correct.
Good
> BTW, another problem, I can't find the CONFIG_CDROM option in kernel
> config menu. Is it deperacated? If it is true, is the module part
> still necessary?
cdrom.o is a hardware independent helper module, it gets included if you
use atapi/scsi/etc cdrom drivers. See drivers/cdrom/Makefile.
--
Jens Axboe
Hi,
> > BTW, another problem, I can't find the CONFIG_CDROM option in kernel
> > config menu. Is it deperacated? If it is true, is the module part
> > still necessary?
>
> cdrom.o is a hardware independent helper module, it gets included if you
> use atapi/scsi/etc cdrom drivers. See drivers/cdrom/Makefile.
>
I know. Sorry for my bad english, I means MODULE's part maybe
deperacated, not uniform cdrom driver itsself.
For example the module_init, module_exit and MODULE_LICENSE, I
remember it could be a standalone module in 2.4 kernels. But now seems
not possible. am I right?
Regards
dave
On Mon, Jun 18 2007, dave young wrote:
> Hi,
> >> BTW, another problem, I can't find the CONFIG_CDROM option in kernel
> >> config menu. Is it deperacated? If it is true, is the module part
> >> still necessary?
> >
> >cdrom.o is a hardware independent helper module, it gets included if you
> >use atapi/scsi/etc cdrom drivers. See drivers/cdrom/Makefile.
> >
> I know. Sorry for my bad english, I means MODULE's part maybe
> deperacated, not uniform cdrom driver itsself.
>
> For example the module_init, module_exit and MODULE_LICENSE, I
> remember it could be a standalone module in 2.4 kernels. But now seems
> not possible. am I right?
It is a stand-alone module.
--
Jens Axboe
Hey Guys,
I can't understand the following instructions from
arch/ppc/mm/hashtable.S::hash_page. If I got the right design, the
following instruction is to get the PMD (Page Middle Descritor) because
Linux for 32-bits PowerPC cut page table into 3 domains: root, PMD, PTE.
The top bits (22 to 31 bit) is the index for PMD, and the next 10 bits
(12 to 21 bit) is the index for PTE in the associative PMD. The
remaining 12 bits (0 to 11 bit) indicated the page size (4KB). However,
the following instruction polled [8-17] bits instead of [22-31] bits as
expected. Anybody could give me answer?
r4 is the address that caused the DSI
r5 is the address of swapper_pg_dir if we are under kernel mode.
rlwimi r5,r4,12,20,29 /* insert top 10 bits of address */
Thanks,
Gavin
On Mon, 18 Jun 2007 16:33:22 +0800, gshan <[email protected]> wrote:
> I can't understand the following instructions from
> arch/ppc/mm/hashtable.S::hash_page. If I got the right design, the
> following instruction is to get the PMD (Page Middle Descritor) because
> Linux for 32-bits PowerPC cut page table into 3 domains: root, PMD, PTE.
> The top bits (22 to 31 bit) is the index for PMD, and the next 10 bits
> (12 to 21 bit) is the index for PTE in the associative PMD. The
> remaining 12 bits (0 to 11 bit) indicated the page size (4KB). However,
> the following instruction polled [8-17] bits instead of [22-31] bits as
> expected. Anybody could give me answer?
>
> r4 is the address that caused the DSI
> r5 is the address of swapper_pg_dir if we are under kernel mode.
>
> rlwimi r5,r4,12,20,29 /* insert top 10 bits of address */
POWER/PowerPC has an insanely broken bit numbering scheme, in
which the most significant bit has number 0 (or is it 1?),
and the least significant bit has number N-1 (or is it N?)
where N is number of bits in a word.
The fact that you refer to the top bits as 22-31 makes me
suspect that you haven't compensated for this quirk.
/Mikael
Mikael Pettersson wrote:
> On Mon, 18 Jun 2007 16:33:22 +0800, gshan <[email protected]> wrote:
>
>> I can't understand the following instructions from
>> arch/ppc/mm/hashtable.S::hash_page. If I got the right design, the
>> following instruction is to get the PMD (Page Middle Descritor) because
>> Linux for 32-bits PowerPC cut page table into 3 domains: root, PMD, PTE.
>> The top bits (22 to 31 bit) is the index for PMD, and the next 10 bits
>> (12 to 21 bit) is the index for PTE in the associative PMD. The
>> remaining 12 bits (0 to 11 bit) indicated the page size (4KB). However,
>> the following instruction polled [8-17] bits instead of [22-31] bits as
>> expected. Anybody could give me answer?
>>
>> r4 is the address that caused the DSI
>> r5 is the address of swapper_pg_dir if we are under kernel mode.
>>
>> rlwimi r5,r4,12,20,29 /* insert top 10 bits of address */
>>
>
> POWER/PowerPC has an insanely broken bit numbering scheme, in
> which the most significant bit has number 0 (or is it 1?),
> and the least significant bit has number N-1 (or is it N?)
> where N is number of bits in a word.
>
> The fact that you refer to the top bits as 22-31 makes me
> suspect that you haven't compensated for this quirk.
>
> /Mikael
>
Mikael, Thanks a lot. I understood why the instruction is there. So the
0-11 bits are used as address index for PMD. swapper_pg_dir is 4KB large
and there are 1K PMDs inside swapper_pg_dir. So 10 bits (0-9 bits) are
used as index for PMD, right?