Andrew Morton wrote:
Badari Pulavarty <[email protected]> wrote:
On Thu, 2006-05-11 at 11:47 -0700, Andrew Morton wrote:
Badari Pulavarty <[email protected]> wrote:
static ssize_t ep_aio_read_retry(struct kiocb *iocb)
{
struct kiocb_priv *priv = iocb->private;
- ssize_t status = priv->actual;
+ ssize_t len, total;
/* we "retry" to get the right mm context for this: */
- status = copy_to_user(priv->ubuf, priv->buf, priv->actual);
- if (unlikely(0 != status))
- status = -EFAULT;
- else
- status = priv->actual;
+
+ /* copy stuff into user buffers */
+ total = priv->actual;
+ len = 0;
+ for (i=0; i < priv->count; i++) {
+ ssize_t this = min(priv->iv[i].iov_len, total);
+
+ if (copy_to_user(priv->iv[i].iov_buf, priv->buf, this))
+ break;
+
+ total -= this;
+ len += this;
+ if (total <= 0)
+ break;
+ }
+
+ if (unlikely(len == 0))
+ len = -EFAULT;
This is still wrong, isn't it? Or am I looking at the same patch?
There's no way in which `total' can go negative, so it'd be nicer to just
test it for equality with zero. Because if it goes unexpectedly negative,
we _want_ the kernel to malfunction, rather than mysteriously covering
things up.
The final test there should be
if (unlikely(total != 0))
yes?
No. The original check is correct - we want to return EFAULT if
copy_to_user() failed and we haven't copied anything so far.
If we copied anything so far, we should return, that many bytes.
(like short-io).
oic. And we're sure that we cannot call into this code if someone's trying
a zero-sized read?
Either way, the below (which is faster!) will fix, yes?
--- 25/drivers/usb/gadget/inode.c~vectorize-aio_read-aio_write-methods-fix Thu May 11 11:53:41 2006
+++ 25-akpm/drivers/usb/gadget/inode.c Thu May 11 13:19:45 2006
@@ -567,18 +567,18 @@ static ssize_t ep_aio_read_retry(struct
for (i = 0; i < priv->count; i++) {
ssize_t this = min(priv->iv[i].iov_len, total);
- if (copy_to_user(priv->iv[i].iov_buf, priv->buf, this))
+ if (copy_to_user(priv->iv[i].iov_buf, priv->buf, this)) {
+ if (len == 0)
+ len = -EFAULT;
break;
+ }
total -= this;
len += this;
- if (total <= 0)
+ if (total == 0)
break;
}
- if (unlikely(len == 0))
- len = -EFAULT;
-
kfree(priv->buf);
kfree(priv);
aio_put_req(iocb);
_
Yes, this is good.
No one should call into this code with size == 0, since we should have
returned
success without doing any IO in the first place.
Thanks,
Badari
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
