| Message ID | 20240304235715.3790858-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs/aio: Check IOCB_AIO_RW before the struct aio_kiocb conversion | expand |
On Mon, Mar 04, 2024 at 03:57:15PM -0800, Bart Van Assche wrote: > The first kiocb_set_cancel_fn() argument may point at a struct kiocb > that is not embedded inside struct aio_kiocb. With the current code, > depending on the compiler, the req->ki_ctx read happens either before > the IOCB_AIO_RW test or after that test. Move the req->ki_ctx read such > that it is guaranteed that the IOCB_AIO_RW test happens first. > > Reported-by: Eric Biggers <ebiggers@kernel.org> > Cc: Benjamin LaHaise <ben@communityfibre.ca> > Cc: Eric Biggers <ebiggers@google.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Avi Kivity <avi@scylladb.com> > Cc: Sandeep Dhavale <dhavale@google.com> > Cc: Jens Axboe <axboe@kernel.dk> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Kent Overstreet <kent.overstreet@linux.dev> > Cc: stable@vger.kernel.org > Fixes: b820de741ae4 ("fs/aio: Restrict kiocb_set_cancel_fn() to I/O submitted via libaio") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- Can I please get a review from Eric and/or Benjamin, please? > fs/aio.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/aio.c b/fs/aio.c > index da18dbcfcb22..9cdaa2faa536 100644 > --- a/fs/aio.c > +++ b/fs/aio.c > @@ -589,8 +589,8 @@ static int aio_setup_ring(struct kioctx *ctx, unsigned int nr_events) > > void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel) > { > - struct aio_kiocb *req = container_of(iocb, struct aio_kiocb, rw); > - struct kioctx *ctx = req->ki_ctx; > + struct aio_kiocb *req; > + struct kioctx *ctx; > unsigned long flags; > > /* > @@ -600,9 +600,13 @@ void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel) > if (!(iocb->ki_flags & IOCB_AIO_RW)) > return; > > + req = container_of(iocb, struct aio_kiocb, rw); > + > if (WARN_ON_ONCE(!list_empty(&req->ki_list))) > return; > > + ctx = req->ki_ctx; > + > spin_lock_irqsave(&ctx->ctx_lock, flags); > list_add_tail(&req->ki_list, &ctx->active_reqs); > req->ki_cancel = cancel;
On 3/4/24 4:57 PM, Bart Van Assche wrote: > The first kiocb_set_cancel_fn() argument may point at a struct kiocb > that is not embedded inside struct aio_kiocb. With the current code, > depending on the compiler, the req->ki_ctx read happens either before > the IOCB_AIO_RW test or after that test. Move the req->ki_ctx read such > that it is guaranteed that the IOCB_AIO_RW test happens first. Reviewed-by: Jens Axboe <axboe@kernel.dk>
On Mon, Mar 04, 2024 at 03:57:15PM -0800, Bart Van Assche wrote: > The first kiocb_set_cancel_fn() argument may point at a struct kiocb > that is not embedded inside struct aio_kiocb. With the current code, > depending on the compiler, the req->ki_ctx read happens either before > the IOCB_AIO_RW test or after that test. Move the req->ki_ctx read such > that it is guaranteed that the IOCB_AIO_RW test happens first. > > Reported-by: Eric Biggers <ebiggers@kernel.org> > Cc: Benjamin LaHaise <ben@communityfibre.ca> > Cc: Eric Biggers <ebiggers@google.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Avi Kivity <avi@scylladb.com> > Cc: Sandeep Dhavale <dhavale@google.com> > Cc: Jens Axboe <axboe@kernel.dk> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Kent Overstreet <kent.overstreet@linux.dev> > Cc: stable@vger.kernel.org > Fixes: b820de741ae4 ("fs/aio: Restrict kiocb_set_cancel_fn() to I/O submitted via libaio") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > fs/aio.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/aio.c b/fs/aio.c > index da18dbcfcb22..9cdaa2faa536 100644 > --- a/fs/aio.c > +++ b/fs/aio.c > @@ -589,8 +589,8 @@ static int aio_setup_ring(struct kioctx *ctx, unsigned int nr_events) > > void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel) > { > - struct aio_kiocb *req = container_of(iocb, struct aio_kiocb, rw); > - struct kioctx *ctx = req->ki_ctx; > + struct aio_kiocb *req; > + struct kioctx *ctx; > unsigned long flags; > > /* > @@ -600,9 +600,13 @@ void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel) > if (!(iocb->ki_flags & IOCB_AIO_RW)) > return; > > + req = container_of(iocb, struct aio_kiocb, rw); > + > if (WARN_ON_ONCE(!list_empty(&req->ki_list))) > return; > > + ctx = req->ki_ctx; > + > spin_lock_irqsave(&ctx->ctx_lock, flags); > list_add_tail(&req->ki_list, &ctx->active_reqs); > req->ki_cancel = cancel; Reviewed-by: Eric Biggers <ebiggers@google.com> - Eric
diff --git a/fs/aio.c b/fs/aio.c index da18dbcfcb22..9cdaa2faa536 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -589,8 +589,8 @@ static int aio_setup_ring(struct kioctx *ctx, unsigned int nr_events) void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel) { - struct aio_kiocb *req = container_of(iocb, struct aio_kiocb, rw); - struct kioctx *ctx = req->ki_ctx; + struct aio_kiocb *req; + struct kioctx *ctx; unsigned long flags; /* @@ -600,9 +600,13 @@ void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel) if (!(iocb->ki_flags & IOCB_AIO_RW)) return; + req = container_of(iocb, struct aio_kiocb, rw); + if (WARN_ON_ONCE(!list_empty(&req->ki_list))) return; + ctx = req->ki_ctx; + spin_lock_irqsave(&ctx->ctx_lock, flags); list_add_tail(&req->ki_list, &ctx->active_reqs); req->ki_cancel = cancel;
The first kiocb_set_cancel_fn() argument may point at a struct kiocb that is not embedded inside struct aio_kiocb. With the current code, depending on the compiler, the req->ki_ctx read happens either before the IOCB_AIO_RW test or after that test. Move the req->ki_ctx read such that it is guaranteed that the IOCB_AIO_RW test happens first. Reported-by: Eric Biggers <ebiggers@kernel.org> Cc: Benjamin LaHaise <ben@communityfibre.ca> Cc: Eric Biggers <ebiggers@google.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Avi Kivity <avi@scylladb.com> Cc: Sandeep Dhavale <dhavale@google.com> Cc: Jens Axboe <axboe@kernel.dk> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Kent Overstreet <kent.overstreet@linux.dev> Cc: stable@vger.kernel.org Fixes: b820de741ae4 ("fs/aio: Restrict kiocb_set_cancel_fn() to I/O submitted via libaio") Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- fs/aio.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)