* [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures
@ 2004-11-28 16:32 Jesper Juhl
2004-11-28 17:34 ` Alan Cox
0 siblings, 1 reply; 7+ messages in thread
From: Jesper Juhl @ 2004-11-28 16:32 UTC (permalink / raw)
To: linux-kernel; +Cc: Gadi Oxman, Jens Axboe, Andrew Morton
This patch ensures that copy_to|from_user() return values get checked and
dealt with by returning -EFAULT if they fail.
Aside from the fact that we really want to handle these failures, this
patch also silences these warnings:
drivers/ide/ide-tape.c: In function `idetape_copy_stage_from_user':
drivers/ide/ide-tape.c:2613: warning: ignoring return value of `copy_from_user', declared with attribute warn_unused_result
drivers/ide/ide-tape.c: In function `idetape_copy_stage_to_user':
drivers/ide/ide-tape.c:2640: warning: ignoring return value of `copy_to_user', declared with attribute warn_unused_result
Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
diff -u linux-2.6.10-rc2-bk11/drivers/ide/ide-tape.c.old linux-2.6.10-rc2-bk11/drivers/ide/ide-tape.c
--- linux-2.6.10-rc2-bk11/drivers/ide/ide-tape.c.old 2004-11-28 16:56:18.000000000 +0100
+++ linux-2.6.10-rc2-bk11/drivers/ide/ide-tape.c 2004-11-28 16:58:25.000000000 +0100
@@ -2596,7 +2596,7 @@
return __idetape_kmalloc_stage(tape, 0, 0);
}
-static void idetape_copy_stage_from_user(idetape_tape_t *tape, idetape_stage_t *stage, const char __user *buf, int n)
+static int idetape_copy_stage_from_user(idetape_tape_t *tape, idetape_stage_t *stage, const char __user *buf, int n)
{
struct idetape_bh *bh = tape->bh;
int count;
@@ -2606,11 +2606,12 @@
if (bh == NULL) {
printk(KERN_ERR "ide-tape: bh == NULL in "
"idetape_copy_stage_from_user\n");
- return;
+ return 0;
}
#endif /* IDETAPE_DEBUG_BUGS */
count = min((unsigned int)(bh->b_size - atomic_read(&bh->b_count)), (unsigned int)n);
- copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count);
+ if (copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count))
+ return -EFAULT;
n -= count;
atomic_add(count, &bh->b_count);
buf += count;
@@ -2621,9 +2622,11 @@
}
}
tape->bh = bh;
+
+ return 0;
}
-static void idetape_copy_stage_to_user(idetape_tape_t *tape, char __user *buf, idetape_stage_t *stage, int n)
+static int idetape_copy_stage_to_user(idetape_tape_t *tape, char __user *buf, idetape_stage_t *stage, int n)
{
struct idetape_bh *bh = tape->bh;
int count;
@@ -2633,11 +2636,12 @@
if (bh == NULL) {
printk(KERN_ERR "ide-tape: bh == NULL in "
"idetape_copy_stage_to_user\n");
- return;
+ return 0;
}
#endif /* IDETAPE_DEBUG_BUGS */
count = min(tape->b_count, n);
- copy_to_user(buf, tape->b_data, count);
+ if (copy_to_user(buf, tape->b_data, count))
+ return -EFAULT;
n -= count;
tape->b_data += count;
tape->b_count -= count;
@@ -2650,6 +2654,8 @@
}
}
}
+
+ return 0;
}
static void idetape_init_merge_stage(idetape_tape_t *tape)
@@ -3695,7 +3701,8 @@
return 0;
if (tape->merge_stage_size) {
actually_read = min((unsigned int)(tape->merge_stage_size), (unsigned int)count);
- idetape_copy_stage_to_user(tape, buf, tape->merge_stage, actually_read);
+ if ((rc = idetape_copy_stage_to_user(tape, buf, tape->merge_stage, actually_read)) < 0)
+ return rc;
buf += actually_read;
tape->merge_stage_size -= actually_read;
count -= actually_read;
@@ -3704,7 +3711,8 @@
bytes_read = idetape_add_chrdev_read_request(drive, tape->capabilities.ctl);
if (bytes_read <= 0)
goto finish;
- idetape_copy_stage_to_user(tape, buf, tape->merge_stage, bytes_read);
+ if ((rc = idetape_copy_stage_to_user(tape, buf, tape->merge_stage, bytes_read)) < 0)
+ return rc;
buf += bytes_read;
count -= bytes_read;
actually_read += bytes_read;
@@ -3714,7 +3722,8 @@
if (bytes_read <= 0)
goto finish;
temp = min((unsigned long)count, (unsigned long)bytes_read);
- idetape_copy_stage_to_user(tape, buf, tape->merge_stage, temp);
+ if ((rc = idetape_copy_stage_to_user(tape, buf, tape->merge_stage, temp)) < 0)
+ return rc;
actually_read += temp;
tape->merge_stage_size = bytes_read-temp;
}
@@ -3792,7 +3801,8 @@
}
#endif /* IDETAPE_DEBUG_BUGS */
actually_written = min((unsigned int)(tape->stage_size - tape->merge_stage_size), (unsigned int)count);
- idetape_copy_stage_from_user(tape, tape->merge_stage, buf, actually_written);
+ if ((retval = idetape_copy_stage_from_user(tape, tape->merge_stage, buf, actually_written)) < 0)
+ return retval;
buf += actually_written;
tape->merge_stage_size += actually_written;
count -= actually_written;
@@ -3805,7 +3815,8 @@
}
}
while (count >= tape->stage_size) {
- idetape_copy_stage_from_user(tape, tape->merge_stage, buf, tape->stage_size);
+ if ((retval = idetape_copy_stage_from_user(tape, tape->merge_stage, buf, tape->stage_size)) < 0)
+ return retval;
buf += tape->stage_size;
count -= tape->stage_size;
retval = idetape_add_chrdev_write_request(drive, tape->capabilities.ctl);
@@ -3815,7 +3826,8 @@
}
if (count) {
actually_written += count;
- idetape_copy_stage_from_user(tape, tape->merge_stage, buf, count);
+ if ((retval = idetape_copy_stage_from_user(tape, tape->merge_stage, buf, count)) < 0)
+ return retval;
tape->merge_stage_size += count;
}
return actually_written;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures
2004-11-28 16:32 [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures Jesper Juhl
@ 2004-11-28 17:34 ` Alan Cox
2004-11-28 19:39 ` Jesper Juhl
2004-11-29 22:47 ` Bill Davidsen
0 siblings, 2 replies; 7+ messages in thread
From: Alan Cox @ 2004-11-28 17:34 UTC (permalink / raw)
To: Jesper Juhl
Cc: Linux Kernel Mailing List, Gadi Oxman, Jens Axboe, Andrew Morton
On Sul, 2004-11-28 at 16:32, Jesper Juhl wrote:
> #endif /* IDETAPE_DEBUG_BUGS */
> count = min((unsigned int)(bh->b_size - atomic_read(&bh->b_count)), (unsigned int)n);
> - copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count);
> + if (copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count))
> + return -EFAULT;
> n -= count;
> atomic_add(count, &bh->b_count);
> buf += count;
If you do this then you don't fix up tape->bh for further operations.
Have you tested these changes including the I/O errors ?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures
2004-11-28 17:34 ` Alan Cox
@ 2004-11-28 19:39 ` Jesper Juhl
2004-11-28 21:16 ` Jesper Juhl
2004-11-29 22:47 ` Bill Davidsen
1 sibling, 1 reply; 7+ messages in thread
From: Jesper Juhl @ 2004-11-28 19:39 UTC (permalink / raw)
To: Alan Cox; +Cc: Linux Kernel Mailing List, Gadi Oxman, Jens Axboe, Andrew Morton
On Sun, 28 Nov 2004, Alan Cox wrote:
> On Sul, 2004-11-28 at 16:32, Jesper Juhl wrote:
> > #endif /* IDETAPE_DEBUG_BUGS */
> > count = min((unsigned int)(bh->b_size - atomic_read(&bh->b_count)), (unsigned int)n);
> > - copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count);
> > + if (copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count))
> > + return -EFAULT;
> > n -= count;
> > atomic_add(count, &bh->b_count);
> > buf += count;
>
> If you do this then you don't fix up tape->bh for further operations.
True, if copy_from_user fails it just bails out, I didn't see anything
really bad that could happen from that, but I'm an idiot, looking at it
again I guess it could probably mess up tape->bh pretty bad.
Guess I need to go back and look at this in greater detail.
Thank you for looking at it.
> Have you tested these changes including the I/O errors ?
>
As I noted in the [0/2] mail I don't have hardware to test these patches,
so they need to be reviewed.
--
Jesper Juhl
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures
2004-11-28 19:39 ` Jesper Juhl
@ 2004-11-28 21:16 ` Jesper Juhl
0 siblings, 0 replies; 7+ messages in thread
From: Jesper Juhl @ 2004-11-28 21:16 UTC (permalink / raw)
To: Jesper Juhl
Cc: Alan Cox, Linux Kernel Mailing List, Gadi Oxman, Jens Axboe,
Andrew Morton
On Sun, 28 Nov 2004, Jesper Juhl wrote:
> On Sun, 28 Nov 2004, Alan Cox wrote:
>
> > On Sul, 2004-11-28 at 16:32, Jesper Juhl wrote:
> > > #endif /* IDETAPE_DEBUG_BUGS */
> > > count = min((unsigned int)(bh->b_size - atomic_read(&bh->b_count)), (unsigned int)n);
> > > - copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count);
> > > + if (copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count))
> > > + return -EFAULT;
> > > n -= count;
> > > atomic_add(count, &bh->b_count);
> > > buf += count;
> >
> > If you do this then you don't fix up tape->bh for further operations.
>
> True, if copy_from_user fails it just bails out, I didn't see anything
> really bad that could happen from that, but I'm an idiot, looking at it
> again I guess it could probably mess up tape->bh pretty bad.
> Guess I need to go back and look at this in greater detail.
Does this really matter though? if copy_from_user fails here the function
bails out, and with the other changes I made the caller will bail out as
well with the effect that the entire IO operation will fail. and once a
new read or write is submitted we get a brand new tape->bh
Am I completely brain-dead or is it not in fact OK as I wrote it the first
time?
--
Jesper Juhl
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures
2004-11-28 17:34 ` Alan Cox
2004-11-28 19:39 ` Jesper Juhl
@ 2004-11-29 22:47 ` Bill Davidsen
2004-11-29 23:23 ` Jesper Juhl
1 sibling, 1 reply; 7+ messages in thread
From: Bill Davidsen @ 2004-11-29 22:47 UTC (permalink / raw)
To: Alan Cox
Cc: Jesper Juhl, Linux Kernel Mailing List, Gadi Oxman, Jens Axboe,
Andrew Morton
Alan Cox wrote:
> On Sul, 2004-11-28 at 16:32, Jesper Juhl wrote:
>
>> #endif /* IDETAPE_DEBUG_BUGS */
>> count = min((unsigned int)(bh->b_size - atomic_read(&bh->b_count)), (unsigned int)n);
>>- copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count);
>>+ if (copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf, count))
>>+ return -EFAULT;
>> n -= count;
>> atomic_add(count, &bh->b_count);
>> buf += count;
>
>
> If you do this then you don't fix up tape->bh for further operations.
> Have you tested these changes including the I/O errors ?
But (a) do you have enough information to backout or fixup correctly?
And (b) won't this result in the whole i/o being treated as invalid?
--
-bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
last possible moment - but no longer" -me
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures
2004-11-29 22:47 ` Bill Davidsen
@ 2004-11-29 23:23 ` Jesper Juhl
2004-11-30 12:03 ` Alan Cox
0 siblings, 1 reply; 7+ messages in thread
From: Jesper Juhl @ 2004-11-29 23:23 UTC (permalink / raw)
To: Bill Davidsen
Cc: Alan Cox, Linux Kernel Mailing List, Gadi Oxman, Jens Axboe,
Andrew Morton
On Mon, 29 Nov 2004, Bill Davidsen wrote:
> Alan Cox wrote:
> > On Sul, 2004-11-28 at 16:32, Jesper Juhl wrote:
> >
> > > #endif /* IDETAPE_DEBUG_BUGS */
> > > count = min((unsigned int)(bh->b_size -
> > > atomic_read(&bh->b_count)), (unsigned int)n);
> > > - copy_from_user(bh->b_data + atomic_read(&bh->b_count), buf,
> > > count);
> > > + if (copy_from_user(bh->b_data + atomic_read(&bh->b_count),
> > > buf, count))
> > > + return -EFAULT;
> > > n -= count;
> > > atomic_add(count, &bh->b_count);
> > > buf += count;
> >
> >
> > If you do this then you don't fix up tape->bh for further operations.
> > Have you tested these changes including the I/O errors ?
>
> But (a) do you have enough information to backout or fixup correctly? And (b)
> won't this result in the whole i/o being treated as invalid?
>
That was my original thought "if copy_from_user fails then something
really bad is happening and we should just fail the entire IO operation".
Then when Alan pointed out that I'd probably be messing up tape->bh I got
nervous becourse it seemed to me that he probably had a point, but when I
went back and looked at the code again I ended up with the conclusion that
even if we did mess it up it wouldn't actually matter since we'll be
failing the entire IO since I also added code in the caller to test for
the <0 return from this function and abort the entire operation in that
case.
Alan: Would you mind explaining why this is not safe? If there's something
I'm missing I'd really like to know.
--
Jesper Juhl
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures
2004-11-29 23:23 ` Jesper Juhl
@ 2004-11-30 12:03 ` Alan Cox
0 siblings, 0 replies; 7+ messages in thread
From: Alan Cox @ 2004-11-30 12:03 UTC (permalink / raw)
To: Jesper Juhl
Cc: Bill Davidsen, Linux Kernel Mailing List, Gadi Oxman, Jens Axboe,
Andrew Morton
On Llu, 2004-11-29 at 23:23, Jesper Juhl wrote:
> Alan: Would you mind explaining why this is not safe? If there's something
> I'm missing I'd really like to know.
Wrong question. Prove it is safe.
Otherwise you are risking errors in critical devices (think backups) for
the sake of fixing an essentially irrelevant limitation.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2004-11-30 13:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-11-28 16:32 [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures Jesper Juhl
2004-11-28 17:34 ` Alan Cox
2004-11-28 19:39 ` Jesper Juhl
2004-11-28 21:16 ` Jesper Juhl
2004-11-29 22:47 ` Bill Davidsen
2004-11-29 23:23 ` Jesper Juhl
2004-11-30 12:03 ` Alan Cox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).