Re: [PATCH][2/2] ide-tape: small cleanups - handle copy_to|from_user()failures

[Jesper Juhl]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/