Messages in this thread | | | Date | Fri, 13 Jan 2012 15:38:04 +0800 | From | Dave Young <> | Subject | Re: [PATCH] loop: zero fill bio instead of return -EIO for partial read |
| |
On 01/08/2012 09:54 PM, Dave Young wrote:
> On 01/07/2012 03:22 AM, Jeff Moyer wrote: > >> Dave Young <dyoung@redhat.com> writes: >> >>> commit 8268f5a7415d914fc855a86aa2284ac819dc6b2e trying to fix the loop device >>> partial read information leak problem. But it changed the semantics of read >>> behavior. When we read beyond the end of the device we should get 0 bytes, >>> which is normal behavior, we should not just return -EIO >>> >>> Instead of return -EIO, zero out the bio to avoid information leak in case of >>> partail read. >> >> I tested the patch with a program which patterns the loop device, >> truncates the backing file, and then performs preads from various >> offsets within the loop device, validates the return values and inspects >> the contents. With this patch, everything works as expected. > > > Many thanks for review and test > >> >> By the way, truncating the backing file for a loop device is insane. >> Why would you do that? Also, if you really want all of the data gone, >> you'll have to flush the contents of the buffer cache for the loop >> device first. It's quite the head scratcher the first time you truncate >> a file, wait a few seconds, and then witness the file size grow. ;-) > > > I think nobody will intend to do that, but random operations could cause > this happen.. > >> >> Reviewed-by: Jeff Moyer <jmoyer@redhat.com> >> >>> >>> Signed-off-by: Dave Young <dyoung@redhat.com> >>> --- >>> drivers/block/loop.c | 24 +++++++++++++----------- >>> 1 file changed, 13 insertions(+), 11 deletions(-) >>> >>> --- linux-2.6.orig/drivers/block/loop.c 2012-01-06 11:19:48.000000000 +0800 >>> +++ linux-2.6/drivers/block/loop.c 2012-01-06 11:20:18.842630580 +0800 >>> @@ -357,14 +357,14 @@ lo_direct_splice_actor(struct pipe_inode >>> return __splice_from_pipe(pipe, sd, lo_splice_actor); >>> } >>> >>> -static int >>> +static ssize_t >>> do_lo_receive(struct loop_device *lo, >>> struct bio_vec *bvec, int bsize, loff_t pos) >>> { >>> struct lo_read_data cookie; >>> struct splice_desc sd; >>> struct file *file; >>> - long retval; >>> + ssize_t retval; >>> >>> cookie.lo = lo; >>> cookie.page = bvec->bv_page; >>> @@ -380,26 +380,28 @@ do_lo_receive(struct loop_device *lo, >>> file = lo->lo_backing_file; >>> retval = splice_direct_to_actor(file, &sd, lo_direct_splice_actor); >>> >>> - if (retval < 0) >>> - return retval; >>> - if (retval != bvec->bv_len) >>> - return -EIO; >>> - return 0; >>> + return retval; >>> } >>> >>> static int >>> lo_receive(struct loop_device *lo, struct bio *bio, int bsize, loff_t pos) >>> { >>> struct bio_vec *bvec; >>> - int i, ret = 0; >>> + ssize_t s; >>> + int i; >>> >>> bio_for_each_segment(bvec, bio, i) { >>> - ret = do_lo_receive(lo, bvec, bsize, pos); >>> - if (ret < 0) >>> + s = do_lo_receive(lo, bvec, bsize, pos); >>> + if (s < 0) >>> + return s; >>> + >>> + if (s != bvec->bv_len) { >>> + zero_fill_bio(bio); >>> break; >>> + } >>> pos += bvec->bv_len; >>> } >>> - return ret; >>> + return 0; >>> } >>> >>> static int do_bio_filebacked(struct loop_device *lo, struct bio *bio) > > >
Jens, what do you think about this patch? Could you take this?
-- Thanks Dave
| |