On Sat, Jun 21, 2014 at 07:09:22PM -0400, Theodore Ts'o wrote:
> On Sat, Jun 21, 2014 at 06:53:07AM +0100, Al Viro wrote:
> >
> > ed include/linux/uio.h <<EOF
> > /iov_iter_truncate/s/size_t/u64/
> > w
> > q
> > EOF
> >
> > Could you check if that fixes the sucker?
>
> The following patch (attached at the end) appears to fix the problem,
> but looking at uio.h, I'm completely confused about *why* it fixes the
> problem. In particular, iov_iter_iovec() makes no sense to me at all,
> and I don't understand how the calculation of iov_len makes any sense:
>
> .iov_len = min(iter->count,
> iter->iov->iov_len - iter->iov_offset),
Eh? We have iov[0].iov_base..iov[0].iov_base+iov[0].iov_len - 1 for
area covered by the first iovec. First iov_offset bytes have already
been consumed. And at most count bytes matter. So yes, this iov_len
will give you equivalent first iovec.
Said that, iov_iter_iovec() will die shortly - it's a rudiment of older
code, with almost no users left. But yes, it is correct.
> It also looks like uio.h is mostly about offsets to memory pointers,
> and so why this would make a difference when the issue is the block
> device offset goes above 2**30?
It is, and your patch is a huge overkill.
> There must be deep magic going on here, and so I don't know if your
> s/size_t/u64/g substitation also extends to the various functions that
> have size_t in them:
No, it does not. It's specifically about iov_iter_truncate(); moreover,
it matters to only one caller of that sucker. Namely,
static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
{
struct file *file = iocb->ki_filp;
struct inode *bd_inode = file->f_mapping->host;
loff_t size = i_size_read(bd_inode);
loff_t pos = iocb->ki_pos;
if (pos >= size)
return 0;
size -= pos;
iov_iter_truncate(to, size);
return generic_file_read_iter(iocb, to);
}
What happens here is capping to->count, to guarantee that we won't even look
at anything past the end of block device. Alternative fix would be to
have
if (pos >= size)
return 0;
if (to->size + pos > size) {
/* note that size - pos fits into size_t in this case,
* so it's OK to pass it to iov_iter_truncate().
*/
iov_iter_truncate(to, size - pos);
}
return generic_file_read_iter(iocb, to);
in there. Other callers are passing it size_t values already, so we don't
need similar checks there.
Or we can make iov_iter_truncate() take an arbitrary u64 argument, seeing that
it's inlined anyway. IMO it's more robust that way...
Anyway, does the following alone fix the problem you are seeing?
diff --git a/include/linux/uio.h b/include/linux/uio.h
index ddfdb53..dbb02d4 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -94,7 +94,7 @@ static inline size_t iov_iter_count(struct iov_iter *i)
return i->count;
}
-static inline void iov_iter_truncate(struct iov_iter *i, size_t count)
+static inline void iov_iter_truncate(struct iov_iter *i, u64 count)
{
if (i->count > count)
i->count = count;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
