Messages in this thread | | | Date | Tue, 30 Nov 2004 00:23:51 +0100 (CET) | From | Jesper Juhl <> | Subject | Re: [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user() failures |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |