On Fri, May 02, 2014 at 10:56:32AM +0200, Lukáš Czerner wrote:
> On Thu, 1 May 2014, Benjamin LaHaise wrote:
>
> > Date: Thu, 1 May 2014 09:07:09 -0400
> > From: Benjamin LaHaise <bcrl@xxxxxxxxx>
> > To: torvalds@xxxxxxxxxxxxxxxxxxxx
> > Cc: linux-aio@xxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx,
> > stable@xxxxxxxxxxxxxxx, Leon Yu <chianglungyu@xxxxxxxxx>
> > Subject: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().
> >
> > iovec should be reclaimed whenever caller of rw_copy_check_uvector() returns,
> > but it doesn't hold when failure happens right after aio_setup_vectored_rw().
> >
> > Fix that in a such way to avoid hairy goto.
>
> As I already replied to Leon,
>
> this does not seem right.
>
> >
> > Signed-off-by: Leon Yu <chianglungyu@xxxxxxxxx>
> > Signed-off-by: Benjamin LaHaise <bcrl@xxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > fs/aio.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/aio.c b/fs/aio.c
> > index 2adbb03..a0ed6c7 100644
> > --- a/fs/aio.c
> > +++ b/fs/aio.c
> > @@ -1327,10 +1327,8 @@ rw_common:
> > &iovec, compat)
> > : aio_setup_single_vector(req, rw, buf, &nr_segs,
> > iovec);
> > - if (ret)
> > - return ret;
> > -
> > - ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
>
> here ret could be possibly set to a positive number.
>
How?
ret = (opcode == IOCB_CMD_PREADV ||
opcode == IOCB_CMD_PWRITEV)
? aio_setup_vectored_rw(req, rw, buf, &nr_segs,
&iovec, compat)
: aio_setup_single_vector(req, rw, buf, &nr_segs,
iovec);
Where aio_setup_vectored_rw:
if (ret < 0)
return ret;
[..]
return 0;
and aio_setup_single_vector:
if (unlikely(!access_ok(!rw, buf, kiocb->ki_nbytes)))
return -EFAULT;
[..]
return 0;
Both functions are returning ssize_t, thus it's either 0 on success or
negative on failure.
"if (ret)" replaced by "if (ret < 0)" should indeed set off alarm bells,
but turns it turns out to be fine here.
> > + if (!ret)
> > + ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
> > if (ret < 0) {
>
So this check is fine and cleanup will be called.
However, there is a yet to be merged patch which fixes actual problem
which is weird rw_copy_check_uvector semantics:
https://lkml.org/lkml/2014/4/25/778
rendering this patch unnecessary
--
Mateusz Guzik
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
