login
Search
After updating my kernel to 2.6.26 the output for the ide-tape drive during booting is garbled eg ide-tape: hdd <-> ht0: Seagate <98>ß8A51|1À<81>ܺ<98>ß STT20000A rev 8A51|1À<81>ܺ<98>ß This patch fixes the problem by NULL terminating the strings. Regards, Mark de Wever PS: please CC me since I'm not subscribed. PPS: there are more problems with my tapestreamer in 2.6.26 but I'll post a separate message for that. Signed-off-by: Mark de Wever <koraq@xs4all.nl> --- drivers/ide/ide-tape.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c index 1bce84b..fd87b43 100644 --- a/drivers/ide/ide-tape.c +++ b/drivers/ide/ide-tape.c @@ -2354,6 +2354,10 @@ static void idetape_get_inquiry_results(ide_drive_t *drive) ide_fixstring(product_id, 18, 0); ide_fixstring(fw_rev, 6, 0); + fw_rev[4] = '\0'; + vendor_id[8] = '\0'; + product_id[16] = '\0'; + printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n", drive->name, tape->name, vendor_id, product_id, fw_rev); } -- 1.5.6.5 --
Hello.
This is not quite correct way to tackle this. More correct would be to
fix the format specifiers in this printk() to only print no more than N string
characters like this:
printk(KERN_INFO "ide-tape: %s <-> %s: %.8s %.16s rev %.4s\n",
drive->name, tape->name, vendor_id, product_id, fw_rev);
MBR, Sergei
--
Hm, I see that every string variable declared there has 2 extra characters, and yet the author have managed to make a mistake... these extra chars don't seem needed. MBR, Sergei --
Those extra characters made me believe the intention was setting the NULL character, therefore I used that solution in my patch. Regards, Mark de Wever --
Hello. It was wrong to call ide_fixstring() on those "long" stings without first setting their last byte to 0 -- this way that function just copied the garbage at the end of the passed unterminated strings. I suggest that you get rid of those extra bytes as well, and use the MBR, Sergei --
Hello.
Looks like this bugs was introduced by this commit:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=41f81d...
MBR, Sergei
--
On Mon, Sep 22, 2008 at 3:16 PM, Sergei Shtylyov
.. and I know why :). Those ide_tape_obj members (char fw_rev[6], vendor_id[10],
product_id[18]) were used only once in idetape_get_inquiry_results() so I moved
them there as local stack variables. Originally, they were kzalloc'ed as part of
struct ide_tape_obj and now they contain stack garbage therefore the funny
values. The simple solution would be to zero them out or:
Does the following patch help?
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 1bce84b..848d9df 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -2338,7 +2338,7 @@ static void
idetape_get_inquiry_results(ide_drive_t *drive)
{
idetape_tape_t *tape = drive->driver_data;
struct ide_atapi_pc pc;
- char fw_rev[6], vendor_id[10], product_id[18];
+ static char fw_rev[6], vendor_id[10], product_id[18];
idetape_create_inquiry_cmd(&pc);
if (idetape_queue_pc_tail(drive, &pc)) {
--
Regards/Gruss,
Boris
--
Yes feel free to add my tested-by.
Only not sure whether the static is the best solution, the following
patch also works, by zeroing the memory as you suggested.
Signed-off-by: Mark de Wever <koraq@xs4all.nl>
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 1bce84b..c41f5b1 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
{
idetape_tape_t *tape = drive->driver_data;
struct ide_atapi_pc pc;
- char fw_rev[6], vendor_id[10], product_id[18];
+ char fw_rev[6] = {'\0'}, vendor_id[10] = {'\0'}, product_id[18] = {'\0'};
idetape_create_inquiry_cmd(&pc);
if (idetape_queue_pc_tail(drive, &pc)) {
--
Regards,
Mark de Wever
--
Hello. Do you realize how much *absolutely unnecessary* code will this bring in? This is certainly worse than your initial patch (if it was correct). Ugh, looks like I'll have t submit the patch myself to stop this ugliness... MBR, Sergei --
Is this what you had in mind?
---
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 1bce84b..3833189 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
{
idetape_tape_t *tape = drive->driver_data;
struct ide_atapi_pc pc;
- char fw_rev[6], vendor_id[10], product_id[18];
+ char fw_rev[4], vendor_id[8], product_id[16];
idetape_create_inquiry_cmd(&pc);
if (idetape_queue_pc_tail(drive, &pc)) {
@@ -2350,11 +2350,11 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
memcpy(product_id, &pc.buf[16], 16);
memcpy(fw_rev, &pc.buf[32], 4);
- ide_fixstring(vendor_id, 10, 0);
- ide_fixstring(product_id, 18, 0);
- ide_fixstring(fw_rev, 6, 0);
+ ide_fixstring(vendor_id, 8, 0);
+ ide_fixstring(product_id, 16, 0);
+ ide_fixstring(fw_rev, 4, 0);
- printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
+ printk(KERN_INFO "ide-tape: %s <-> %s: %.8s %.16s rev %.4s\n",
drive->name, tape->name, vendor_id, product_id, fw_rev);
}
--
Regards/Gruss,
Boris.
--
On Tue, Sep 23, 2008 at 11:29 AM, Sergei Shtylyov Mark, can you please test? Thanks. -- Regards/Gruss, Boris --
I think this patch looks good, better as all previous versions (including my initial version). I just tested it and it solves the problem. Feel free to add my tested-by. Regards, Mark de Wever PS I was not able to test earlier ;-) --
No worries and thanks. By the way, you mentioned something about other problems with ide-tape. FWIW, it seems you're its only user and we were about to kill it but decided not to yet and did the whole cleanup kinda only by compile-testing since almost no one (well, except you :)) has the hardware. So, feel free to send me any dmesg/debug/error messages you get and I'll look into them. Also, set IDETAPE_DEBUG_LOG to 1 before compiling for full debug info. Thanks. -- Regards/Gruss, Boris --
Thanks for not killing the driver :-) Feel free to add me to your list
of testers in case you need more testing in the future.
I already posted the problem [1] but didn't CC you since your name
wasn't listed in MAINTAINERS. I recompiled the kernel and tested again,
but the output doesn't seem to contain extra output.
In order to compile the kernel with IDETAPE_DEBUG_LOG enabled I had to
apply the following build fix.
Signed-off-by: Mark de Wever <koraq@xs4all.nl>
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 3833189..7258eca 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -978,9 +978,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
struct request *postponed_rq = tape->postponed_rq;
u8 stat;
- debug_log(DBG_SENSE, "sector: %ld, nr_sectors: %ld,"
+ debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %ld,"
" current_nr_sectors: %d\n",
- rq->sector, rq->nr_sectors, rq->current_nr_sectors);
+ (unsigned long long)rq->sector, rq->nr_sectors,
+ rq->current_nr_sectors);
if (!blk_special_request(rq)) {
/* We do not support buffer cache originated requests. */
[1] http://marc.info/?l=linux-kernel&m=122203193728465&w=2
Regards,
Mark de Wever
--
I never got a reaction on this patch, but please apply it.
In order to compile the kernel with IDETAPE_DEBUG_LOG enabled I had to
apply the following build fix.
Signed-off-by: Mark de Wever <koraq@xs4all.nl>
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 3833189..7258eca 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -978,9 +978,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
struct request *postponed_rq = tape->postponed_rq;
u8 stat;
- debug_log(DBG_SENSE, "sector: %ld, nr_sectors: %ld,"
+ debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %ld,"
" current_nr_sectors: %d\n",
- rq->sector, rq->nr_sectors, rq->current_nr_sectors);
+ (unsigned long long)rq->sector, rq->nr_sectors,
+ rq->current_nr_sectors);
if (!blk_special_request(rq)) {
/* We do not support buffer cache originated requests. */
Regards,
Mark de Wever
--
Hi,
Bugger, I should be getting at least warnings when compiling this but I don't.
This is because I don't have CONFIG_LBD enabled. However, the %ld and %d
format specifiers are also not entirely correct but gcc doesn't warn about them
Would you like to redo your patch and add a proper comment ontop?
Thanks.
--
Regards/Gruss,
Boris.
--
On Wed, Oct 08, 2008 at 08:33:52AM +0200, Borislav Petkov wrote:
Here you go.
@Boris do you want to add your Signed-off-by too?
@Bart please apply this patch.
--
Subject: ide-tape: Buildfix when IDETAPE_DEBUG_LOG is set to 1.
The format specifier for rq->sector didn't specify the proper size and
signedness. Borislav Petkov discovered that the signedness for
rq->nr_sectors and rq->current_nr_sectors also were incorrect.
Signed-off-by: Mark de Wever <koraq@xs4all.nl>
---
drivers/ide/ide-tape.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 3833189..3d5f782 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -978,9 +978,10 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
struct request *postponed_rq = tape->postponed_rq;
u8 stat;
- debug_log(DBG_SENSE, "sector: %ld, nr_sectors: %ld,"
- " current_nr_sectors: %d\n",
- rq->sector, rq->nr_sectors, rq->current_nr_sectors);
+ debug_log(DBG_SENSE, "sector: %llu, nr_sectors: %lu,"
+ " current_nr_sectors: %u\n",
+ (unsigned long long)rq->sector, rq->nr_sectors,
+ rq->current_nr_sectors);
if (!blk_special_request(rq)) {
/* We do not support buffer cache originated requests. */
--
1.5.6.5
Regards,
Mark de Wever
--
Bart,
please apply the following patch.
@Sergei: would you like to add your Signed-off-by too?
--
Subject: ide-tape: fix vendor strings
Remove superfluous two bytes from each string buffer and add proper length
format specifiers.
There should be no functional change resulting from this patch.
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
Tested-by: Mark de Wever <koraq@xs4all.nl>
--
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 1bce84b..3833189 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -2338,7 +2338,7 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
{
idetape_tape_t *tape = drive->driver_data;
struct ide_atapi_pc pc;
- char fw_rev[6], vendor_id[10], product_id[18];
+ char fw_rev[4], vendor_id[8], product_id[16];
idetape_create_inquiry_cmd(&pc);
if (idetape_queue_pc_tail(drive, &pc)) {
@@ -2350,11 +2350,11 @@ static void idetape_get_inquiry_results(ide_drive_t *drive)
memcpy(product_id, &pc.buf[16], 16);
memcpy(fw_rev, &pc.buf[32], 4);
- ide_fixstring(vendor_id, 10, 0);
- ide_fixstring(product_id, 18, 0);
- ide_fixstring(fw_rev, 6, 0);
+ ide_fixstring(vendor_id, 8, 0);
+ ide_fixstring(product_id, 16, 0);
+ ide_fixstring(fw_rev, 4, 0);
- printk(KERN_INFO "ide-tape: %s <-> %s: %s %s rev %s\n",
+ printk(KERN_INFO "ide-tape: %s <-> %s: %.8s %.16s rev %.4s\n",
drive->name, tape->name, vendor_id, product_id, fw_rev);
}
--
Regards/Gruss,
Boris.
--
Hello. Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com> MBR, Sergei --
On Wednesday 24 September 2008, Sergei Shtylyov wrote: applied --
I did not, I just had a look at the code GCC produced. I did expect much My initial patch did work, but that doesn't matter much, since Boris posted another patch based on your suggestions. I like that patch better as my initial patch. I'm testing it now and I expect it to work. Regards, Mark de Wever --
My imagination sufficed to foresee how much code a compiler would have to
produce to completely initialize the arrays of the *auto* memory class -- even
If ide_fixstring() wouldn't have to do any space compression, it would
work. If it would have to compress spaces, 2 garbage characters would be
WBR, Sergei
--
I expected a rep stos based code, which isn't much code. But the code was created without a rep, instead it used several stos opcodes. I agree with the mostly no purpose part, since the memcpy later overwrites most It worked, thanks for your suggestion :-) Regards, Mark de Wever --
