| Message ID | 20200706125135.23511-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: do not access the kiocb after aio reqeusts | expand |
On Mon, 2020-07-06 at 08:51 -0400, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > In aio case, if the completion comes very fast just before the > ceph_read_iter() returns to fs/aio.c, the kiocb will be freed in > the completion callback, then if ceph_read_iter() access again > we will potentially hit the use-after-free bug. > > URL: https://tracker.ceph.com/issues/45649 > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/file.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 160644ddaeed..704bae794054 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -1538,6 +1538,7 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) > struct inode *inode = file_inode(filp); > struct ceph_inode_info *ci = ceph_inode(inode); > struct page *pinned_page = NULL; > + bool direct_lock = false; Looks good. I made a slight change to this patch and had it initialize this variable to iocb->ki_flags & IOCB_DIRECT, and then use that rather than setting direct_lock in the true case. Merged into testing. Thanks! > ssize_t ret; > int want, got = 0; > int retry_op = 0, read = 0; > @@ -1546,10 +1547,12 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) > dout("aio_read %p %llx.%llx %llu~%u trying to get caps on %p\n", > inode, ceph_vinop(inode), iocb->ki_pos, (unsigned)len, inode); > > - if (iocb->ki_flags & IOCB_DIRECT) > + if (iocb->ki_flags & IOCB_DIRECT) { > ceph_start_io_direct(inode); > - else > + direct_lock = true; > + } else { > ceph_start_io_read(inode); > + } > > if (fi->fmode & CEPH_FILE_MODE_LAZY) > want = CEPH_CAP_FILE_CACHE | CEPH_CAP_FILE_LAZYIO; > @@ -1603,7 +1606,7 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) > } > ceph_put_cap_refs(ci, got); > > - if (iocb->ki_flags & IOCB_DIRECT) > + if (direct_lock) > ceph_end_io_direct(inode); > else > ceph_end_io_read(inode);
On 2020/7/7 0:17, Jeff Layton wrote: > On Mon, 2020-07-06 at 08:51 -0400, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> In aio case, if the completion comes very fast just before the >> ceph_read_iter() returns to fs/aio.c, the kiocb will be freed in >> the completion callback, then if ceph_read_iter() access again >> we will potentially hit the use-after-free bug. >> >> URL: https://tracker.ceph.com/issues/45649 >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/file.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >> index 160644ddaeed..704bae794054 100644 >> --- a/fs/ceph/file.c >> +++ b/fs/ceph/file.c >> @@ -1538,6 +1538,7 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) >> struct inode *inode = file_inode(filp); >> struct ceph_inode_info *ci = ceph_inode(inode); >> struct page *pinned_page = NULL; >> + bool direct_lock = false; > Looks good. I made a slight change to this patch and had it initialize > this variable to iocb->ki_flags & IOCB_DIRECT, and then use that rather > than setting direct_lock in the true case. Merged into testing. Okay, looks good to me. Thanks Jeff. > Thanks! > >> ssize_t ret; >> int want, got = 0; >> int retry_op = 0, read = 0; >> @@ -1546,10 +1547,12 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) >> dout("aio_read %p %llx.%llx %llu~%u trying to get caps on %p\n", >> inode, ceph_vinop(inode), iocb->ki_pos, (unsigned)len, inode); >> >> - if (iocb->ki_flags & IOCB_DIRECT) >> + if (iocb->ki_flags & IOCB_DIRECT) { >> ceph_start_io_direct(inode); >> - else >> + direct_lock = true; >> + } else { >> ceph_start_io_read(inode); >> + } >> >> if (fi->fmode & CEPH_FILE_MODE_LAZY) >> want = CEPH_CAP_FILE_CACHE | CEPH_CAP_FILE_LAZYIO; >> @@ -1603,7 +1606,7 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) >> } >> ceph_put_cap_refs(ci, got); >> >> - if (iocb->ki_flags & IOCB_DIRECT) >> + if (direct_lock) >> ceph_end_io_direct(inode); >> else >> ceph_end_io_read(inode);
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 160644ddaeed..704bae794054 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1538,6 +1538,7 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) struct inode *inode = file_inode(filp); struct ceph_inode_info *ci = ceph_inode(inode); struct page *pinned_page = NULL; + bool direct_lock = false; ssize_t ret; int want, got = 0; int retry_op = 0, read = 0; @@ -1546,10 +1547,12 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) dout("aio_read %p %llx.%llx %llu~%u trying to get caps on %p\n", inode, ceph_vinop(inode), iocb->ki_pos, (unsigned)len, inode); - if (iocb->ki_flags & IOCB_DIRECT) + if (iocb->ki_flags & IOCB_DIRECT) { ceph_start_io_direct(inode); - else + direct_lock = true; + } else { ceph_start_io_read(inode); + } if (fi->fmode & CEPH_FILE_MODE_LAZY) want = CEPH_CAP_FILE_CACHE | CEPH_CAP_FILE_LAZYIO; @@ -1603,7 +1606,7 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) } ceph_put_cap_refs(ci, got); - if (iocb->ki_flags & IOCB_DIRECT) + if (direct_lock) ceph_end_io_direct(inode); else ceph_end_io_read(inode);