Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 11 May 2006 13:45:29 -0700 | From | Badari Pulavarty <> | Subject | Re: [PATCH 1/4] Vectorize aio_read/aio_write methods |
| |
Andrew Morton wrote:
>Badari Pulavarty <pbadari@us.ibm.com> wrote: > >>On Thu, 2006-05-11 at 11:47 -0700, Andrew Morton wrote: >> >>>Badari Pulavarty <pbadari@us.ibm.com> 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 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/
| ![\](/images/icornerr.gif) |