| Message ID | 20201210013828.417576-1-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] zonefs: fix page reference and BIO leak | expand |
On 12/9/20 17:41, Damien Le Moal wrote: > In zonefs_file_dio_append(), the pages obtained using > bio_iov_iter_get_pages() are not released on completion of the > REQ_OP_APPEND BIO, nor when bio_iov_iter_get_pages() fails. > Furthermore, a call to bio_put() is missing when > bio_iov_iter_get_pages() fails. > > Fix these resource leaks by adding BIO resource release code (bio_put()i I think extra 'i' above needs to be removed at the time of applying the patch. > and bio_release_pages()) at the end of the function after the BIO > execution and add a jump to this resource cleanup code in case of > bio_iov_iter_get_pages() failure. > > While at it, also fix the call to task_io_account_write() to be passed > the correct BIO size instead of bio_iov_iter_get_pages() return value. > > Reported-by: Christoph Hellwig <hch@lst.de> > Fixes: 02ef12a663c7 ("zonefs: use REQ_OP_ZONE_APPEND for sync DIO") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> Looks good. Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
On 10/12/2020 02:38, Damien Le Moal wrote: > In zonefs_file_dio_append(), the pages obtained using > bio_iov_iter_get_pages() are not released on completion of the > REQ_OP_APPEND BIO, nor when bio_iov_iter_get_pages() fails. > Furthermore, a call to bio_put() is missing when > bio_iov_iter_get_pages() fails. > > Fix these resource leaks by adding BIO resource release code (bio_put()i > and bio_release_pages()) at the end of the function after the BIO > execution and add a jump to this resource cleanup code in case of > bio_iov_iter_get_pages() failure. > > While at it, also fix the call to task_io_account_write() to be passed > the correct BIO size instead of bio_iov_iter_get_pages() return value. > > Reported-by: Christoph Hellwig <hch@lst.de> > Fixes: 02ef12a663c7 ("zonefs: use REQ_OP_ZONE_APPEND for sync DIO") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > fs/zonefs/super.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c > index ff5930be096c..bec47f2d074b 100644 > --- a/fs/zonefs/super.c > +++ b/fs/zonefs/super.c > @@ -691,21 +691,23 @@ static ssize_t zonefs_file_dio_append(struct kiocb *iocb, struct iov_iter *from) > bio->bi_opf |= REQ_FUA; > > ret = bio_iov_iter_get_pages(bio, from); > - if (unlikely(ret)) { > - bio_io_error(bio); > - return ret; > - } > + if (unlikely(ret)) > + goto out_release; > + > size = bio->bi_iter.bi_size; > - task_io_account_write(ret); > + task_io_account_write(size); > > if (iocb->ki_flags & IOCB_HIPRI) > bio_set_polled(bio, iocb); > > ret = submit_bio_wait(bio); > > + zonefs_file_write_dio_end_io(iocb, size, ret, 0); > + > +out_release: > + bio_release_pages(bio, false); > bio_put(bio); > > - zonefs_file_write_dio_end_io(iocb, size, ret, 0); > if (ret >= 0) { > iocb->ki_pos += size; > return size; > Aren't we loosing bio->bi_status = BLK_STS_IOERR in case bio_iov_iter_get_pages() fails now?
On Thu, Dec 10, 2020 at 07:03:08AM +0000, Johannes Thumshirn wrote:
> Aren't we loosing bio->bi_status = BLK_STS_IOERR in case bio_iov_iter_get_pages() fails now?
We do, but it does not matter because nothing actually looks at
->bi_status in this failure path.
On 10/12/2020 08:06, Christoph Hellwig wrote: > On Thu, Dec 10, 2020 at 07:03:08AM +0000, Johannes Thumshirn wrote: >> Aren't we loosing bio->bi_status = BLK_STS_IOERR in case bio_iov_iter_get_pages() fails now? > > We do, but it does not matter because nothing actually looks at > ->bi_status in this failure path. > Right we never pass the bio to the block layer in the error case. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c index ff5930be096c..bec47f2d074b 100644 --- a/fs/zonefs/super.c +++ b/fs/zonefs/super.c @@ -691,21 +691,23 @@ static ssize_t zonefs_file_dio_append(struct kiocb *iocb, struct iov_iter *from) bio->bi_opf |= REQ_FUA; ret = bio_iov_iter_get_pages(bio, from); - if (unlikely(ret)) { - bio_io_error(bio); - return ret; - } + if (unlikely(ret)) + goto out_release; + size = bio->bi_iter.bi_size; - task_io_account_write(ret); + task_io_account_write(size); if (iocb->ki_flags & IOCB_HIPRI) bio_set_polled(bio, iocb); ret = submit_bio_wait(bio); + zonefs_file_write_dio_end_io(iocb, size, ret, 0); + +out_release: + bio_release_pages(bio, false); bio_put(bio); - zonefs_file_write_dio_end_io(iocb, size, ret, 0); if (ret >= 0) { iocb->ki_pos += size; return size;
In zonefs_file_dio_append(), the pages obtained using bio_iov_iter_get_pages() are not released on completion of the REQ_OP_APPEND BIO, nor when bio_iov_iter_get_pages() fails. Furthermore, a call to bio_put() is missing when bio_iov_iter_get_pages() fails. Fix these resource leaks by adding BIO resource release code (bio_put()i and bio_release_pages()) at the end of the function after the BIO execution and add a jump to this resource cleanup code in case of bio_iov_iter_get_pages() failure. While at it, also fix the call to task_io_account_write() to be passed the correct BIO size instead of bio_iov_iter_get_pages() return value. Reported-by: Christoph Hellwig <hch@lst.de> Fixes: 02ef12a663c7 ("zonefs: use REQ_OP_ZONE_APPEND for sync DIO") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- fs/zonefs/super.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)