| Message ID | 1424714436-19371-7-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 24, 2015 at 2:00 AM, Christoph Hellwig <hch@lst.de> wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > AIO_PREAD requests call ->aio_read() with iovec on caller's stack, so if > we are going to access it asynchronously, we'd better get ourselves > a copy - the one on kernel stack of aio_run_iocb() won't be there > anymore. function/f_fs.c take care of doing that, legacy/inode.c > doesn't... > > Cc: stable@vger.kernel.org > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/usb/gadget/legacy/inode.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c > index db49ec4..9fbbaa0 100644 > --- a/drivers/usb/gadget/legacy/inode.c > +++ b/drivers/usb/gadget/legacy/inode.c > @@ -566,7 +566,6 @@ static ssize_t ep_copy_to_user(struct kiocb_priv *priv) > if (total == 0) > break; > } > - > return len; > } > > @@ -585,6 +584,7 @@ static void ep_user_copy_worker(struct work_struct *work) > aio_complete(iocb, ret, ret); > > kfree(priv->buf); > + kfree(priv->iv); > kfree(priv); > } > > @@ -605,6 +605,7 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req) > */ > if (priv->iv == NULL || unlikely(req->actual == 0)) { > kfree(req->buf); > + kfree(priv->iv); > kfree(priv); > iocb->private = NULL; > /* aio_complete() reports bytes-transferred _and_ faults */ > @@ -640,7 +641,7 @@ ep_aio_rwtail( > struct usb_request *req; > ssize_t value; > > - priv = kmalloc(sizeof *priv, GFP_KERNEL); > + priv = kzalloc(sizeof *priv, GFP_KERNEL); > if (!priv) { > value = -ENOMEM; > fail: > @@ -649,7 +650,14 @@ fail: > } > iocb->private = priv; > priv->iocb = iocb; > - priv->iv = iv; > + if (iv) { > + priv->iv = kmemdup(iv, nr_segs * sizeof(struct iovec), > + GFP_KERNEL); > + if (!priv->iv) { > + kfree(priv); > + goto fail; > + } > + } It should be simpler and more efficient to allocate 'iv' piggyback inside 'priv'. > priv->nr_segs = nr_segs; > INIT_WORK(&priv->work, ep_user_copy_worker); > > @@ -689,6 +697,7 @@ fail: > mutex_unlock(&epdata->lock); > > if (unlikely(value)) { > + kfree(priv->iv); > kfree(priv); > put_ep(epdata); > } else > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks, Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 08 2015, Ming Lei <tom.leiming@gmail.com> wrote: > On Tue, Feb 24, 2015 at 2:00 AM, Christoph Hellwig <hch@lst.de> wrote: >> From: Al Viro <viro@zeniv.linux.org.uk> >> >> AIO_PREAD requests call ->aio_read() with iovec on caller's stack, so if >> we are going to access it asynchronously, we'd better get ourselves >> a copy - the one on kernel stack of aio_run_iocb() won't be there >> anymore. function/f_fs.c take care of doing that, legacy/inode.c >> doesn't... >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Acked-by: Michal Nazarewicz <mina86@mina86.com> but at the same time: >> @@ -649,7 +650,14 @@ fail: >> } >> iocb->private = priv; >> priv->iocb = iocb; >> - priv->iv = iv; >> + if (iv) { >> + priv->iv = kmemdup(iv, nr_segs * sizeof(struct iovec), >> + GFP_KERNEL); >> + if (!priv->iv) { >> + kfree(priv); >> + goto fail; >> + } >> + } > > It should be simpler and more efficient to allocate 'iv' piggyback > inside 'priv'. +1 priv = kmalloc(sizeof *priv + (iv ? nr_segs * sizeof *iv : 0), GFP_KERNEL); … priv->iv = iv ? (void*)(priv + 1) : NULL; >> priv->nr_segs = nr_segs; >> INIT_WORK(&priv->work, ep_user_copy_worker); >>
diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c index db49ec4..9fbbaa0 100644 --- a/drivers/usb/gadget/legacy/inode.c +++ b/drivers/usb/gadget/legacy/inode.c @@ -566,7 +566,6 @@ static ssize_t ep_copy_to_user(struct kiocb_priv *priv) if (total == 0) break; } - return len; } @@ -585,6 +584,7 @@ static void ep_user_copy_worker(struct work_struct *work) aio_complete(iocb, ret, ret); kfree(priv->buf); + kfree(priv->iv); kfree(priv); } @@ -605,6 +605,7 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req) */ if (priv->iv == NULL || unlikely(req->actual == 0)) { kfree(req->buf); + kfree(priv->iv); kfree(priv); iocb->private = NULL; /* aio_complete() reports bytes-transferred _and_ faults */ @@ -640,7 +641,7 @@ ep_aio_rwtail( struct usb_request *req; ssize_t value; - priv = kmalloc(sizeof *priv, GFP_KERNEL); + priv = kzalloc(sizeof *priv, GFP_KERNEL); if (!priv) { value = -ENOMEM; fail: @@ -649,7 +650,14 @@ fail: } iocb->private = priv; priv->iocb = iocb; - priv->iv = iv; + if (iv) { + priv->iv = kmemdup(iv, nr_segs * sizeof(struct iovec), + GFP_KERNEL); + if (!priv->iv) { + kfree(priv); + goto fail; + } + } priv->nr_segs = nr_segs; INIT_WORK(&priv->work, ep_user_copy_worker); @@ -689,6 +697,7 @@ fail: mutex_unlock(&epdata->lock); if (unlikely(value)) { + kfree(priv->iv); kfree(priv); put_ep(epdata); } else